Community
Participate
Working Groups
For huge files or when running the uncapped comparing algorithm, opening the Compare Editor may take a while. We would have to make sure the algorithm can be canceled and that it doesn't block the UI. Ideally, we would also like to provide progress monitor when opening the editor.
Created attachment 176337 [details] Fix v01 Fix does two thing: * passes the progress monitor down to the LCS algorithm -- this allows to stop the algorithm halfway and report progress at the same time, sweet! * support monitor cancellation. Currently, the InterruptedException is ignored[1], so when the monitor is cancelled an NPE is thrown when trying to iterate over e, since it's null. I guess this is what happened in bug 313017. [1] DocumentMerger.doDiff
Created attachment 176338 [details] mylyn/context/zip
Dani, does it make sense to you do open the compare editor when computing diffs has been cancelled? In Fix v01 I open the editor, but show no diffs in that case. But on second thought, maybe I should close the editor. I can also utilize the message added in bug 292831, inform that computing diff has been interrupted and allow to run the (uncapped?) algorithm again.
Created attachment 176444 [details] Hitting Cancel on "Intitializing Compare Editor" dialog Another thing I've just noticed is that the compare editor opens with "Initializing..." label (and stays with it) when Cancel is pressed during startup. This is easy to reproduce with a breakpoint set on entry to org.eclipse.team.ui.synchronize.SaveableCompareEditorInput.prepareInput(IProgressMonitor).
>Dani, does it make sense to you do open the compare editor when computing diffs >has been cancelled? In Fix v01 I open the editor, but show no diffs in that >case. But on second thought, maybe I should close the editor. I think currently when we start a compare (at least out of the Synchronize view), the editor opens and then first tries to fetch the source from the repository. I *think* if that fails we leave the editor open and show a dialog or even show the error inside the editor. So, I guess it would make sense to keep it open in that case. If the editor is not yet open and one cancels, then it should not open an empty editor.
Created attachment 176538 [details] Two dialogs with progress The "Initializing Compare Editor" dialog is for the job that opens the editor, the modal "Finding diffs" dialog is opened for the algorithm's runnable in DocumentMerger.
(In reply to comment #5) > I *think* if that fails we leave the editor open and show a dialog > or even show the error inside the editor. So, I guess it would make sense to > keep it open in that case. > > If the editor is not yet open and one cancels, then it should not open an empty > editor. Both of these make sense to me, but istr that no long time ago when no diffs were found a message was displayed and then the editor was closed. This is no longer true once we switched to SaveablesCompareEditorInput in bug 193324, now this seems like a regression to me (bug 275153). Leaving the editor open when no diffs have been found, and doing the same thing when finding diffs has been cancelled can lead to confusion. It's even worse when we compare two large, but identical files. We will get an info about "inaccurate diffs" when in fact there are none. Displaying the dialog and closing the editor as it happened before bug 193324 is a reasonable approach imo. Moreover, looking at the code of org.eclipse.compare.internal.CompareEditor.createCompareControl() I can see that even canceling the editor is supposed to close it. I wasn't able to hit that line running the current code though.
Created attachment 176547 [details] Closing the editor when cancelled during init
Tomasz, I didn't try your patch yet. Can you tell me where/how I would be able to cancel - especially in the case where I start many compare editors at the same time via Synchronize view?
Good point, I assume you have the "reuse compare editors" preference off and the "always run in background" on. In that case, pressing Cancel on the "Finding diffs" dialog will cancel the algorithm, but since it's still a modal dialog you won't be able to start a new comparison while it's on. When I artifically prolong initing the editor, I'm able to open several editors, before the first one starts finding the diffs. The problem is that as soon as the dialog is up, I'm not sure for which editor I'll cancel the algorithm when I hit the button.
>Good point, I assume you have the "reuse compare editors" preference off and >the "always run in background" on. Yes. I start many compares at the same time because our network connection is slow and it takes a while to fetch the content for each file. During that time I do other stuff but of course nothing should interrupt me here. > In that case, pressing Cancel on the "Finding diffs" dialog I never saw a "Finding diffs" dialog. Will that be introduced? If so, that's a no go. Opening the editors in the background must not be broken. For those editors you need to offer the Cancel button (red square) inside the editor. You could even report progress in there. Also note that the Progress view itself is not very useful in that regard since it does now show the file/editor name currently.
(In reply to comment #11) > I never saw a "Finding diffs" dialog. Will that be introduced? If so, that's a > no go. No, the fix does not add any new dialogs. The "Finding diffs" dialog should blink when you compare large files with many diffs, i.e. when the algorithm needs more time to run. "Fix v01" fixes that dialog and makes the Cancel button work. > Opening the editors in the background must not be broken. I haven't touched it. > For those editors you need to offer the Cancel button (red square) inside the editor. You > could even report progress in there. I like the idea, but do I actually need to add the Cancel button there? What if I improve how the Progress view presents the jobs. The diff algorithm can also be ran when the editor is already open (e.g. ignoring whitespaces) I guess I would need to add the button (and progress) then as well. > Also note that the Progress view itself is not very useful in that regard since > it does now show the file/editor name currently. True.
>No, the fix does not add any new dialogs. The "Finding diffs" dialog should >blink when you compare large files with many diffs, Oohkay, then I assume that the dialog only comes after the file has been downloaded and that the 'Cancel' fix is only for the algorithm and not for the downloading part, right? >I like the idea, but do I actually need to add the Cancel button there? What if >I improve how the Progress view presents the jobs. Perfectly fine for me.
(In reply to comment #13) > I assume that the dialog only comes after the file has been downloaded > and that the 'Cancel' fix is only for the algorithm and not for the downloading part, right? Right. Does it mean that fetching content, which may take longer time than finding diffs, cannot be interrupted as well? Frankly speaking, I haven't checked that.
>Does it mean that fetching content, which may take longer time than finding >diffs, cannot be interrupted as well? Frankly speaking, I haven't checked that. At least for the compare editor opened via Synchronize view, you can cancel it via Progress view (if you hit the right job ;-). The result is a bit lame though: the job is killed and the editor stays open with the text that it showed at that time i.e. "Initializing...". Just for fun, see bug 273 (yes three digit bug :-). Anyway, I wouldn't spend too much time on that. At least for me this was never an issue so far.
(In reply to comment #15) > At least for the compare editor opened via Synchronize view, you can cancel it > via Progress view (if you hit the right job ;-). The result is a bit lame > though: the job is killed and the editor stays open with the text that it showed > at that time i.e. "Initializing...". Yup, this is similar to what I noticed in comment 4 and tried to fix with patch from comment 8. Krzysztof, could you verify if the patch from comment 1 is any good? Especially in the case described by Dani in comment 9. Next, could you check how to improve Compare editor jobs presentation in the Progress view? I guess it's a matter of changing the job's name, if so we could fix it here. However, if this appeared to be non trivial, I would like you to open a separate bug for it and explain what the problem is.
(In reply to comment #7) > Leaving the editor open when no > diffs have been found, and doing the same thing when finding diffs has been > cancelled can lead to confusion. It's even worse when we compare two large, but > identical files. We will get an info about "inaccurate diffs" when in fact there > are none. Displaying the dialog and closing the editor as it happened before bug > 193324 is a reasonable approach imo. Fixing bug 318826 would solve the problem with opening editor that shows no diffs.
If fetching contents is cancelled, the diff with the empty remote file is displayed. It looks like a bug. Pressing the cancel during initializing the editor seemed to have no effect - the button remained pressed until the diff was calculated (fraction of second) and then the editor was opened. This may be caused by the busy cursor, which is displayed probably during the diff computation. I was not able to cause the 'Finding diffs' dialog to appear.
I have managed to observe the finding differences dialog when I tested cap preference. Cancelling the diff computation causes Eclipse to use faster algorithm, but it is not Cancel per se.
(In reply to comment #18) > Pressing the cancel during initializing the editor seemed to have no effect Have you applied attachment 176547 [details] first? > I was not able to cause the 'Finding diffs' dialog to appear. When working on the fix I put couple of Thread.sleeps to give myself a chance to see and hit Cancel on these dialogs (attachment 176538 [details]). I recommended the same thing to the other Krzysztof ;) (In reply to comment #19) > I have managed to observe the finding differences dialog when I tested cap > preference. There is no such preference yet, is there? But we're working on it, see bug 307280. > Cancelling the diff computation causes Eclipse to use faster > algorithm, but it is not Cancel per se. Yup, the Cancel is broken and this is what attachment 176337 [details] is trying to fix, but I know nothing about running the faster algo once the Cancel is hit.
I have tried to open multiple diff editors in the background from the sync view. Indeed there is only one modal dialog opened, and it looks like a bug.IMO this dialog should display one job per diff computation (similar dialog appears when Eclipse is being closed).
(In reply to comment #21) > I have tried to open multiple diff editors in the background from the sync > view. Indeed there is only one modal dialog opened, and it looks like a bug. Does it mean that there is one dialog for all scheduled/running diffs computations? What happens if you hit Cancel? Does it cancel all the computations? I guess it cancels all diffs that haven't been counted yet. > IMO this dialog should display one job per diff computation > (similar dialog appears when Eclipse is being closed). That would work, but I'd rather get rid of the model dialog so I could monitor progress of each computation in Progress view.
(In reply to comment #22) > > What happens if you hit Cancel? Does it cancel all the > computations? > I guess it cancels all diffs that haven't been counted yet. The cancel button appears as inactive, one diff job is cancelled, the other seems to hang forever (it has not end for an hour).
(In reply to comment #15) > At least for the compare editor opened via Synchronize view, you can cancel it > via Progress view (if you hit the right job ;-). The result is a bit lame > though: the job is killed and the editor stays open with the text that it showed > at that time i.e. "Initializing...". This is not related to diffs computation, so it shouldn't be part of the bug blocking bug 307280. I've opened separate bug for it: bug 324965.
(In reply to comment #13) > > What if I improve how the Progress view presents the jobs. > Perfectly fine for me. I submitted a fix for that on bug 324965. Hitting cancel on init is a different story than canceling diffs computation.
(In reply to comment #21) > I have tried to open multiple diff editors in the background from the sync view. > Indeed there is only one modal dialog opened, and it looks like a bug.IMO this > dialog should display one job per diff computation (similar dialog appears when > Eclipse is being closed). I haven't found a way to solve this without fixing bug 324993. I was hoping I could show the file/editor name on that dialog but the RangeComparatorLCS.findDifferences where the "Finding differences" task name is set has no clue about file/input it operates on. I could improve the runnable's name started in DocumenteMerger.doDiff to something like "Computing Differences for <CompareEditorInput's title>...", but that would stay only for a split second to be replaced with the task from RangeComparatorLCS.findDifferences. imo, Fix v01 is ok. It fixes the main problem. I'm only worried about the scenario from bug 324993, comment 2.
It's too late to release it in M2, so I will commit the patch during first days of M3.
(In reply to comment #7) > (In reply to comment #5) > > I *think* if that fails we leave the editor open and show a dialog > > or even show the error inside the editor. So, I guess it would make sense to > > keep it open in that case. > > > > If the editor is not yet open and one cancels, then it should not open an > empty editor. > > Both of these make sense to me, but istr that no long time ago when no diffs > were found a message was displayed and then the editor was closed. This is no > longer true once we switched to SaveablesCompareEditorInput in bug 193324, now > this seems like a regression to me (bug 275153). Leaving the editor open when no > diffs have been found, and doing the same thing when finding diffs has been > cancelled can lead to confusion. It's even worse when we compare two large, but > identical files. We will get an info about "inaccurate diffs" when in fact there > are none. Displaying the dialog and closing the editor as it happened before bug > 193324 is a reasonable approach imo. Filed bug 325748 where we can decide what to do in those cases. As for now I will go with the approach suggested by Dani and leave the editor open. Fix v01 applied to HEAD. Available in builds >=I20100921-0800.
Thank you for fixing the problem. Is builds >=I20100921-0800 a driver for 3.6.1 or 3.7? Wonder how I can pick up a test driver.
I20100921-0800 will come out tomorrow, it's an integration build towards 3.7.
verified
Sam, have you had a chance to verify the fix as well?
The build I can find on the eclipse site are I20100922-0800 and I20100921-1024 for 3.7 stream. Both are marked with a red cross for windows 3.2. Which one should I try?
(In reply to comment #33) > The build I can find on the eclipse site are I20100922-0800 and I20100921-1024 > for 3.7 stream. Both are marked with a red cross for windows 3.2. Which one > should I try? It does not mean which one you choose - both should contain given fix.
The red cross indicates that some tests have failed. The builds are still good from our point of view and like Krzysztof said they both contain the fix.
I verified the fix and it works fine. Thank you for the fix
This is not yet working well, see bug 326680, bug 326683, bug 326685 and bug 326686.
We are close to our shipment. We are fine with the current fix.
*** Bug 344511 has been marked as a duplicate of this bug. ***
