Community
Participate
Working Groups
Created attachment 145852 [details] Patch to introduce stop button Usually Cancel button in the wizard dialog is essentially the same as close button in the window taskbar. Until the long-running task is started from the wizard using getContainer::run - when the Cancel button becomes a "Cancel Task" button. This is rather confusing as the user might think that Cancel will not only stop the task, but will also close the dialog loosing all user input. I suggest we introduce a separate "stop" button and make Cancel button behavior consistent with other wizard buttons (i.e. it is grayed out when long-running task is started - as you can't close wizard dialog).
Created attachment 145853 [details] Screenshot of the Stop button
I like this
+1 I think this is a much much clearer design! Most people I tell have no idea that Cancel can do something other than close the wizard.
Thanks for the patch and redesign, Eugene! As others have said, this makes the wizard design much clearer to users, thank you for your contribution. I've applied the patch locally and played around with it on Linux/gtk+, comments as follows: 1. The createImage() API mandates that the returned Image instance be disposed by the client. We are causing a leak by not doing so. "Creates and returns a new SWT image for this image descriptor. Note that each call returns a new SWT image object. The returned image must be explicitly disposed using the image's dispose call." 2. The CURSOR_ARROW cursor is set to the cancel button in the aboutToStart(boolean) method. This is still done though it should be set on the stop button instead. At the moment I get a clock cursor when I'm hovering over the stop button. Being able to click on something with the clock cursor is counterintuitive in my opinion. The stopped(Object) method also needs to change. Less important stuff: 1. Not too gung-ho on the static ImageDescriptor idea though that could just be me. 2. There's probably some code we can can out as a result of this change like the 'cancelListener' field and the 'cancelPressed()' method. saveUIState()'s javadocs needs to be updated (since its 'keepCancelEnabled' parameter got removed). Admittedly, this might cause the patch to grow too big in size. *cough* ;) There may be others but that's all for now from me anyway. I personally think that it looks better as a ToolItem on a flat ToolBar instead of a Button but that won't fly because ProgressMonitorParts attach onto Controls. Oh well.
Created attachment 146232 [details] Patch with fixes for comment #4 Remy, I updated the patch. I tried to account for all you suggestions (except for the LessImportant#2 - I'll do that if the other fixes are good, I tried to make the patch smaller so it is easier to review). Notice - I changed the API a bit. I believe it is more logical (and reusable) if the button is part of ProgressMonitorPart as opposed to WizardDialog. So other clients can use the whole component including the button. I tried to make it API-compatible so clients will see old behavior is they use old API (I tested it by using non-patched WizardDialog).
Eugene, your attachment 146232 [details] has actually broken API by removing ProgressMonitorPart's protected initialize(Layout, int) method and replacing it with a new initialize(Layout, int, boolean) method. I'll let others weigh in on the suggested changes for ProgressMonitorPart.
Created attachment 146554 [details] Restored the initialize(org.eclipse.swt.widgets.Layout, int) method The method is back. I added a r/o boolean field that keeps if the button should be shown.
I can redo patch to move the button to wizard instead of ProgressMonitorPart - if that would make the change less "risky".
(In reply to comment #8) > I can redo patch to move the button to wizard instead of ProgressMonitorPart - > if that would make the change less "risky". I think the button in ProgressMonitorPart is fine. One other question: Why did you remove the "if (activeRunningOperations <= 0)" check from cancelPressed()?
> One other question: Why did you remove the "if (activeRunningOperations <= 0)" check from cancelPressed()? My understanding was that that method will grey out the cancel button when it was used to stop the progress runnable. The cancel button is no longer used for that purpose and is already grayed out when the long-running operation is in progress. Basically, the cancel button should already be grayed out if activeRunningOperations is greater then 0.
Created attachment 154510 [details] screen shot of the patch in action A good place to see this in action is in the "Install New Software" wizard. Make sure you have a bunch of sites defined and then select something and try to advance with the [x] Contact all sites checkbox checked...
Boris asked me to comment on this. First of all - Eugene, thanks for the patch. I really think this is a great direction and it wouldn't have gotten in without you implementing it. Nice job! Now for the nit picking... UI thoughts: - would be nice if the stop button better vertically aligned with the progress bar. See the previous attachment. I checked the progress view and it has a similar issue, though less noticeable. - I was worried that in losing the cancel handler we would lose the ability to cancel/stop the operation with the keyboard. However it looks like in both cases (before and after), pressing Esc during the operation launches a dialog saying the wizard cannot be closed while an operation is in progress (because the wizard's close handler is notified before the cancel handler was). This seems like a bug, but not one introduced by this improvement. API thoughts: - I'm not a big fan of adding a new constructor when a new option is added (having been down that road myself). Where does one stop? Having the setter is probably enough. - I'm not sure I understand the setStopCursor API. Wouldn't the cursor default to an arrow if we did nothing? If so, then I suggest we leave this API out, since it's not needed and since we want to promote consistency in the whole experience. If it is needed, then we could do it inside the progress monitor part - the new API needs javadoc and @since tags - agree that it makes sense for the stop button to live in the progress monitor part Again, nice job. I know this will make a big difference in the p2 install wizard when a software site hangs! (and certainly in other places...)
> - I'm not a big fan of adding a new constructor when a new option is added (having been down that road myself). Where does one stop? Having the setter is probably enough. Currently all the widgets are created in the initialize method that is called from the constructor. Introducing a setter method would require adding (and removing) a button to/from existing layout - this would complicate the code. I can implement it if it is required - but I'm not sure it is needed. > - I'm not sure I understand the setStopCursor API. Wouldn't the cursor default to an arrow if we did nothing? When button cursor is null then it will show the display cursor. Cursor is changed to hourglass (or another "wait" cursor) when wizard is running tasks - with only the cancel button having arrow cursor.
Created attachment 154844 [details] Patch that adds a new stop image to fix alignment (alignment problem - solution 1) The alignment problem in the progress view is caused by reusing the image that is in the org.eclipse.jface.action.images package. This patch adds gif with proper alignment so the layout is better now. But this will result in two similar images...
Created attachment 154845 [details] Button alignment "like in progress view" (alignment problem - solution 2) I updated the layout to move the button to the same "raw" of the grid layout. Button still seems to be not aligned well because of the image. This patch does not introduce any new images.
Created attachment 154846 [details] Higher progress bar (alignment problem - solution 3) I was experimenting - and found that one of the possible solutions would be to make the progress bar the same height as button. This changes the wizard looks a bit drawing more attention to the progress bar - but I don't know if this is ok.
Created attachment 154847 [details] This is how the wizard looks with "higher progress bar" patch
(In reply to comment #13) > > - I'm not a big fan of adding a new constructor when a new option is added > (having been down that road myself). Where does one stop? Having the setter > is probably enough. > > Currently all the widgets are created in the initialize method that is called > from the constructor. Introducing a setter method would require adding (and > removing) a button to/from existing layout - this would complicate the code. I > can implement it if it is required - but I'm not sure it is needed. Understood. I'll take a look at the latest patches in the next couple of days. Boris, I'll take this bug off your plate. It makes sense for me to look at it while I review bug 293098.
It is getting late for M5. I do want to see this go in, moving to M6
Created attachment 159349 [details] proposed patch Here is my proposed patch, which is based largely on the patch with a new image, but with some changes. I didn't like the idea of the taller progress bar, because we already create too much dead space in the wizard for progress bars when the bar is not used, and making it taller to fix the bug isn't so great. The biggest changes I made to your patch involve minimizing as much as possible the new API needed in ProgressIndicatorPart. If we want most dialogs to adopt this new way, we need to make it easy as possible for them to do so, and protect them from implementation detail. - removed the new enablement methods for the stop button and instead redefined the semantics of the attach/removeCancelComponent methods. When an internal stop button is used, these methods control the enablement rather than attaching to a component. This means that a null argument can now be provided to these methods. I'm not really sure if this is a better solution, but it seemed to me that the attach/remove methods need to change in their semantics anyway (would we want to hook listeners on an external control if there is also a stop button?) - removed the API for setting the stop button cursor. I have the part create the cursor and manage it, so that the arrow button is always available on the stop button. This seems better than forcing the clients to manage this. - made the new fields for the toolitem and stop button flag private rather than protected - adjusted the layout code so that the "numColumns" is incremented when needed inside the progress monitor part rather than making clients adjust their layout data to get the button. This way, the only new API needed is the new constructor that enables the behavior. Clients can use the new constructor, keep their attach/remove code, use existing layouts, and the "right thing" will happen. Ideally, I would have loved to have just added this behavior for free so that the new constructor is not needed. In most cases I think we would want dialogs that use cancel buttons to use this button instead. However this part is used in some places (splash handlers) where a cancel button is not appropriate. I also considered interpreting the "attach" method (not used by the splash handler) as a request for cancel behavior, but we really want to know up front whether to create the tool bar button.
Created attachment 159354 [details] screen shot of patch in install dialog This screen shot shows the alignment as used in the patch. I created a new stop image based on the one used in jface (but with the extra pixels around the actual image removed).
Boris - could you review the latest patch and comment on the proposed API changes? In particular the "redefinition" of the attach/removeCancelComponent API vs. introducing new API. Also on the challenge of getting "free improvements" vs. having to use this new constructor. Markus & Dani - could you also comment on the proposed approach? I would imagine we'd want JDT dialogs that use progress monitor parts to adopt this strategy. (RefactoringWizardDialog2, etc.)... I was hoping that the right thing would "just happen" but since we have no idea how other clients might be using progress monitor part, it seems safest to introduce a new constructor that requests the new behavior. Note that I've left one of Eugene's patches as "non-obsolete" because I will want to mark something for the IP log. After attaching this patch I realize that we might want to also include some hover text on the stop button such as "cancel current operation".
Created attachment 159375 [details] stop image, place in org.eclipse.jface.wizard.images
I don't like this change for several reasons: - "cancel" is a well known term in the Eclipse UI to stop jobs (like e.g. in the Progress view or dialogs, hence as soon as the user sees the progress bar it's obvious that 'Cancel' cancels the job. Seeing that button disabled seems odd. I see the problem that sometimes you get the wrong behavior i.e. you want to stop the job but it has just finished and then the wizard closes. This can be fixed by adding a small delay after the a job ends, during which the wizard won't be closed on 'Cancel'. - Users are used to the current behavior. Not being able to hit cancel any longer is unexpected. This can be especially an issue for blind users. The stop button is a nightmare for them. Other issues, though not fundamental: - when the the runnable is short running then I see no progress bar but a red button on the right side - that looks odd - the stop button (image) is not the one used elsewhere - the button misses a hover - the button is not prepared for accessibility
(In reply to comment #24) > I don't like this change for several reasons: > - "cancel" is a well known term in the Eclipse UI to stop jobs (like e.g. in > the > Progress view or dialogs, hence as soon as the user sees the progress bar > it's > obvious that 'Cancel' cancels the job. That was never obvious to me I have to admit. The first time I used it I was expecting the entire wizard to close and that I'd have to restart the wizard to change my choice. >Seeing that button disabled seems > odd. I agree with this point now that you mention it. >I see the problem that sometimes you get the wrong behavior i.e. you > want > to stop the job but it has just finished and then the wizard closes. This can > be fixed by adding a small delay after the a job ends, during which the > wizard > won't be closed on 'Cancel'. That's an interesting idea. I don't think that case was the driving case behind this bug, but I have been bitten by that time window and it does make the behavior more confusing. > > - Users are used to the current behavior. Not being able to hit cancel any > longer > is unexpected. This can be especially an issue for blind users. The stop > button > is a nightmare for them. What if the stop button were introduced but it was still also possible to push the cancel button? This would help with discoverability. I think there are users who wouldn't ever think to cancel the wizard to stop the operation, but might be likely to push the stop button. > > Other issues, though not fundamental: > - when the the runnable is short running then I see no progress bar but a > red button on the right side - that looks odd > - the stop button (image) is not the one used elsewhere > - the button misses a hover > - the button is not prepared for accessibility These can be dealt with if we go forward. (Though I believe we are using the same stop button image as elsewhere,..ie, debugger, etc.)
(In reply to comment #25) > (In reply to comment #24) > > I don't like this change for several reasons: > > - "cancel" is a well known term in the Eclipse UI to stop jobs (like e.g. in > > the > > Progress view or dialogs, hence as soon as the user sees the progress bar > > it's > > obvious that 'Cancel' cancels the job. > > That was never obvious to me I have to admit. The first time I used it I was > expecting the entire wizard to close and that I'd have to restart the wizard to > change my choice. Same here. I believe the use of the Cancel button in wizard dialogs for two purposes (depending on whether progress is shown or not) is very confusing. > >Seeing that button disabled seems > > odd. > > I agree with this point now that you mention it. I don't agree, I instead agree with comment 0 that the Cancel button should be consistent with other wizard buttons. You may argue that it would be consistent with general dialog behaviour if clicking the cancel button dismissed the dialog (including canceling the long-running operation) - but this would hurt users who may rely on the old (confusing) behaviour. So, I feel it would be safer to disable the cancel button as suggested by Eugene. > >I see the problem that sometimes you get the wrong behavior i.e. you > > want > > to stop the job but it has just finished and then the wizard closes. This can > > be fixed by adding a small delay after the a job ends, during which the > > wizard > > won't be closed on 'Cancel'. > > That's an interesting idea. I don't think that case was the driving case > behind this bug, but I have been bitten by that time window and it does make > the behavior more confusing. A delay sounds like a hack when the real fix would be to make the cancel button single-purpose. > > - Users are used to the current behavior. Not being able to hit cancel any > > longer > > is unexpected. This can be especially an issue for blind users. The stop > > button > > is a nightmare for them. Wouldn't (or shouldn't) the stop button be the focus control for the duration of the long-running operation, since everything else is disabled? With this in mind, would we really have an accessibility problem?
>Wouldn't (or shouldn't) the stop button be the focus control for the duration >of the long-running operation, since everything else is disabled? With this in >mind, would we really have an accessibility problem? No accessibility problem anymore but a more sever one because due to the timing issue - which is the *real* problem of the wizard - it will make the behavior even dangerous and not just an nuisance: in case all data is available the 'Finish' button must have the focus in the wizard as per the UI guidelines. Now imagine that a job is running and finishes just when you hit enter to cancel the job: instead your 'Enter' can go to 'Finish' and changes might happen that you don't want. This is more dangerous than the unwanted canceling and closing of the dialog we see now. And of course this is also the reason why 'Cancel' doesn't get the focus when a job is run. Another approach could be to morph the 'Cancel' button into the red square while a job is running. No matter what we do with the cosmetics here, I still think the timeout is what's needed to fix the real problem of the dialog, be it the current or a future polished version.
Just chiming in -- I think the direction this ticket was originally going towards: namely, disabling the cancel button and offering a red-stop for the operation in progress was the right thing to do. I wouldn't mind so much if the Cancel button remained, but I think the red-stop is a must-have improvement.
(In reply to comment #26) > I don't agree, I instead agree with comment 0 that the Cancel button should be > consistent with other wizard buttons. You may argue that it would be consistent > with general dialog behaviour if clicking the cancel button dismissed the > dialog (including canceling the long-running operation) - but this would hurt > users who may rely on the old (confusing) behaviour. So, I feel it would be > safer to disable the cancel button as suggested by Eugene. Actually, I was wondering if we should introduce the red stop button, and keep the current (yes, it's confusing) behavior whereby the cancel button also means stop. This is not where I'd start from scratch, but I do think Dani's points show that some users find the cancel button quite obvious. This also points out that we have to be clear about what hitting Esc and what closing the close box do during this window.
What do you think about showing the stop icon instead of the Cancel button while a job is performed?
(In reply to comment #30) > What do you think about showing the stop icon instead of the Cancel button > while a job is performed? I think that might look pretty weird to see an icon appear where the button used to be (or an icon added to the button, depending on what you were imagining). I think the value of the stop icon as proposed is that it is adjacent to the progress bar. That's what makes it meaningful. It's also familiar to users who are used to seeing it next to the progress bar in the progress view, jobs dialog, Firefox downloads, etc. I think it would lose its meaning moved to the button area. As for accessibility, if we can ensure that Esc still cancels the job, I think we would be okay.
>I think that might look pretty weird to see an icon appear where the button >used to be (or an icon added to the button, depending on what you were >imagining). I guess that's true and I agree that if we add the button it's best to place it as suggested. I think having 'Cancel' and 'Stop' enabled at the same time is even more confusing for people than what we have now. If we change this, we should come up with a clean solution that also removes the issue of unwanted closing (or finishing) the dialog. How about this: 1. when we start a job we a. disable 'Cancel' immediately b. we show the 'Stop' button after a small delay, like we show the busy cursor for a little while before we decide to show the dialog. This prevents UI flickering for short jobs. The 'Stop' button will get the focus, accessibility info and a hover. 2. 'Enter' or clicking 'Stop' stops the job. Clicking on [x] or pressing 'Esc' either behaves as before (shows the dialog) or does nothing. Stopping the job is not what we do at other places: the progress dialog for example, simply ignores clicking [x] and pressing 'Esc'. 3. when the job is done we remove/hide the progress UI wait for a small delay and then restore the UI state and focus control. The delay ensures that an 'Enter' key press doesn't cancel or finish the dialog.
(In reply to comment #32) > If we change this, we should come > up with a clean solution that also removes the issue of unwanted closing (or > finishing) the dialog. > > How about this: > 1. when we start a job we > a. disable 'Cancel' immediately > b. we show the 'Stop' button after a small delay, like we show the busy > cursor for a little while before we decide to show the dialog. This > prevents UI flickering for short jobs. The 'Stop' button will get the > focus, accessibility info and a hover. Do you agree that when the progress bar is showing the icon should show, when it's not it should not? Whatever delay is used in showing the progress is what we should use. Or are you suggesting a different/new delay? Also, we should restore focus to its original place when the controls re-enable (maybe we already do this). > 2. 'Enter' or clicking 'Stop' stops the job. Clicking on [x] or pressing 'Esc' > either behaves as before (shows the dialog) or does nothing. Stopping the > job > is not what we do at other places: the progress dialog for example, simply > ignores clicking [x] and pressing 'Esc'. Not sure what you mean by "shows the dialog", but I agree that if cancel is disabled, we ignore Esc and [x]. > 3. when the job is done we remove/hide the progress UI wait for a small delay > and then restore the UI state and focus control. The delay ensures that an > 'Enter' key press doesn't cancel or finish the dialog. This seems reasonable. I'll look at the code to see when we restore things now and plug this hole (not sure if we are doing things in the wrong order, need an async, or truly need a delay). You mentioned before that we had the wrong stop icon? Can you point me to the one you think is correct? I'm using the one in jface (with the asymmetrical margins removed), although it looks to me like the same one used in the progress views.
An API change Boris and I discussed long ago that is not currently in the patch: - we can "interpret" the existing attach/removeCancelComponent API as a request to enable/disable the stop button as done in the patch, BUT we should define new API so that new adopters don't have to use this. This also allows us to define our interpretation of attach/remove in terms of the new method, which could be called: setCancelEnabled(boolean enabled) I like this name better than the original setStopButtonEnabled(...) in the first patch because it keeps the terminology separate from the implementation.
Created attachment 160034 [details] Diff of stop images
>Do you agree that when the progress bar is showing the icon should show, when >it's not it should not? Whatever delay is used in showing the progress is what >we should use. Or are you suggesting a different/new delay? Yep, they should definitely appear as a unit. I didn't check the code whether we already have a delay for the progress bar (would probably be good to reduce UI noise), but if not we can also leave that delay out of this discussion (open separate bug/enhancement). >Not sure what you mean by "shows the dialog" Currently when you click [x] or press 'Esc' in a wizard while a job is running we show a dialog which tells you that the wizard currently can't be closed. >but I agree that if cancel is >disabled, we ignore Esc and [x]. I would also suggest that. The dialog on top of the wizard always looked strange to me and aligning the LAF with the progress dialog seems reasonable to me. >You mentioned before that we had the wrong stop icon? Can you point me to the >one you think is correct? I'm using the one in jface (with the asymmetrical >margins removed), although it looks to me like the same one used in the >progress views. Your image (attachment 159375 [details]) seems broken, most likely while you edited it manually, see attachment 160034 [details] for a visual diff of the original stop.gif (on the left) and yours (on the right).
Ok, I think we are in agreement and I'll prepare another patch that takes these comments into account. As Dani mentions, we can open bugs on further enhancements (delays, etc.). These can be addressed after API freeze. My priority now is to get the basic support and new API in place for M6. (Thanks for taking the time to make the comparison snap, that indeed must be some problem with my palette when I trimmed the margin off the original image.)
some clarification we had in a chat: daniel_megert@ch.ibm.com - Daniel Megert/Zurich/IBM: but with the cancel button it's clear: the user clicks and just in that time the job finishes and then dialog closes. 9:32:23 AM: daniel_megert@ch.ibm.com - Daniel Megert/Zurich/IBM: when the same happens with the stop button then it can cause to finish the dialog which can cause major trouble since there's mostly no undo 9:32:47 AM: daniel_megert@ch.ibm.com - Daniel Megert/Zurich/IBM: if the new solution introduces such a behavior then this would be a no go. 9:33:15 AM: susan_franklin@us.ibm.com - Susan Franklin McCourt/Beaverton/IBM: I agree it would be a no-go if a new window of trouble/accidental finish is introduced 9:33:38 AM: daniel_megert@ch.ibm.com - Daniel Megert/Zurich/IBM: k. how you ensure that (delay, sync,...) is not important to me 9:34:10 AM: susan_franklin@us.ibm.com - Susan Franklin McCourt/Beaverton/IBM: right. I hadn't realized what the underlying concern was - that in the old case, the worst case scenario was the wizard cancelling, and that now the worst case scenario is accidental finish 9:34:14 AM: susan_franklin@us.ibm.com - Susan Franklin McCourt/Beaverton/IBM: this is your concern, right? 9:34:23 AM: susan_franklin@us.ibm.com - Susan Franklin McCourt/Beaverton/IBM: which means we have to plug the hole or come up with some solution to prevent this 9:34:25 AM: daniel_megert@ch.ibm.com - Daniel Megert/Zurich/IBM: yes, exactly 9:34:27 AM: susan_franklin@us.ibm.com - Susan Franklin McCourt/Beaverton/IBM: got it
Created attachment 160560 [details] stop image, place in org.eclipse.jface.wizard.images corrected stop image
Created attachment 160561 [details] latest patch Here is the latest patch.
this is a work in progress patch, I have a few more changes, test cases, and some explanation to add before asking for more comments...
Created attachment 160569 [details] latest patch
Created attachment 160570 [details] once more...with feeling... Sorry for the attachment noise, I had a back-level patch file. This patch is the candidate for M6. Here's what we do: - the stop button appears when the progress bar does, disappears when the progress bar disappears. (the previous patches simply did not handle this correctly, so the button appeared too early and stayed around after). - when the operation starts, the UI is disabled, including the cancel button. The focus moves to the stop button. Pressing Space or Enter, or pushing the button, will cancel the operation. Per Dani's comment #32 and subsequent discussion, pressing "Esc" or [X] behaves as before. (I think some users might expect Esc to cancel the job because of the old behavior, but that would be inconsistent, so I think this is the best answer.) In the end, I did *not* remove the "can't close the wizard" dialog behavior because that is really a different issue and I wanted to make minimal changes to support this new feature. - when the user presses the stop button (or hits Enter or Space), the operation is canceled. Once it stops, the UI is restored after a small timeout. I somewhat arbitrarily chose 500ms as the timeout, but this can be tweaked after API freeze. The bottom line is that introducing a UI element that encourages the user to hit "enter" might cause mistakes, and there's no reasonable timeout that will guarantee they won't make this mistake. But I don't think it's a reason not to implement this feature. We'll just have to see. - as for the annoyance Dani mentioned in the old behavior - the user sometimes closing the dialog when they thought they were still cancelling the job. I noticed an oddity in the code whereby the counter that determined whether close became active was updated before the UI state was restored. So we really did have a small window where the UI controls were still disabled, but the close box and Esc could dismiss the dialog. I changed the code to "reinstate" the close behavior *after* the UI is updated, which is now also after the delay. - added a tooltip (the same one used in the progress view stop button) and fixed the icon. - new API introduced in this bug: public ProgressMonitorPart(Composite parent, Layout layout, int progressIndicatorHeight, boolean createStopButton) public void setCancelEnabled(boolean enabled) - redefined API public void attachToCancelComponent(Control cancelComponent) now includes this in the javadoc: * Since release 3.6, if an internal stop button has been created, this * parameter will be ignored and this method will be interpreted * as if the sender used {@link #setCancelEnabled(boolean)} with a * value of <code>true</code>. public void removeFromCancelComponent(Control cc) now includes this in the javadoc: * Since release 3.6, if an internal stop button has been created, this * parameter will be ignored and this method will be interpreted * as if the sender used {@link #setCancelEnabled(boolean)} with a * value of <code>false</code>.
Dani, can you please review. If you agree with the behavior as implemented, then I would like to release ASAP and then have someone in JDT try to incorporate it into something like RefactoringDialog. Having a client use the new API will help flesh out any remaining issues we might have with respect to API. We can tweak the timeout or other behavior later, but we need to get the API in and verified by a real client besides ourselves.
Hi Susan, overall LAF looks good! I found two issues: 1) The stop button has the focus but as soon as I hover over it, the focus is lost and 'Enter' doesn't work. 2) Sometimes when I e.g. use File > Import... > Plug-ins and Fragments > Next, I briefly see the red stop button on the right. I'm sure the timing where it appears is the same as for the progress bar but I guess the red is more visible and hence eye catching. Minor for now but if we see this more often we probably have to delay showing the progress bar a bit. API: I'm worried about setCancelEnabled(boolean) a bit: on one side it gives the impression that the client can control that state but on the other hand several ProgressMonitorPart methods call this and set it to 'false' or 'true' without checking the state that has been set by the client. In my opinion we should simply keep removeFrom/attachToCancelComponent to control the state of the cancel controls if 'fNeedsStopButton' is 'true'. That way the client needs to do less changes i.e. he can simply leave the code as is and use the new constructor. This worked very well when I tried to adopt the new feature in one of the refactoring wizard dialogs. setCancelEnabled(boolean) can be made private. The 'progressIndicatorHeight' parameter in the new constructor is overkill: so far no one passes something else than 'SWT.DEFAULT' (which is not even documented in the Javadoc as valid value in the existing constructors). ==> remove that parameter from the new constructor ==> in the existing constructor document that 'SWT.DEFAULT' can be used and what it exactly means Minor stuff: - setCancelEnabled(false) should probably not set the focus to the toolbar: only in case of 'true' it needs to set the focus - message.properties misses copyright date update - fNeedsStopButton: I would rename this to fHasStopButton - the icon needs testing on all platforms
W A I T: while running with the patch I found a major issue in the restoring of the UI state: 1. Package Explorer context menu > New > Java Project 2. enter name for the project 3. click 'Next' ==> the last page is shown but the 'Next' button is enabled but has no effect. This is not the case without your patch. I suspect this is caused by how you implement the timeout: it overwrites changes to the enabled sate of the buttons that happened in-between.
(In reply to comment #46) > W A I T: while running with the patch I found a major issue in the restoring of > the UI state: > > 1. Package Explorer context menu > New > Java Project > 2. enter name for the project > 3. click 'Next' > ==> the last page is shown but the 'Next' button is enabled but has no effect. I've just experienced this same problem in the p2 install wizard. Investigating. (In reply to comment #45) > Hi Susan, overall LAF looks good! I found two issues: > > 1) The stop button has the focus but as soon as I hover over it, the focus is > lost and 'Enter' doesn't work. Do you consider this a showstopper? I observe this same problem with toolbars in general. If you put focus on an item in the eclipse toolbar, then hover, the focus is gone. I believe it's a more general bug involving focus on tool items being lost when a tooltip shell comes up. Happens in the progress view with the stop button as well. I do not foresee this being fixed in the near term (at least not by me). Seems like we could try it with this limitation given: - in the accessibility case, the user is not hovering over the button anyway. We've placed the focus there for the person who can't manipulate the mouse to stop the button. - there's nothing else the user can do while this modal and disabled dialog is up (ie, it's not as if they will hover, go do work in Eclipse and then come back and expect the keybinding to work.) A fallback would be to remove the hover. > > 2) Sometimes when I e.g. use File > Import... > Plug-ins and Fragments > Next, > I briefly see the red stop button on the right. I'm sure the timing where it > appears is the same as for the progress bar but I guess the red is more visible > and hence eye catching. Minor for now but if we see this more often we probably > have to delay showing the progress bar a bit. Agree we could address this later if it's annoying to people. > API: > I'm worried about setCancelEnabled(boolean) a bit: on one side it gives the > impression that the client can control that state but on the other hand several > ProgressMonitorPart methods call this and set it to 'false' or 'true' without > checking the state that has been set by the client. In my opinion we should > simply keep removeFrom/attachToCancelComponent to control the state of the > cancel controls if 'fNeedsStopButton' is 'true'. That way the client needs to > do less changes i.e. he can simply leave the code as is and use the new > constructor. This worked very well when I tried to adopt the new feature in one > of the refactoring wizard dialogs. setCancelEnabled(boolean) can be made > private. This was my original proposal. Boris was worried about that API being weird for new adopters, and thought the new method could be used to help explain the behavior. (see comment #34). I don't feel strongly about this issue. > > The 'progressIndicatorHeight' parameter in the new constructor is overkill: so > far no one passes something else than 'SWT.DEFAULT' (which is not even > documented in the Javadoc as valid value in the existing constructors). > ==> remove that parameter from the new constructor > ==> in the existing constructor document that 'SWT.DEFAULT' can be used and > what it exactly means ack. > > Minor stuff: > - setCancelEnabled(false) should probably not set the focus to the toolbar: > only > in case of 'true' it needs to set the focus ack. > - message.properties misses copyright date update ack. > - fNeedsStopButton: I would rename this to fHasStopButton ack. > - the icon needs testing on all platforms This could happen during the M6 test pass since any problems found can be fixed post-API freeze.
This timeout is problematic. Delaying the UI state updating has the side effect of introducing new "wizard can't be closed" dialogs in the automatic test cases, since the code that tries to close the wizard gets invoked before the timeout. I'm sure it introduces a variety of other other problems not yet found, in additions to the ones we've found. I'm going to investigate a solution where the UI is restored immediately and the timeout only affects the "finish" behavior. This means the user could "accidentally" traverse to next/previous or any other default operation during this window, but could not accidentally finish the wizard.
Created attachment 160884 [details] latest patch This is the best I can do for M6. Out of time. Dani, what do you think? Changes: - progressIndicatorHeight parameter is gone and better documented in the existing constructor. - focus is only set on stop button when enabled - fixed up copyright - renamed stop button field - reimplemented the timeout to simply keep an additional counter of the open operations. The UI is restored immediately, but the new counter is decremented only after a timeout. This additional counter is only used by the finish code. The finish button is ignored during the timeout. This means subclasses implementing #finishPressed simply won't have that method called during the timeout, so for most normal cases, this should work. - since I was on the fence about the new setCancelEnabled API, I've opted not to include it. Tie goes to reducing bloat. Returned to the older version of these methods where they were documented in terms of their behavior instead of the new method. Now for the bad: - I did not address the "losing focus on hover" behavior as it is a much more global problem. A fallback position is remove the hover. I think the hover adds value and the case where a user can hover over a button but cannot click it isn't believable enough to get rid of the hover. - I see what Dani means about the red button attracting more attention than a progress bar. We'll just have to see how this play for users. - It *is* possible to accidentally trigger a "Next" or "Previous" in the wizard if you hit Enter just as the operation completes. We could consider checking the "timingOutOperations" flag on next and previous. I was on the fence about this one, and could change this before committing the code. - If a subclass of WizardDialog overrides the buttonPressed method and calls finishPressed() without checking the flag, they don't get the timeout behavior. However so much of this framework is exposed that I don't see how we could implement the timeout without a risk somewhere. For most "normal" overrides, the subclass implements finishPressed() rather than the buttonPressed method.
Another risk is that an existing test case based on WizardDialog could break because of the timeout. That is, any code path that ran an operation and then called finishPressed(), buttonPressed(), etc...expecting the wizard to finish, would fail because of the timeout. I tried the UI tests and there were no problems, but theoretically this could happen and it would be pretty confusing to someone who didn't know about this change why the test case would suddenly fail. If this is a showstopper I'd have to say that we are out of time for M6 and in the next release we could consider alternatives, such as not putting focus on the stop button (and thereby not needing a timeout), and keeping the cancel button alive to allow Esc to work for canceling the wizard. Of course, we couldn't prevent the user from putting focus on the button and pressing Enter, but at least we wouldn't be guiding them down this path.
>> 1) The stop button has the focus but as soon as I hover over it, the focus is >> lost and 'Enter' doesn't work. >Do you consider this a showstopper? No. >A fallback would be to remove the hover. Unfortunately that does not work: the tool tip itself is not causing the issue but the fact that moving the mouse over the ToolItem and exit again, disarms the ToolItem. I filed SWT bug 304672 to track this. As a workaround we could add a mouse track listener the resets the focus on exit: fToolBar.addMouseTrackListener(new MouseTrackAdapter() { public void mouseExit(MouseEvent e) { if (fStopButton.isEnabled()) { fProgressIndicator.setFocus(); fToolBar.setFocus(); } } }); >> - the icon needs testing on all platforms >This could happen during the M6 test pass since any problems found can be fixed >post-API freeze. Agreed. >- since I was on the fence about the new setCancelEnabled API, I've opted not >to include it. Tie goes to reducing bloat. Returned to the older version of >these methods where they were documented in terms of their behavior instead of >the new method. Right, additional methods make it harder to understand a class (more stuff more to understand, unless something gets deprecated along with it). Also, if we reuse the existing methods then it's easier to convert the existing code with less changes on the client side (the code changes to pass 'null' instead of the cancel control are not needed). I would reword the Javadoc: to the user it is not relevant that an internal stop button was created but rather that he used the new constructor with the flag set to true. How about: * @param cancelComponent the control whose selection will trigger a cancel. This parameter will * be ignored and hence can be <code>null</code> if a stop button was requested upon * construction and instead the stop button will be enabled and serve as the cancel * component. * @see #ProgressMonitorPart(Composite, Layout, boolean) Another question is whether we should point the clients to the new constructor i.e. add a hint in the old ones. The Delay: - I don't like timerExec (almost never ;-) as it often leads to strange side effects as we've already seen. - as you noted disabling the 'Finish' button is not enough: an arbitrary button can be the default button at this point - the delay affects all kinds of button clicks, e.g. also mouse clicks which don't need to be intercepted. ==> a better approach which can be installed without taking care of possible subclasses providing their own buttonPressed(int) is to use a traverse listener which only catches the 'Return' key and sets doit=false in case we're still in the timeout. Implementation Details: - I would not write if (fStopButton != null) { setCancelEnabled(false); but rather test for 'fHasStopButton' ('null' check is done in setCancelEnabled) - getShell() in aboutToStart(boolean) could be extracted to a local variable. I didn't do this in my patch to keep the changes small
Created attachment 160941 [details] Previous patch + implemented previous comment
Fixed in HEAD > 20100304. This should be in tomorrow's warmup build. I talked to Dani about his latest patch. We agreed on some additional changes: - ignore the space bar key in addition to enter during the timeout - some renaming and commenting to clarify what's going on - decided not to add the mouse exit handling on the toolbar because we can't be sure that it wouldn't introduce funny side effects on various platforms with the focus transferring back to the ToolItem. We'll ignore the "focus lost on hover" problem and can revisit later if needed. Tested on Win7. Happily, the scenarios that used to cause accidental advancement on "Next" are fixed (as well as the previous bug where Next was not enabled). This still needs to be tested on the warm-up build on Linux and Mac, and I will test again on Win7 in the actual build.
Comment on attachment 154844 [details] Patch that adds a new stop image to fix alignment this is not the final implementation, but represents the closest one. Marking for IP log. Thanks again, Eugene, for getting the ball (button?) rolling on this.
(In reply to comment #54) > (From update of attachment 154844 [details]) > this is not the final implementation, but represents the closest one. Marking > for IP log. Thanks again, Eugene, for getting the ball (button?) rolling on > this. To clarify: this comment was made while marking one of Eugene's patches for the IP Log. The code I committed *is* the final implementation (that is, until a bug is found).
We have a slight problem here, exposed by org.eclipse.jface.tests,wizards.WizardProgressMonitorTest.testProgressLabelsClearedBug271530() This test was failing due to this Assert made in the ProgressMonitorPart: public void removeFromCancelComponent(Control cancelComponent) { if (fHasStopButton) { setCancelEnabled(false); } else { Assert.isTrue(fCancelC omponent == cancelComponent && fCancelComponent != null); ... The problem here is that in this test case, the WizardDialog overrode createProgressMonitorPart() and did *not* use the new constructor. But the WizardDialog code is now passing null as the cancelComponent. So the assert is triggered. For today's purposes I changed the test case to use the new constructor. This verifies that the defect tested by this test case (clearing progress labels) really works in light of the new stop button. However the previous version of the test points out this other problem. I think the answer is that the cancelComponent still needs to be passed in by the wizard dialog, but in general I need to look at the code and see if there's a way to gracefully deal with the general problem. If someone has overridden createProgressMonitorPart() and is not using the stop button, we need to ensure that they aren't broken and ideally the old behavior should remain.
(In reply to comment #49) > However so much of this framework is exposed that I don't see how we could > implement the timeout without a risk somewhere. I'm getting to the point that I don't see how we can implement this entire *feature* without a risk somewhere. There is so much implementation made protected that it's really hard to imagine not breaking someone who has subclassed WizardDialog and reimplemented parts of it. So the question now is do we let this risk keep us from implementing the feature, or do we think we can minimize it? (In reply to comment #56) > If someone has overridden > createProgressMonitorPart() and is not using the stop button, we need to ensure > that they aren't broken and ideally the old behavior should remain. I really can't think of a good way to handle this gracefully that meets all of our criteria. The original idea was "get the behavior for free in WizardDialog" and opt in elsewhere. I think this can work in most cases. The problem case is someone who has overridden #createProgressMonitorPart(). By definition anyone who has overridden this method without calling the superclass implementation will not get the new stop button constructor. Yet the other parts of the code are assuming that the stop button is present, so we pass "null" to the cancelComponent methods, etc. Even if we didn't pass null, we now have code that disables the cancel button with the rest of the UI, etc., on the assumption that there is a stop button. The code I committed earlier today actually finds the problem in an Assert, but of course we can't throw runtime excpetions because we've added a feature. The only options that come to mind for solving this problem are: 1 - assume this is a very rare case that is not worth penalizing the other scenarios. Remove the assert and instead log a problem when the mismatched progress monitor is found. Allow the operation to run. The impact will be that the user will have no way to cancel the operation because there is no stop button and cancel is disabled. But the code runs and we've left a trace of evidence in the log as to why the user can't cancel. This is what I've implemented and committed for now, because it's better than what was in HEAD prior to this discovery. We are penalizing a (small?) subset of users in order to teach a broader group of users that one can cancel a wizard operation. . 2 - force the stop button on all wizard dialog users. There is no new constructor, and instead the stop button is simply provided for free. This means all subclasses now work as expected (we hope), but we know that we would break consumers of ProgressMonitorPart who do not use a WizardDialog, because their old cancel component decisions remain. We can only guess if this is more or less users affected than case 1. The symptom is that the cancel component likely remains enabled but is not operational, however the stop button would still work. That in itself is better than symptom of #1, but we take on the risk of not really knowing how people are using ProgressMonitorPart without WizardDialog and potentially cause unforeseen problems that are worse than #1. 3 - have conditional code in WizardDialog that checks whether there is a stop button and acts differently based on the result. The cancel button is kept alive and the old listeners used when the stop button is there. This is error-prone and introduces code bloat. It's also risky because the #run(...) method itself is public and technically could be overridden. It also isn't going to happen this release. 4 - punt the feature and back out today's commits because the implementation is too exposed and we can't do it safely. The other ideas I've considered (a new subclass with a stop button, a new progress monitor part creation method, etc.) suffer from some variants of the above. But it's possible my brain is simply too fatigued and that someone else out there has a better idea? From a usability point of view, the question is. Which user group is greater? - Users who never cancel because they never knew they could? (what this bug is addressing) - Users whose product implementor subclassed the progress monitor part creation who knew they could cancel and now cannot? I suspect the first group is a greater number, but of course it's pretty bad to simply remove a feature on the second group. My brain hurts.
To give my two cents on 287887...I'd opt for the User who never cancel because they never knew they code. I'd go for useability...make a note to the other users and platforms can adjust during M7. While I know we are ultra conservative on this. Cancel should only ever mean one thing, not change it's meaning depending on what is happening. It would be like Next and Back or Finish having different meanings. Cancel should always take you out of the wizard. It's just consistent, and honestly, I would expect it to take me out of the Wizard, not cancel the job running. The stop buttong is much more intuitive. We've changed UI before, so this is no different and to me provides a better user experience for a greater majority of users.
Susan, I don't like the latest code changes in ProgressMonitorPart because they change the contract (by only logging the NPE) only to support the WizardDialog problem we face. To fix the WizardDialog we have to: 0. revert latest ProgressMonitorPart changes 1. add a boolean flag that tells whether we created the monitor 2. set that flag in createProgressMonitorPart 3. rest of the code has to respect the difference between internal and custom progress part. Yes, it adds a bit of bloat but it reduces the risk that possible subclasses get broken. Of course there is still little room for breakage since a client might call our create method and throw the result away but I won't cry a tear about those ;-) I've made those changes in HEAD and released them (including test) into I20100305-0800. I've verified that the new and the old version of the test pass. HTH Susan, please review.
(In reply to comment #59) > Yes, it adds a bit of bloat but it reduces the risk that possible subclasses > get broken. Of course there is still little room for breakage since a client > might call our create method and throw the result away but I won't cry a tear > about those ;-) This is a much better solution. I woke up this morning thinking I was going to give this a try and see how bad it looked (I was putting out too many other fires yesterday). And then I saw:... > > I've made those changes in HEAD and released them (including test) into > I20100305-0800. I've verified that the new and the old version of the test > pass. > > HTH > > Susan, please review. Nice work. I tried to find a hole in it but it looks complete, and the bloat is really not that bad. (One couldn't really call this "pair programming," more like "time zone relay programming.") thanks.
verified on Win7, Build id: I20100309-0100. Tried a lot of repeated Enter hitting, which can cause Next/Cancel/Next/Cancel/Next/Cancel (repetitive restarting of the operation followed by cancel). Also tried cancelling very near to the end of the task. Looks good. Prakash, have you had a chance to try this on Mac?
(In reply to comment #61) > Prakash, have you had a chance to try this on Mac? Yes. Tried few combination, and works fine.
Please see https://bugs.eclipse.org/bugs/show_bug.cgi?id=311917#c7 for my point of view on still being able to have the Cancel button available in order to cancel out of the entire "Install New Software..." operation.
