Community
Participate
Working Groups
I20090429-0100. The progress dialog that allows to cancel history pruning on shutdown needs polish (see attched picture1): - should not say "Saving workspace." as to someone which doesn't read all the dialog text (i.e. text below progress) it could indicate that the whole save operation can be canceled - should not tell how to cancel (i.e. mouse click vs. button press) - should not say "safely" Suggestion: 1. replace first sentence with: Pruning local history. 2. replace second sentence with: Cancel to skip reducing history space.
Created attachment 133728 [details] picture1
*** Bug 274263 has been marked as a duplicate of this bug. ***
I released - should not tell how to cancel (i.e. mouse click vs. button press) - should not say "safely" Working on "Saving workspace".
Removing 3.5 target milestone. We are in the end-game now. Please have a look and decide if this should be targeted at 3.6.
Boris, the dialog is very prominent (every user sees it on each shutdown) and confusing. I still think it's worth looking at this for 3.5.
Kevin, could you have a look at this?
I scheduled it for 3.5 to do as much as possible to make information in the dialog less confusing. I doesn't mean that I want to change the dialog itself, rather details presented during saving the workspace. I'm not sure why Boris removed the target milestone.
I did a mass update for all bugs still targeted at 3.5 (instead of a specific milestone). Szymon, if you still plan to do more work on this, please target it to a specific milestone.
Szymon, can you clarify what work remains for RC1? I gather it's just "Working on "Saving workspace".
(In reply to comment #9) > Szymon, can you clarify what work remains for RC1? > I gather it's just "Working on "Saving workspace". Right. The dialog will stay untouched, only messages will be affected.
For what it's worth, I liked seeing the word "safely" in the message. It could be scary to an end user to cancel something during shutdown, because they may think it will cause shutdown or save to not complete normally. I suppose it shouldn't require saying that something is "safe" (everything should be safe!). But, to me the word "safely" conveyed, "there is no negative consequence of doing this", which seemed worth mentioning in this context Dani, was there a particular reason you wanted to remove that word?
>Dani, was there a particular reason you wanted to remove that word? Yes: if it is safe to cancel, why do we do it at all? Almost all such operations are done in the background during normal work. This is one of the few UI blocking things. In addition, it is not responsive i.e. after I click 'Cancel' nothing happens for a long time. I would expect that the pruning is topped immediately and the text in the dialog updated. I'm picky about this because it got introduced late in the game and every user sees it on each shutdown.
(In reply to comment #12) > In addition, it is not responsive i.e. after I click 'Cancel' nothing happens > for a long time. I would expect that the pruning is topped immediately ... Bug 275435. > and the text in the dialog updated. The dialog used while "saving workspace" is a bit limited, however this seems to be doable. > I'm picky about this because it got introduced late in the game and every user > sees it on each shutdown. That's why it is RC1.
(In reply to comment #12) > >Dani, was there a particular reason you wanted to remove that word? > Yes: if it is safe to cancel, why do we do it at all? Almost all such > operations are done in the background during normal work. This is one of the > few UI blocking things. I think what's really bothering me about this dialog is that the user is being asked to make an uniformed choice. I guess I'd assume its safe, otherwise I wouldn't be given the option, but what's the trade off when I cancel? Does it mean I take up more disk space? If so how much? Or is the next startup slower? If so by how much? Why am I being asked this question anyway? Q: Can't we trim as a background process on startup, instead of delaying shutdown? I guess that question is too late for 3.5 but I'd like to understand this better so that maybe we can at least phrase this better.
(In reply to comment #14) oops typo, that should've been "uninformed choice", not that they need to, err, get dressed up funny.
>Does it mean I take up more disk space? If so how much? Or is the next startup >slower? If so by how much? Why am I being asked this question anyway? It means potentially more disk space will be used. We won't know how much until we're done ;) It doesn't impact the next startup either way. The question is asked because some people with very large local histories complained about shutdown taking too long. It is difficult to optimize or do in the background, so this was seen as a simple thing we could do in 3.5 timeframe to improve the user experience.
(In reply to comment #16) > It means potentially more disk space will be used. We won't know how much until > we're done ;) It doesn't impact the next startup either way. The question is > asked because some people with very large local histories complained about > shutdown taking too long. So really the dialog should read: Do you care about: [Shutdown Speed] [Disk Space] :) In speaking with John, here's my suggestion: 1) Wording: The term "prune" is a problem since the user doesn't know what's being thrown out, thus can't make a decision. I think that's why "safe" was used but it doesn't inform the user about the decision consequences. What I didn't get is that this pruning is exactly based on the local history preferences for number and age of entries, and max file size. So I suggest we change wording to, ** "Compacting local history to reduce disk space." That sounds like something one can cancel reasonably. 2) When to show the message > The question is > asked because some people with very large local histories complained The ability to cancel is only needed for the 5% of the people who've changed their local history preferences. Yet 100% of the people are exposed to it. What's worse, those who haven't changed their preferences don't understand why now they're being asked to make a decision which functionally doesn't matter (for default values, the pruning is negligible, which is why we never asked before). Thus it'd be good to have some kind of heuristic whereby we'd know to only provide the choice (message + cancel) if it actually mattered. A simple one for 3.5 is: ** Only prompt if the local history preferences are different than the default. Better would be: if any of the three preferences are greater than the default. We could have more clever ones for 3.6.
See also bug #275716 about the wording in the preferences being incorrect, and about using the term "compaction" in both the progress dialog and the pref page.
In speaking with Szymon, here's the two levels of suggestions: Minimal: 1) Only show the message while actually compacting. Use wording I suggested below. In and above that, time permitting and if of low complexity (safe change): 2) Disable the Cancel button until we get to a cancellable subtask, which in this case is the compaction.
Created attachment 135599 [details] Proposal v01
With the fix we have 1) Cancel is enabled from the beginning till the end of compacting the history 2) "Cancel to skip compacting history" label is shown when compacting really starts 3) When compacting is finished, the label is cleared and the Cancel button is disabled
Created attachment 135624 [details] Proposal v02
+1 Released by me for 0513-2000 Proposal v02 looks good for tactical for 3.5. Currently Cancel is enabled immediately even though only the local history pruning is cancellable. We should only allow cancel during pruning but it requires people to wait through initial part of progress, so better 3.6 solution for presistent pruners is pref to never prune. We'd then change message to not say "Cancel" in it. Also note use of constants like: if (!isCanceled() && total == 4 /* right before history compacting */) are fragile. They won't break for 3.5 so ok as tactical for RC1 but we should fix later.
Sorry guys, but this seems not to work as intended by you. When I exit my large workspace (see attached picture2): - the message I see is: "Saving workspace." - while 'Cancel' is enabled - and the VM inside cleaning the history i.e. I don't see a history related message.
Created attachment 135765 [details] picture2
When I hit 'Cancel' it exits quickly. I guess the code that controls when "Cancel to skip compacting local history." is shown (and then removed again) has some problems.
Thanks Dani. I'll check that.
FYI: I tried several times now and sometimes I did get the message. re comment 21: Not having the message history related message from the beginning but having an enabled 'Cancel' button is confusing: can I really cancel to save the workspace?
(In reply to comment #28) > FYI: I tried several times now and sometimes I did get the message. > > re comment 21: > Not having the message history related message from the beginning but having an > enabled 'Cancel' button is confusing: can I really cancel to save the > workspace? I remember that I discussed it with Kevin yesterday. Kevin stated that Cancel should be considered as working with subtasks. However users see that there is a subtask only for compacting the history. Before that users don't know what is being canceled. I would agree with Dani that we should show "Cancel to skip compacting..." from the beginning. I will prepare a fix for that and wait for a Kevin's comment.
How I think it should work is: 1) Cancel only enables once the compacting subtask begins, and 2) Keep new bahviour of only showing the message during that subtask. This is how every progress bar works. The downside is that you have to wait a fraction of a second before you can get to the cancellable phase. Why I don't like having the "Cancel" message from the start is 1) There's no longer a correspondence between the actual subtask and the message 2) We invite everyone to cancel something which for most won't take any time at all (so why make them think about it?). 3) As with current behaviour, hitting Cancel early apparently does nothing immediately, which can be interpreted as a bug. I think the underlying problem is that what we're kind of trying to do is to simulate a pref that allows you to never have to prune history. Let's make this work like everyone would expect, and put the pref in for 3.6.
I agree Kevin. In comment 28 I was not asking to get the subtask/compact message from the beginning (except if there's no other way to solve this) but that we should not have the button enabled from start without the subtask/compact message. What's really important is: 1. that 'Cancel' is only enabled while I see the subtask/compact message 2. that 'Cancel' is never enabled if I don't see the subtask/compact message 3. that 'Cancel' gets disabled if click 4. bonus is if the subtask/compact message is shown only while compacting indeed happens
Fantastic Dani, we're in complete agreement. I think we misunderstood an earlier comment and went down the wrong path.
Created attachment 135847 [details] Additional fix
No +1 from Kevin. It may be too late for RC1. It seems this will be release in RC2.
+1 released for 1514-2000
Created attachment 135959 [details] Screenshot with I20090514-2000 (In reply to comment #24) > Sorry guys, but this seems not to work as intended by you. When I exit my large > workspace (see attached picture2): > - the message I see is: "Saving workspace." > - while 'Cancel' is enabled > - and the VM inside cleaning the history > > i.e. I don't see a history related message. In I20090514-2000, I see the same, but now 'Cancel' is *disabled* all the time.
Reopening, since the feature is completely lost again.
Same here: it's now back to square one :-( I can't say for sure that the message does not appear near the end but I wait half a minute seeing 'Cancel' being disabled and the stack trace showing it's clearing the history.
Argh, I will investigate today.
Kevin, my workspace is really large. Let me know if you need a crash test dummy.
Created attachment 136084 [details] Additional patch with pre/post compact messages This patch provides messages before the compaction (cancellable) phase and after. Note: I'd actually like to change the main progress message from "Saving Workspace." (which shouldn't have a period) to "Exiting Workbench" since that's what the command is in the File menu (Exit). But that would require a change to org.eclipse.core.resources (in /utils/messages.properties key resources_saving_0) and I think that's beyond the scope of what we want to change now. We should consider it as part of bug #276130 though.
Created attachment 136085 [details] Shows the new message sequence
(In reply to comment #40) > Kevin, my workspace is really large. Let me know if you need a crash test > dummy. Thanks Dani, if you could try the new patch and let us know what you think that'd be great. In order to properly see what was going on, I ended up putting a workspace on my iPod (which I figured had a slow'ish drive) and then ran a virus scan on it in parallel :)
I like the two additional detail messages. The question is whether we could now change the main message from "Saving workspace." to "Shutting down the workbench." or "Exiting the workbench.". Some additional info about my problem: - when I start my large workspace and then exit without doing any work it looks like it is working even in I20090511-2000. - when I work with it for a while (could not yet find pattern) then the button is disabled until the end. ==> there seems to be different code paths or timing issues that result in different progress monitor total count and hence in (not) triggering the clean history case (because total lower than 4). Now, the problem I have with the patch is that it doesn't change the timing/logic of enabling and disabling the cancel button and hence won't fix Markus's or my issue. According to Markus he can always reproduce and he's now debugging it.
I debugged through this and found the problem in the ProgressMonitorWrapper in IDEWorkbenchAdvisor.disconnectFromWorkspace(). You cannot assume that summing up doubles gives an exact value. In my case, 'total' was 4.000000000000001 which is not == 4. If there really is no better way to find out when to toggle the Cancel button and the messages, then you should at least replace if (... total == 4) by something like if (... Math.abs(total - 4.0) < 0.0001)
Good catch! Expecting a certain progress monitor total work number is brittle but I can't see another way at this point except adding new API that notifies before clean starts and after it ended. For now I suggest to go with with Math.abs(total - 4.0) < 0.0001 and file a bug to clean this up in 3.6. One way to get rid of this problem in 3.6 would be to no longer clean automatically and instead offer a 'Clean..' button on the preference page. Other tools also a similar approach, e.g. compacting a folder in Thunderbird has to be manually initiated by the user.
A better implementation for 3.5 would be: - Declare an interface ICancelEnablement in an internal Core/Resources package with method public void setCancelable(boolean cancelable, String subTaskMessage); - Let the ProgressMonitorWrapper in IDEWorkbenchAdvisor.disconnectFromWorkspace() implement ICancelEnablement (accept the API problem for 3.5) and call 'subTask(subTaskMessage); p.setCancelable(cancelable);', like in updateProgressDetails(). - In SaveManager.save(int, boolean, Project, IProgressMonitor) line: 1038, check whether one of the progress monitors in the getWrappedProgressMonitor() chain implements ICancelEnablement, and if it does, call setCancelable(..) on it. This hack should of course be removed in 3.6 when the root problem has been solved and manual canceling is not necessary any more.
(In reply to comment #41) > Note: I'd actually like to change the main progress message from "Saving > Workspace." (which shouldn't have a period) to "Exiting Workbench" since that's > what the command is in the File menu (Exit). But that would require a change > to org.eclipse.core.resources (in /utils/messages.properties key > resources_saving_0) resources_saving_0 is fine. We can do the full save not only at Eclipse shutdown. (In reply to comment #46) > For now I suggest to go with with > Math.abs(total - 4.0) < 0.0001 and file a bug to clean this up in 3.6. Agree. > One way to get rid of this problem in 3.6 would be to no longer clean > automatically and instead offer a 'Clean..' button on the preference page. > Other tools also a similar approach, e.g. compacting a folder in Thunderbird > has to be manually initiated by the user. Cleaning the history and saving the worksapce should be separate actions. This is what should be done in 3.6. Then users will be able to trigger clean anytime. I couldn't introduce this change in 3.5 since this would require some API changes. (In reply to comment #47) > This hack should of course be removed in 3.6 when the root problem has been > solved and manual canceling is not necessary any more. We should do the smallest change as possible to fix the issue. Since "abs" is smaller and introduces the hack only in one place (IDE), I would choose "abs".
>Cleaning the history and saving the worksapce should be separate actions. This >is what should be done in 3.6. Then users will be able to trigger clean >anytime. We can discuss this in a separate bug for 3.6, but I'm not sure about this. If cleanup has to be manually initiated by the user, very few people will think of doing this (or even realize that they can). The set of people who don't want any cleaning is very small (two so far), so adding an extra manual step for everyone else doesn't make sense to me. Without cleanup at some point, disk usage will continue to grow indefinitely. Both Dani and Markus have > 1GB history, which many users won't find to be an acceptable history size for a single application (a single workspace even).
>We can discuss this in a separate bug for 3.6, Yes of course, I never mean to look at this for 3.5. Szymon already expressed that he will continue to work on this for 3.6. >The set of people who don't want >any cleaning is very small (two so far) Just to be clear: it's not the cleaning I don't want but for sure I don't to want wait half a minute on each shutdown or cancel that dialog every time.
Nice debugging Markus. Szymon and I were also worried about comparing against work done constants but figured it was only thing for tactical for 3.5. Agree that *for 3.6* we need better way of knowing which subtask we are in, which message it should therefore get, and whether it's cancellable. > We should do the smallest change as possible to fix the issue. Since "abs" is > smaller and introduces the hack only in one place (IDE), I would choose "abs". > > For now I suggest to go with with > > Math.abs(total - 4.0) < 0.0001 and file a bug to clean this up in 3.6. Agree. For 3.5 let's fix the math. If we were earlier in the cycle I'd push more on changing the main task message "Saving Workspace" as in comment #41 but I tried to come up with new wordings for subtasks that weren't overly redundant. But Szymon expressed agreement on this change, and if John is +1 then I am too.
(In reply to comment #51) > If we were earlier in the cycle I'd push more on changing the main task message > "Saving Workspace" as in comment #41 but I tried to come up with new wordings > for subtasks that weren't overly redundant. But Szymon expressed agreement on > this change, and if John is +1 then I am too. I agree to have "Exiting Workbench" in the shutdown dialog, however we don't have to change anything in core.resources to achieve it. As mentioned in comment 48 "resources_saving_0 is fine. We can do the full save not only at Eclipse shutdown." (In reply to comment #49) > We can discuss this in a separate bug for 3.6, but I'm not sure about this... My intention is to allow those two users to run "Cleaning the history" anytime, when they disable it at shutdown. That's why I would like to separate cleaning the history from the full save. For regular users, it would work as it used to.
(In reply to comment #52) > I agree to have "Exiting Workbench" in the shutdown dialog, however we don't > have to change anything in core.resources to achieve it. As mentioned in > comment 48 "resources_saving_0 is fine. We can do the full save not only at > Eclipse shutdown." I'm confused, that message key is in core resources: org.eclipse.core.resources in /utils/messages.properties key resources_saving_0 I wasn't sure if it was used in other contexts (e.g. workspace save without exit) thus didn't suggest for 3.5.
(In reply to comment #53) > I'm confused, that message key is in core resources: > org.eclipse.core.resources in /utils/messages.properties key resources_saving_0 Seems there was some confusion between Szymon and I as to which version we were discussing: we both agree to NOT make that change in 3.5, in this bug. So it's just applying the two new subtask messages as attached, and fixing the math, for this bug in 3.5.
Created attachment 136484 [details] Kevin's patch with the math
Once the patch is accepted and released, I'll raise another bug to get rid of those hacks in 3.6.
(In reply to comment #55) > Created an attachment (id=136484) [details] > Kevin's patch with the math +1 for 3.5. Verified that Cancel becomes enabled and works in my dev workspace where it failed before. For 3.6, we already have bug 276130 and bug 274262.
(In reply to comment #55) > Created an attachment (id=136484) +1 looks good
Released to HEAD. Thanks all.
Fixed.
Verified in I20090520-2000.
