RefactoringAPIReview

Refactoring API Review

Submitter:

  • Jan Becicka (JB)

Reviewers:

  • Miloslav Metelka (voting) (MM)
  • Tomas Pavek (voting) (TP)
  • Petr Pisl (voting) (PP)
  • Sandip Chitale (voting) (SC)
  • Jesse Glick (JG)
  • Jarda Tulach (Y)
  • Sonali Kochar (SK)

Issue: http://www.netbeans.org/issues/show_bug.cgi?id=89605

Prg05, Lucerna, Tue 13-Feb-2007, 4pm-5pm CET.
Ready access: 3309157
Sun: 44444

Talks during the review

This API review purpose is to identify TCAs and TCRs and once they are implemented announce the time since users of the API can rely on its stability.


JB: Gives introduction, this is actually third review of this API: http://openide.netbeans.org/tutorial/reviews/opinions_52016.html http://openide.netbeans.org/tutorial/reviews/opinions_41519.html JB: Incompatible rewrite because it has been generalized, but Jan tried to keep the usages similar

MM: Did you consider generification? JB: I do not know how to do it. SC: Cannot you use Lookup instead of Object[[ | ]]? JB: You still do not know what is inside the lookup. PP: But with lookup you can say what JB: Submitter was forced to agree. *TCR*: Use lookup everywhere ;-) TP: So maybe lookup is fine if you get it, but maybe it is better to use Object[[ | ]] JT: Imho, you should take current selection (Utilities.actionGlobalContext, or Node.getLookup), send it directly to (Move)Refactoring without any conversion and then all the plugins should analyse the content, the API should be "opaque".


PP: How will you find out that there is an object that nobody wants to handle? JB: Refactoring provides impl for plain FileObject JT: How you handle move of java then? JB: File is modified on place, then the plain refactoring moves it to new place.

PP: How the package rename is handled for HTML files? JB: You need to handle rename of non-recursive folder. PP: A/B/C folder, how do I rename just A to X SC: The rename plug in will search for *NonRecursiveFolder*, and do whatever it wants

PP: Javadoc is wrong JB: File an issue.


JB: *JG03* - SC: BackupFacility works only with FileObjects, can in future we do this for schema in database? JB: Originally I did not want it at all. I wanted to use localhistory module, but this module does not have any API, and it is hard for me to depend on this API. No plans, to work with databases. JT: Should not we merge localhistory and this BackupFunctionality. SC: Is this used during undo? Yes. Client call should still use the localhistory mechanism. JB: Correct solution is to reuse the backup stuff from localhistory, also the integration SC: Is it possible to register own BackupFacilities? PP: Plugable BackupFacility. *TCA*: make plugable for 6.0.

PP: Will the api be official? JB: No, just under development. JT: Ok, will not be listed in javadocs for 6.0. JB: Ok.

SC: Javadoc points: Java refactoring is using some maps. We need some documentation.

  • TCR*: Document java API using <api group="property"/>

SC: Sharing of the actions menu: We need standard for all people who implement the Action provider. PP: How to write new refactoring actions? JB: Out of scope of the refactoring, it is like new actions. SC: There should be a standard way of how people should do it. SC: The actions are added into Loaders/x-java/Actions or Loaders/x-jsp/Actions SC: More HIE question, how we make sure the refactoring actions in menu are in right place.

  • TCR*: Document as a arch-usecase how to register new refactoring action or how to add existing action into my dataobjects popup.

PP: Refactoring actions should be transparent and be automatically called when drag and drop, delete, copy thru clipboard is done. JB: Move and dnd is handled. Delete waits for openide/explorer. And inplace rename can be handled only for one's own nodes. SC: what about dnd of packages in java JB: I hope it will work. Refactoring API is fine, possibly we have a bug in java impl.


JT: *TCR* Do not subclass AbstractLookup, but only Lookup.

Review material

Questions and Opinions

JG01: RefactoringActionsFactory Javadoc example

InstanceContent ic = new InstanceContent();
ic.add(node);
Lookup l = new AbstractLookup(ic);

could be simplified to

Lookup l = Lookups.singleton(node);

JB: Yes. It can. Javadoc improved.


JG02: Javadoc of BackupFacility is badly broken.

JB: Ending
</pre> tag added.

JG03: Strange that BackupFacility is a singleton. Also strange that it uses long's as handles rather than some opaque Handle class.

JG04: TreeElementFactory.cleanUp should not be public if it is an "implementation detail".

JB: Will fix.

