Community
Participate
Working Groups
Once the bug 322329 is fixed we will be able to cancel diffs computation (this may take a while for large documents if capping is disabled bug 307280). When cancelled, the editor opens showing no diffs. This may be confusing, the user may not remember that he has cancelled the computation and there are no diffs between both sides. Since we don't show the dialog indicating that the two files are the same (bug 318826) this a real threat. imo, the best way out of this situation would be displaying the "There are no differences..." message in the editor as suggested in bug 183959.
*** Bug 326686 has been marked as a duplicate of this bug. ***
Raising severity to match bug 326686.
This should be addressed for 3.7.
Agreed, Krzysztof could you please take a look at it as soon as you're done with bug 315694. The piece of code that opens the Compare Editor without any changes (when canceled) has been added in bug 322329 so looking at the patch there should be a good place to start.
Tomasz, I am little confused when reading this bug. The bug title is about closing editor when interrupted but in comment 0 you suggest >>displaying the "There are no differences..." message in the editor<< Did you mean implementing both functionality or just fixing one of them?
Right, the solution suggested in summary is contrary to the one from comment 0, but this bug is about to decide which way to go. Please, keep in mind what Markus wrote on bug 183959 about showing the dialog for asynchronously opened Editor. I'm not sure if it's still a case though.
Created attachment 182254 [details] dirty patch I am attaching patch displaying message about cancelling comparing but keeping editor opened. The message is displayed in the same label (place) where we display message about capping used. The patch is only for testing purposes. Do not commit! Tomek, did you mean such patch in comment 0? I am waiting for suggestions and comments.
(In reply to comment #7) > Tomek, did you mean such patch in comment 0? Yup, that's what I meant. But frankly speaking, I'm not keen on this approach. I know that once the editor is open we could keep it like that and just add a message about the fact the diffs calculation has been cancelled. At the same time, if the editor is not yet open and the user hit cancel, we should not open an empty editor. (see Dani's comment on bug 322329, comment 5. On the other hand, here are reasons which in my opinion make closing the editor a better option: * if the editor is narrow, the message (in the form proposed in comment 7) may become invisible, so the user may be not aware that there is no diffs displayed because she has hit the cancel * even when visible, the message in the toolbar may be omitted by the user if she opened a lot of editors (eg. from the sync view) and now she's browsing through them. A poping up message once the cancelled editor gets focus would fix that. But this is something I haven't seen in Eclipse, so I guess it's not an option. * I think that if one cancels he wants to abort the comparison and is not interested in examining file states in the compare editor. I believe 9 out of 10 users will close the editor once they cancelled diffs finding. Closing the editor on cancel would save one click.
>* I think that if one cancels he wants to abort the comparison and is not >interested in examining file states in the compare editor. Correct: the editor should only say that it got canceled and not show partial comparison. I agree that in the non-background case where a dialog is in front, canceling should close the dialog and the editor. The tricky case is when I have 'Always run in background' enabled and then open 10 or more compare editors at once. It might be confusing if some of the editors just goes away. In that scenario I think it's better to keep the editor(s) open with a "canceled" message.
(In reply to comment #9) > I agree that in the non-background case where a dialog is in front, canceling > should close the dialog and the editor. The tricky case is when I have 'Always > run in background' enabled and then open 10 or more compare editors at once. It > might be confusing if some of the editors just goes away. In that scenario I > think it's better to keep the editor(s) open with a "canceled" message. Dani, as I understand you, you are proposing different behavior for 'Always run in background' checked and unchecked yes?
(In reply to comment #10) > (In reply to comment #9) > > I agree that in the non-background case where a dialog is in front, canceling > > should close the dialog and the editor. The tricky case is when I have 'Always > > run in background' enabled and then open 10 or more compare editors at once. It > > might be confusing if some of the editors just goes away. In that scenario I > > think it's better to keep the editor(s) open with a "canceled" message. > > Dani, as I understand you, you are proposing different behavior for 'Always run > in background' checked and unchecked yes? Yes, that would be best but as minimal solution we could implement the same solution for both cases: the editor stays open and tells the user that the operation got canceled (no partial info must be shown).
Created attachment 187112 [details] patch v2 Here is patch warking as mentioned in comment 11. The patch displays empty editor with message that comparing has been canceled. The patch requires externalizing strings yet. Dani, Tom could you review if such behavior when canceling comparison files?
The patch is not good. It shows the message in the compare editor but at the same time it shows diffs (see attached screenshot). Even worse: I can click on one of the diffs and it looks like it's computing diffs but it then clears the lower pane again and says that comparing has been canceled. > The patch requires externalizing strings yet. And use a constant for the new property. Also note that we use American English i.e. "canceled" and not "cancelled".
Created attachment 188055 [details] Picture showing wrong state of compare editor
Moving to 3.8
Krzysztof any chance for an updated patch?
Krzysztof's last patch pushed to Gerrit: https://git.eclipse.org/r/#/c/5241/ I'll try to review it and prepare a new version incorporating Dani's comments.
The suggested adjustment seems to be easy to apply. Gosia, redo the patch and ask for review again, please.
Is anyone from the team willing to give the review another chance, or should it be abandoned?
(In reply to comment #19) > Is anyone from the team willing to give the review another chance, or should > it be abandoned? What is your opinion about the change? All the comments you gave where about coding standard, how about substantive side of the change?
(In reply to comment #20) > What is your opinion about the change? I'm sorry but the patch is over a year old, and I totally don't remember if I had any other concerns about it.
(In reply to comment #21) > (In reply to comment #20) > > What is your opinion about the change? > > I'm sorry but the patch is over a year old, and I totally don't remember if > I had any other concerns about it. Since you "activated" the bug again in comment 10, how about you doing the review again and comment in the Gerrit change how it works with 'Always run in background' enabled and disabled?
(In reply to comment #22) > Since you "activated" the bug again in comment 10, It wasn't me in comment 10. > how about you doing the review again I'm afraid I won't have time for this in the near future.
(In reply to comment #23) > (In reply to comment #22) > > Since you "activated" the bug again in comment 10, > > It wasn't me in comment 10. Yeah a typo, I meant comment 19.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. If the bug is still relevant, please remove the stalebug whiteboard tag.
