This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | Enhanced diff view API | ||
---|---|---|---|
Product: | utilities | Reporter: | Martin Entlicher <mentlicher> |
Component: | Diff | Assignee: | Martin Entlicher <mentlicher> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | arseniy |
Priority: | P1 | Keywords: | API_REVIEW_FAST |
Version: | 5.x | ||
Hardware: | PC | ||
OS: | All | ||
Issue Type: | TASK | Exception Reporter: | |
Attachments: |
The proposed extension of diff API
The slightly modified version according to Yarda comments. The updated DiffView, that I forgot to include into the previous diff. |
Description
Martin Entlicher
2005-05-23 15:28:57 UTC
Created attachment 22263 [details]
The proposed extension of diff API
Please review this addition to diff APIs. DiffView is interface, are you sure all its methods are correct? you will not be allowed to add new ones in future. The expected usage is not described in the documentation neither in tests. That leads me to speculation that the DiffView interface is missing method save to programmatically save changes made in the component. This speculation may or may not be correct, that will be visible from the set of usecases and tests as soon as they get written. I believe that there is too many UnsupportedOperationsExceptions that it make the API a bit unusuable. The UOE was not meant for purpose of checking capabilities of an implmentation of some interface. Imho createDiff shall always return a valid DiffView which is either overriden by the DiffProvider and (very likely) supports all operations, or it returns default operation that delegates the view method to createView and returns false on canSetCurrentDiff while supporting the rest of the operations. Once again this speculation may or may not be correct, depends on the usecases and tests that shall be written. There is a naming inconsistency "canSetCurrentDiff" vs. "setCurrentDifference". Better to use *Difference in all cases. We have code completion in the IDE. How could the diffCount change? Who is currently implementing the API. Is the impl covered with tests? How big is the coverage? > DiffView is interface, are you sure all its methods are correct? you will not > be allowed to add new ones in future. I know, I've tallked about this with Maros and we hope that it's sufficient. Having this as an interface is more comfortable, since the GUI component can implement it. > DiffView interface is missing method save to programmatically save changes > made in the component. This speculation may or may not be correct, that will > be visible from the set of usecases and tests as soon as they get written. We do not have the full UI spec yet. :-( But we believe that the save should be accomplished by the modified component itself. We could handle that via SaveCookie if it will be really necessary. > there is too many UnsupportedOperationsExceptions There are two in Diff.java and DiffVisualizer.java - you're right that these can be replaced with some dummy impl. of DiffView. I'll do this. Other two are in DiffView. These are O.K. IMHO, since one should check canSetCurrentDiff() first. > naming inconsistency "canSetCurrentDiff" vs. "setCurrentDifference" Agree. I'll rename canSetCurrentDiff() to canSetCurrentDifference(). > How could the diffCount change? When it's modified. We plan to allow the diff view to be editable. New differences can occur or existing can disappear. > Who is currently implementing the API. There's DiffViewImpl at diff/src/org/netbeans/modules/diff/builtin/visualizer on vcslite branch. It's a little out-of-date now. > Is the impl covered with tests? No AFAIK. Is is required to have tests for the *implementation* ? Interface - ok, I just want to prevent the situation that is currenetnly being discussed in debugger apis. That means you should be ready to add new things into the DiffView lifecycle. See for example instanceInfo() at http://openide.netbeans.org/tutorial/api-design.html I am not sure where the savecookie would be added? Someone would implement DiffView and SaveCookie at once? A bit ugly but ok. "Other two UOE are in DiffView. These are O.K. IMHO, since one should check canSetCurrentDiff() first." - yes, they are very likely ok. "Is is required to have tests for the *implementation* ?" - well, strictly speaking it is required to have TCK for your API and then verify that all implementations you have successfully pass thru the TCK tests. For more info see: http://openide.netbeans.org/tutorial/test-patterns.html But as you are going to have just two implementations now, you may not write TCK, but only tests for those implementations. It maybe simper and probably more useful. Still consider usage of layered test, so some of the tests are shared and test both of your impls. If you need further help, I can stop by. Created attachment 22320 [details]
The slightly modified version according to Yarda comments.
I've renamed canSetCurrentDiff() to canSetCurrentDifference() and UnsupportedOperationException in Diff.java and DiffVisualizer.java is replaced with a dummy impl. of DiffView. Created attachment 22322 [details]
The updated DiffView, that I forgot to include into the previous diff.
I'll merge the last attached changes into trunk in 24 hours... Comitted to trunk & tests written. Thanks for the review and hints. /cvs/diff/api/doc/changes/apichanges.xml,v <-- apichanges.xml new revision: 1.6; previous revision: 1.5 done Checking in diff/nbproject/project.properties; /cvs/diff/nbproject/project.properties,v <-- project.properties new revision: 1.4; previous revision: 1.3 done Checking in diff/src/org/netbeans/api/diff/Diff.java; /cvs/diff/src/org/netbeans/api/diff/Diff.java,v <-- Diff.java new revision: 1.2; previous revision: 1.1 done Checking in diff/src/org/netbeans/api/diff/DiffView.java; /cvs/diff/src/org/netbeans/api/diff/DiffView.java,v <-- DiffView.java new revision: 1.2; previous revision: 1.1 done Checking in diff/src/org/netbeans/modules/diff/builtin/DefaultDiff.java; /cvs/diff/src/org/netbeans/modules/diff/builtin/DefaultDiff.java,v <-- DefaultDiff.java new revision: 1.14; previous revision: 1.13 done Checking in diff/src/org/netbeans/modules/diff/builtin/visualizer/DiffViewImpl.form; /cvs/diff/src/org/netbeans/modules/diff/builtin/visualizer/DiffViewImpl.form,v <-- DiffViewImpl.form new revision: 1.2; previous revision: 1.1 done Checking in diff/src/org/netbeans/modules/diff/builtin/visualizer/DiffViewImpl.java; /cvs/diff/src/org/netbeans/modules/diff/builtin/visualizer/DiffViewImpl.java,v <-- DiffViewImpl.java new revision: 1.2; previous revision: 1.1 done Checking in diff/src/org/netbeans/spi/diff/DiffVisualizer.java; /cvs/diff/src/org/netbeans/spi/diff/DiffVisualizer.java,v <-- DiffVisualizer.java new revision: 1.3; previous revision: 1.2 done RCS file: /cvs/diff/test/.cvsignore,v done Checking in diff/test/.cvsignore; /cvs/diff/test/.cvsignore,v <-- .cvsignore initial revision: 1.1 done RCS file: /cvs/diff/test/build-unit.xml,v done Checking in diff/test/build-unit.xml; /cvs/diff/test/build-unit.xml,v <-- build-unit.xml initial revision: 1.1 done RCS file: /cvs/diff/test/build.xml,v done Checking in diff/test/build.xml; /cvs/diff/test/build.xml,v <-- build.xml initial revision: 1.1 done RCS file: /cvs/diff/test/cfg-unit.xml,v done Checking in diff/test/cfg-unit.xml; /cvs/diff/test/cfg-unit.xml,v <-- cfg-unit.xml initial revision: 1.1 done RCS file: /cvs/diff/test/unit/src/org/netbeans/modules/diff/DefaultDiffViewTest.java,v done Checking in diff/test/unit/src/org/netbeans/modules/diff/DefaultDiffViewTest.java; /cvs/diff/test/unit/src/org/netbeans/modules/diff/DefaultDiffViewTest.java,v <-- DefaultDiffViewTest.java initial revision: 1.1 done RCS file: /cvs/diff/test/unit/src/org/netbeans/modules/diff/DiffViewAbstract.java,v done Checking in diff/test/unit/src/org/netbeans/modules/diff/DiffViewAbstract.java; /cvs/diff/test/unit/src/org/netbeans/modules/diff/DiffViewAbstract.java,v <-- DiffViewAbstract.java initial revision: 1.1 done RCS file: /cvs/diff/test/unit/src/org/netbeans/modules/diff/FallbackDiffViewTest.java,v done Checking in diff/test/unit/src/org/netbeans/modules/diff/FallbackDiffViewTest.java; /cvs/diff/test/unit/src/org/netbeans/modules/diff/FallbackDiffViewTest.java,v <-- FallbackDiffViewTest.java initial revision: 1.1 |