JG05: Not clear to me why ActionsImplementationProvider.*Impl methods return a Runnable rather than simply doing the body.

JB: Good question. There were some historical reasons. Probably should be changed the way you proposed.

JB: I'll fix it as Jesse says. So the return type will be void.

JG06: Various classes and methods are missing Javadoc comments. Everything should be carefully documented.

JB: It is just general complaint or do you mean anything special?

Y00: This is a huge submission. I am pretty sure that the listed usecases do not justify all the methods and classes in the API - for example TreeElement's purpose is not explained much, imho.

JB: Usecase added to arch.xml.

Y01: What is test coverage for the API, can you generate it and attach it? Can I see the tests that check the API behaviour, where they are?

JB: Tests are under usual directory: refactoring/api/test. Test coverage attached. I know, that unit tests are area for improvement. I work closely with QE on unit test development.

Y02: What getComposite is for?

JB: Returns element associated with this refactoring element. For instance Java Implementation returns internal representation of method from getComposite() of RefectoringElement representing reference inside of method body.

  • Y: Imho, the Java Implementation should store whatever it wants into the associated Lookup and then find it there. No need to create a public method which everyone sees just for private module communication.

JB: Forget to mention, that this information is used to build up a tree in refactoring preview panel. The method is not for private communication.

Y03: You should use <api group="property"/> for the properties that either this API itself, or its Java implementation understands. The reason is that 95% of users of the API is likely to work with Java and there is no special API for java (or is it?). That is why this API will serve as a documentation of all the additional properties and behaviors of Java. That is why I think this API shall more document the actual behavior of Java implementation.

JB: I'm not sure if 95% of users is likely to work with Java. This API is generic, because it should work with JSP, XML and other languages. Anyway I agree, that Java behavior should be somewhere specified. But not sure where.

  • Y: Ok, document java behaviour, would be a TCR. Imho, it makes sense to document it directly in this API.

Y04: Rewrite Context. It should not subclass AbstractLookup - that is an implementation detail. It should not have add method that is public. If you want it to be public, than justify that with usecases, but I am pretty sure that you want to give the right to modify context just to someone (probably refactoring plugins) but not everyone.

JB: If there will be TCR not to subclass AbstractLookup it is not problem to change it.

  • Y: Ok, I am proposing this as a TCR.

JB: As for add method I want the Context to be modifiable. Here is the usecase: Java Refactoring - User want to do Find Usages. Context of this refactoring in terms of Java implementation is ClasspathInfo. After preCheck state of refactoring user is allowed to set scope of refactoring. For instance default Project. So there must be a way, how to add values into Context instance.

  • Y: You said it - the preCheck needs to modify the content of the lookup, not anyone else. So you can pass some interface ModifyLookupContent into all registered plugin's preCheck methods (or somewhere near).

Y05: The API is full of Object..., Object[[ | ]], etc. It deserves a change - maybe for Lookup which is more typed than array of Objects. Whatever happens it shall be documented what are the possible values of these Object... parameters, what Java implementation accepts, etc.

JB: API full of objects is definitely not nice, but I'm don't understand which use case does introduction of Lookup solve. As for doc for Java impl. See my answer Y03.

SK0 : Need openInEditor() and showPreview() methods on the RefactoringElement class. The class implementing RefactoringCustomUI gets a collection of RefactoringElements, which are then used to get the corresponding TreeElements. The XML module code then uses the TreeElements to draw a preview Graph. The Graph is nothing but a different visual representation of the Jtree and we would like similiar (single click/dbl click) mouse behavior on the usage nodes as in Jtree.

JB: not a problem to add.

SK1: Need a way to filter out duplicate tree nodes (TreeElements) from the Preview Tree

JB: This is not problem in API. Just don't create duplicate nodes :)

Sandip:

In VW we have a couple of use cases that may not have been covered:

  • When one renames a Jsp file which is a JsfJspDataObject, we want to do the correct refactoring. However, along with this we want to perform additional refactorings to the backing bean Java file which is a JsfJavaDataObject. We want to delegate most of the Java refactoring to the refactoring provided by refactoring/java module. However we also want to also perform additional Java refactorings that may not be covered by the standard Java refactorings e.g. bean names inside string literals.

JB: Delegation is something which is supported: in prepare method of your plugin you can do this:

public Problem prepare(RefactoringElementsBag refactoringElements) {
  RefactoringSession currentSession = refactoringElements.getSession();
  RenameRefactoring delegate = new RenameRefactoring(...);
  ...
  ...
  delegate.prepare(currentSession);
  refactoringElements.add(...yourElements);
  ...
}

