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.

Bug 59179

Summary: Enhanced diff view API
Product: utilities Reporter: Martin Entlicher <mentlicher>
Component: DiffAssignee: 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
Due to UI improvements in cvs light module, it's necessary to enhance the diff
API. It's not sufficient to return just java.awt.Component from the diff
visualizers, but we need also extended info, like number of differences,
additional toolbar, etc. There is a special interface - DiffView created for
this purpose.
Comment 1 Martin Entlicher 2005-05-23 15:58:01 UTC
Created attachment 22263 [details]
The proposed extension of diff API
Comment 2 Martin Entlicher 2005-05-23 15:59:26 UTC
Please review this addition to diff APIs.
Comment 3 Jaroslav Tulach 2005-05-24 10:02:07 UTC
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? 
 
 
Comment 4 Martin Entlicher 2005-05-24 11:41:55 UTC
> 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* ?
Comment 5 Jaroslav Tulach 2005-05-24 15:04:03 UTC
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. 
Comment 6 Martin Entlicher 2005-05-26 15:12:15 UTC
Created attachment 22320 [details]
The slightly modified version according to Yarda comments.
Comment 7 Martin Entlicher 2005-05-26 15:27:07 UTC
I've renamed canSetCurrentDiff() to canSetCurrentDifference() and
UnsupportedOperationException in Diff.java and DiffVisualizer.java is replaced
with a dummy impl. of DiffView.
Comment 8 Martin Entlicher 2005-05-26 15:28:01 UTC
Created attachment 22322 [details]
The updated DiffView, that I forgot to include into the previous diff.
Comment 9 Martin Entlicher 2005-05-30 16:02:09 UTC
I'll merge the last attached changes into trunk in 24 hours...
Comment 10 Martin Entlicher 2005-05-31 17:34:47 UTC
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