Community
Participate
Working Groups
I20061121-1845 I often get confused when using the compare views by the stale information in the structure comparison which only updates on safe. I'd like to have the option that the structure compare updates when the content changes, e.g. when I move a change from right to left or the other way around.
This is something I have been considering but I suspect it is not trivial given the current shape of things. How does it work for the Java Outline view. Is the complete tree regenerated when changes occur or is there a delta mechanism. Do you think it would it be possible to reuse the mechanism in the context of Compare?
There is a delta mechanism but most of the time we refresh the whole view. I think as a start to try out it would be good enough to 1. use StandardJavaElementContentProvider as content provider 2. register a delta listener to JavaCore (similar to JavaOutlinePage's ElementChangedListener) which refreshes the view.
Thanks for the pointer. Compare currently uses a different mechanism for the content but it may be possible to just use the JavaCore delta mechanism. Does the JavaCore mechanism fire deltas when there are changes in the file buffer?
>Does the JavaCore mechanism fire deltas when there are changes in the file >buffer? Only if there is a reconciler which triggeres ICompilationUnit.reconcile(...) like the Java editor does.
I would like to look into this during M5, if I can find the time.
I've given this some thought and to do this efficiently, we need some mechanism that traces the structure of the content being compared and can recognize when it has changed. Or, to put it another way, we do not want to blindly recalculate the structure every time the content changes. This is especially relevant given that the Java editor is already calculating the structure of a file. Ideally, the same mechanism would be used for both the Java editor and the Compare editor. When a structure change in one side of the comparison was detected, a recalculation of the structure viewer content could be triggered. Given that there is a delta mechanism involved, I am hesitant to proceed at defining a general mechanism to support auto-reconciling. However, since JDT defines it's own subclass of StructureDiffViewer, it may be possible to implement a JDT specific solution that requires minimal change (and possibly even no change) to Compare. Given this, I am deferring this feature. However, I am willing to help out on the Compare side if someone from JDT wanted to work on a JDT specific solution.
Created attachment 114308 [details] Example code This example code adds a Java element changed listener to the JavaStructureDiffViewer (needs to narrow down the delta but that's easy). This patch reveals two problems: - the caret/selection can be lost in the merge viewer because the merge viewer also updates once the structure has been updated. - the compare editor isn't fully made to work off working copies (dirty files). This can be seen just in the beginning when comparing a dirty file with HEAD: it asks to save first. I guess we could live with this for now. The patch just takes us closer to the final solution. Markus, what do you think of the patch?
Unfortunately, this only works if a Java editor is also open on the edited file. Otherwise, there's no reconciler, and it again only works on save, see comment 4. And you don't have to asyncExec the call to "contentChanged(null);". The update happens in a background job anyway.
Pawel did some work in this area, take a look at bug 261199.
Created attachment 130385 [details] Example code (updated)
(In reply to comment #8) > Unfortunately, this only works if a Java editor is also open on the edited file. > Otherwise, there's no reconciler, and it again only works on save, see comment > 4. Markus, please correct me if I'm wrong, but with the introduction of enhanced Java compare (bug 169386) the above statement is no longer valid, which is good news. Am I right? At least, this is the conclusion I came to after applying the updated version of Dani's code.
(In reply to comment #11) I guess you know that better than me ;-). And if you tried it and it worked without another Java editor, I guess that's all we need. > And you don't have to asyncExec the call to "contentChanged(null);". The update > happens in a background job anyway. This is still an issue in the updated patch.
I've been trying to figure out how to implement the org.eclipse.jdt.internal.ui.compare.JavaStructureDiffViewer.isAffectedBy(ElementChangedEvent) method added by Dani. I have IJavaElement or even IResource retrieved from the delta but I still don't know how should I match it to DiffNodes from the Diff Viewer. If I understood Dani's intention properly, this is required to update content of the viewer only in a response to related ElementChangedEvents.
>If I understood Dani's intention properly, this is required to update >content of the viewer only in a response to related ElementChangedEvents. Right, but as Markus said, this only works if a reconciler is installed that triggers the Java model which in return sends out the delta.
(In reply to comment #13) > I have IJavaElement or even IResource retrieved from the > delta but I still don't know how should I match it to DiffNodes from the Diff > Viewer. If I understood Dani's intention properly, this is required to update > content of the viewer only in a response to related ElementChangedEvents. To do find out whether the structural diff tree needs to be updated, you would have to implement the diffing a second time (since you never know whether you have to add another node). I think that would be overkill. Just check the delta and refresh the tree when anything has changed in the CU (but not when unrelated elements have changed). See e.g. JavaOutlinePage.ElementChangedListener for how to find out if the delta contains changes in the CU.
Created attachment 130707 [details] Patch v01 This patch is based on the example code by Dani + some tips from Markus made offline. I made two assumptions here: * decided that a change affects an element if the element from delta equals the one being tested * instead of just checking if an event affects the viewer I tried to figure out which part/structure of the comparison should be refreshed (changed boolean isAffected to ITypedElement findAffectedElement). I'm not sure if this if the later is required though. After little debugging I observed that even when a change to the left side only is made and the left structure is refreshed, an event affecting the right side is fired, which refreshed the right structure as well. Could we get away with passing null to contentChanged without guessing which structure to refresh?
Created attachment 130708 [details] mylyn/context/zip
(In reply to comment #16) > * decided that a change affects an element if the element from delta equals the > one being tested This works the same way as would returning always true from JavaOutlinePage.ElementChangedListener.isPossibleStructuralChange(IJavaElementDelta) do, if copied to the JavaStructureDiffViewer.
I think I mixed to issues here. Reconciling structure (comment 0) is one thing and outdated node's ranges (bug 260865, comment 5) is a slightly different issue. Fix for this bug *should* contain the isPossibleStructuralChange method as the issue here is about structure only. And it's the fix to bug 260865 that should contain any changes to conditions under which structures (including nodes) have to be refreshed (like in comment 18).
Created attachment 130820 [details] Patch v02 See comment 19.
The proposed fix requires changes in JDT/UI not Platform/Compare.
Dani, do you have time to take a look at the updated patch? I've heard you're the editor/reconcile master so I should ping you ;)
Looks not too bad. Three things: 1) it looks like you only refresh one side (the first you find). Shouldn't you refresh all matches, e.g. in case where you have changes to the left and the right side in the delta? 2) it has too many find* methods which aren't obvious to distinguish. I would e.g. rename findElement to findElementDelta. 3) calling JavaStructureDiffViewer.findElement(IJavaElement, IJavaElementDelta) for read-only elements (e.g. if input comes from history) could be avoided.
(In reply to comment #23) > 3) calling JavaStructureDiffViewer.findElement(IJavaElement, IJavaElementDelta) > for read-only elements (e.g. if input comes from history) could be avoided. True, before calling findElement(IJavaElement, IJavaElementDelta) I could check if the Java element is null and if it's read only. However, findJavaElement(ITypedElement element) returns null for elements that are not IResourceProviders so I guess this covers revisions. Passing null to findElement(IJavaElement, IJavaElementDelta) will result in returning immediately from the method. Nevertheless I could still check if a Java element is read only and if yes, skip calling findElement(IJavaElement, IJavaElementDelta). Would that be ok with you Dani? btw, I totally agree with two other things you mentioned.
>Would that be ok with you Dani? Yep. I just want to avoid querying the delta if not needed.
Created attachment 131906 [details] Patch v03 Patch updated according to comment 23 by Dani.
Created attachment 132233 [details] Improved fix The patch is good except for the isReadOnly(...) check. Tomasz, here's an updated version of it. Please do a final review.
Looks good and works fine. You're the author of the code anyway ;). Thanks for the update.
Committed to HEAD. Available in builds > N20090416-2000.
Thanks Dani.
Verified in I20090429-0100.