Renaming inside of String literals is not implemented by Java Refactoring modules, but I believe it is out of scope of this API review.

  • How the inplace renaming of nodes in explorer translates into refactoring actions/operations could also be handled by the refactoring framework in a standard way (i.e. based on canRename() in ActionProviderImpl). If not please at least provide guidance through a sample/javadoc or some other documentation.

JB: I can document it, but this issue belongs more to Nodes API than to Refactoring API.

  • Similarly the translation of drag & drop as well as cut/copy/paste into refactoring actions/operations could also be handled by the refactoring framework in a standard way. If not please at least provide guidance through a sample/javadoc or some other documentation.

JB: Drag and drop is handled by refactoring framework and it is completely transparent. If nodes are being D'n'Ded and Move action is enabled in this context - it is invoked.

TP01: Would be nice to add an example on delegation (i.e. how to use other refactorings from RefactoringPlugin). It is not that obvious that one can e.g. create refactorings even in the prepare method of the plugin. BTW is SPI use case #3 the one about delegation?

JB: Delegation example should be added. See example above.

TP02: RefactoringElementsBag - what is the difference between registerTransaction and registerFileChange?

JB: There need to 2 level of those changes. The first level need to commit changes to files, which are in the same state, they was when refactoring was started. And the second level of transaction does file changes (file moves, file renames etc.)

TP03: RefactoringElementImplementation.performChange - what it can really do? When one should register a Transaction instead?

JB: There are 2 levels of changes. "Transaction" level changes are designed for model-driven refactorings, while "performChange()" chages are designed for file level changes.

TP04: RefactoringElementBag.add - is the description about guarded block handlers correct? Does really every refactoring element need to be replaced by the guarded handler?

JB: I don't see such description in any doc...

TP05: RefactoringElementBag.addAll - is it really needed? If yes, shouldn't the collection be typed?

JB: Yes it should. TCR?

TP06: CustomRefactoringPanel is a bit strange, lacks documentation. Is it there just to let the client implement initialize? It is not clear if it provides some UI or how the client is allowed to modify it. Couldn't there be an interface with getComponent and initialize methods instead?

TP07: Do we need all those Problem, ProblemDetails, ProblemDetailsImplementation, ProblemDetailsFactory, ...? Couldn't it be a bit simpler? ;) Practical question: So how one should create a Problem correctly?

JB: Historical reasons. There was a Problem and there was a need for improvements. ProblemDetails, ProblemDetailsImplementation and ProblemDetailsFactory is backward-compatible solution of those requirements. There was API review for it: ProblemDetails API Review

TP08: Some classes/interfaces don't have clear use cases. E.g. who needs RefactoringCustomUI? Or what is RefactoringUIBypass good for (no doc, no usages)?

JB: RefactoringCustomUI was requested by Sonali Kochar (xml refactoring custom preview). RefactoringUIBypass is used for bypassing common operations (rename, move, etc..)

TP09: Minor: Javadoc of RefactoringSession says it is a singleton, but it is not.

JB: will fix.

Comments

I did not get a chance to or forgot to mention in time in yesterdays API review one more issue about Refactoring Undo and Redo. It is not clear top me as to how the clients are supposed to plug into the Undo and Redo Refactoring actions. Are there standard guidelines for this? This is important because how will several spi implementors coordinate how the Undo/Redo Refactoring actions are supported. Please point to me any location if it is already documented.

JB: There are several request for missing or unclear documentation. I created FAQ page. Feel free to add questions there.

Voting

Approved with TCRs

TCRs:
http://www.netbeans.org/issues/show_bug.cgi?id=95977
http://www.netbeans.org/issues/show_bug.cgi?id=95978
http://www.netbeans.org/issues/show_bug.cgi?id=95979
http://www.netbeans.org/issues/show_bug.cgi?id=95980
http://www.netbeans.org/issues/show_bug.cgi?id=96014
http://www.netbeans.org/issues/show_bug.cgi?id=96015
http://www.netbeans.org/issues/show_bug.cgi?id=96016
http://www.netbeans.org/issues/show_bug.cgi?id=96017
http://www.netbeans.org/issues/show_bug.cgi?id=96019

TCA:
http://www.netbeans.org/issues/show_bug.cgi?id=95981


Jan Becicka - 05 Feb 2007

Not logged in. Log in, Register

By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo