Community
Participate
Working Groups
Pressing Command-Q on Mac OSX is expected to bring "Confirm Exit" dialog.
We need the fix from Bug 283345 here. PW
(In reply to comment #1) > We need the fix from Bug 283345 here. It turns out that the code is in 4.x stream, but there was something else. Suspense... Drumrolls... There was a line in CocoaUIEnhancer that redirects OS quit menu to use Eclipse quit command. Which seems to be logical on the surface except that QuitHandler calls Workbench.close() which does not prompt. The code that does prompt is added to the Display's SWT.Close listener. However, because OS processing is redirected to the Eclipse command, the SWT.Close never gets generated (IDEWorkbenchWindowAdvisor#promptOnExit()). Git reference: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2213493c9d2dfe6d0ae03b77e9d5c8c81a5eb1dd
But as a result, E4 apps can't install their own quit handler. Such as, say, to prompt the user as to confirm whether they want to quit! Isn't the right answer here to change the IDE exit handlers to prompt before closing?
(In reply to comment #3) > But as a result, E4 apps can't install their own quit handler. Such as, say, > to prompt the user as to confirm whether they want to quit! > > Isn't the right answer here to change the IDE exit handlers to prompt before > closing? Hmm.... I think that is what we are doing now - the "prompt" code is in IDEWorkbenchWindowAdvisor. Besides, if it is the general RCP behaviour we are talking about, the *Cocoa*UIEnhancer can't be the place. I think it is a good idea to have a workbench "close" and/or "shutdown" handler for the RCP apps and it should be a part of our lifecycle story. I don't think we have that now so I opened bug 377911 to consider for 4.3.
I was unclear in what I wrote: IMHO the right answer is to change the org.eclipse.ui.file.exit handler (org.eclipse.ui.internal.handlers.QuitHandler) to prompt before closing. It doesn't do that right now. To verify, invoke File > Exit on Eclipse (on a non-mac platform) or through the Quick Access bar. This invokes the org.eclipse.ui.file.exit command. Notice that it doesn't prompt for confirmation. Fixing QuitHandler to prompt is the right fix for this bug, IMHO. So I'm re-opening this again: regardless of my disagreement with this change, the change is incomplete as the CocoaUIHandler also hides all menu items that trigger org.eclipse.ui.file.exit (since they're remapped to the system menu). If we're not remapping org.eclipse.ui.file.exit then those items shouldn't be remapped (see redirectHandledMenuItem()). You'll see the Contacts demo has an Exit item that triggers org.eclipse.ui.file.exit, that should no longer be remapped.
Created attachment 214739 [details] Alternative approach Here's an alternative approach that installs an org.eclipse.ui.ide.application-specific QuitHandler to prompt the user, using the same mechanism as the close listener.
Sorry, I still don't get it. As far as I can tell, the current I-build (0430) for 4.2 works the same as 3.8 on Mac and Windows while using Command+Q, "x" button, and Eclipse -> Quit Eclipse / File -> Quit menu. The "Alternative approach" patch proposes Mac-specific code changes to achieve the same. 1) If we are speaking about RCP apps in general, why code changes are Mac-specific? The patch diverts quit command to a special handler that does the same work as the normal handler. I suppose the point is that then Mac RCP app can override the handler; but what about other OSes? And if there is really a need to override it, the RCP app coudl use the same code to track quit event. 2) Why do we need two handlers if Mac handler does not add anything Mac-specific? Brian, could you provide an example of something that you could do before this change and can't do now?
(In reply to comment #5) > I was unclear in what I wrote: IMHO the right answer is to change the > org.eclipse.ui.file.exit handler (org.eclipse.ui.internal.handlers.QuitHandler) > to prompt before closing. It doesn't do that right now. I disagree that this is the right answer. See e.g. bug 58762 comment 1: "The dialog is only there to prevent you from accidentally exiting when you close the last window, not when you explicitly choose File > Exit." Quit on the Mac is different from File > Exit, since it has a key binding and is also used via other OS gestures (e.g. Command+Tab, keep Command pressed, type Q to quit the selected app), so it also needs the "accident protection". The prompt-less File > Exit should stay, but I see that the org.eclipse.ui.file.exit handler is problematic, since it can be invoked from Quick Access and it can be bound to a shortcut. Proposal: - Remove the categoryId from the org.eclipse.ui.file.exit command, so that it doesn't show up on the Keys page and in Quick Access. File > Exit continues to use that command. - Add a new "Quit" command that just calls Display.close() and lets the existing infrastructure handle the showing of the prompt. This one can get a category, since it is safe to use and bind on all platforms. I'm just commenting on the UI here -- I don't know enough about implementation details for E4 applications, so maybe there's more to this on that level.
As the proposed patch does not seem to hurt anything and hopefully will help Brian, I released it into the master.
(In reply to comment #9) > As the proposed patch does not seem to hurt It does hurt: Now, one also gets a dialog with File > Exit on Windows. This is unexpected and not good. Clicking the [x] can easily happen by mistake, hence a warning dialog. File > Exit does rarely happen by accident, no dialog there. That is the reason for having the separate behavior that we had for many years (since 2.1). Also note, that the preference on General > Startup and Shutdown says: Confirm exit when closing last window ************************ If we decide to change it in M7 or even RC1 now, then we must also add a second preference so that the original behavior can be preserved. I'd prefer not to go down that path and leave things as is.
In addition, something is very fishy with the fix (4.2-I20120503-1800, Windows 7): sometimes File > Exit shows the dialog and sometimes it doesn't using these steps. 1. start new workspace 2. File > Exit ==> no dialog! (sometimes) 1. start new workspace 2. click 'Link with Editor' in the Package Explorer 3. File > Exit ==> dialog (sometimes)
(In reply to comment #11) That's probably because the new handler illegally targets commandId "org.eclipse.ui.file.exit" without an activation expression. The declaration of the command in org.eclipse.ui specifies a defaultHandler, and the spec clearly says "This handler will conflict with other handler definitions that specify no activeWhen conditions". See bug 378457 for the missing conflict detection.
(In reply to comment #10) > (In reply to comment #9) > > As the proposed patch does not seem to hurt > > It does hurt: Now, one also gets a dialog with File > Exit on Windows. Hi Dani, do we back out the fix and ask for a respin, or do we live with this until RC1? PW
(In reply to comment #13) > Hi Dani, do we back out the fix and ask for a respin, or do we live with this > until RC1? OK, my take on this is that we fix it in RC1. If there's dissent, please comment on the bug. PW
I was surprised that Oleg committed my patch as I thought after yesterday's call that we were going to thrash this out on the bug and fix for RC1. My concern is this: the original fix for this bug, removing the binding of the Quit menu to invoke the org.eclipse.ui.file.exit command, treats the immediate symptom but not the underlying problem. Having multiple exit points complicates life for RCP developers (E4 and, from bug 283345, for E3 too). I lost several hours in debugging to figure out why Cmd-Q wasn't triggering org.eclipse.ui.file.exit to invoke my clean shutdown handler. And although I've been developing Eclipse/SWT apps for years, I only recently became aware of the SWT.Close from this bug. From an app developer's perspective, I don't see a Mac app's App Menu > Quit as being different File > Exit, other than the Mac has a default keybinding. Note that WorkbenchActionBuilder omits File > Exit if on the Mac, implying it's equivalent to Cmd-Q. Nor do I see a real difference between an SWT.Close event (sent if Cmd-Q isn't bound) and File > Exit. Rather than have SWT.Close issue a prompt and org.eclipse.ui.file.exit not issue prompt, my take is that we should be triggering org.eclipse.ui.file.exit in all circumstances and instead provide a parameter to describe the triggering context to allow the handler to make the best decision. This parameter would be: keybinding, menu item, tool item, or app close request. I think that's a much easier-to-understand story. Another alternative is to instead define two exit commands to encode different semantics. I'm not a big fan of this approach though. (In reply to comment #10) > Also note, that the preference on General > Startup and Shutdown says: > Confirm exit when closing last window ************************ So if we're going to debate semantics (and we should!), then this argues against removing the original behaviour in the first place as the SWT.Close guard triggers regardless of the number of windows being closed :-) > If we decide to change it in M7 or even RC1 now, then we must also add a second > preference so that the original behavior can be preserved. I'd prefer not to go > down that path and leave things as is. I guess I'd argue that this option is misnamed and should be "Confirm exit": it's just that exiting the last window causes the app to exit. Supported in part by the fact that the method on IDEWorkbenchWindowAdvisor to check and issues the prompt is called promptOnExit().
(In reply to comment #14) > (In reply to comment #13) > > Hi Dani, do we back out the fix and ask for a respin, or do we live with this > > until RC1? > > OK, my take on this is that we fix it in RC1. If there's dissent, please > comment on the bug. > > PW Yes, completely agree with RC1 (I already mentioned this in the "declare M7" bug).
> From an app developer's perspective, I don't see a Mac app's App Menu > Quit as > being different File > Exit, other than the Mac has a default keybinding. Note > that WorkbenchActionBuilder omits File > Exit if on the Mac, implying it's > equivalent to Cmd-Q. Nor do I see a real difference between an SWT.Close event > (sent if Cmd-Q isn't bound) and File > Exit. I agree that they look similar from an app developer's perspective, but there's quite a difference in common usage. The Mac's Quit command is a built-in OS command and easily accessible, so it needs the prompt. Basically, it's the usual Mac way to close all windows. In contrast, Windows doesn't have such a pervasive Quit command/gesture. There, the usual way to quit an application is to close all windows (and we show the prompt when you close the last one). The reason for not prompting on File > Exit on Windows is that you wouldn't hit that menu item by accident, but usually in cases where you're already sure you want to exit (without closing all windows first). That's all the history and UX input I have. If we were in M1, I would say "let's regularize this and also prompt on File > Exit". But in RC0/1, I'm not so sure... A point to consider when changing the "org.eclipse.ui.file.exit" handler is that this command never showed a dialog. It could e.g. be used by scripts. That problem would be mitigated with the proposal from comment 8 (add a new command that prompts) or by adding a parameter (promptOnExit, false by default).
I can't say much about the behavior on Mac, but on Windows one thing is given: 1. File > Exit must not prompt if there's no dirty part 2. Closing last window must prompt with an option to never ask again.
I think we need to rollback the "Alternative approach" patch in RC1, but I don't think we should do anything else here for 4.2. This will get us in a state where the actual issue reported (see comment 0) is fixed and, as far as I can tell, we'll match 3.8 behavior. I do agree that Eclipse exit handling could be simplified, but this does not strike me as something that needs to be done in a Release Candidate phase. For the impact on e4 RCP apps, it is easily mitigated. Using contacts demo as an example, all that is needed is to add prompt to the ExitHandler - if RCP author thinks that prompt is necessary. In both 3.x and 4.x worlds prompting code resides at IDE levels. I don't see moving that code to the JFace as a good task for the RCx. So, I suggest that we roll back the last patch, close this bug as the original defect will be fixed, open an enhancement request for streamlining Eclipse exit handling.
Just so I'm clear here, I'm not disputing that Cmd-Q shouldn't prompt. All I'm disputing is that manner in which this is implemented because... (In reply to comment #19 from Oleg) > For the impact on e4 RCP apps, it is easily mitigated. Using contacts demo as > an example, all that is needed is to add prompt to the ExitHandler - if RCP > author thinks that prompt is necessary. That *was* true. It won't be true if we revert to your original patch, unhooking Cmd-Q. They'll need to either add an SWT.Close handler to the Display or add a listener to Display.getSystemMenu()'s SWT.ID_QUIT, either by providing an IApplication class that wraps E4Application or through a model processor/addon. And then add a visibleWhen to their existing File > Exit menu item. That's work that we can help them avoid. > In both 3.x and 4.x worlds prompting code resides at IDE levels. I don't see > moving that code to the JFace as a good task for the RCx. Um, there's nothing proposed to move into JFace. > So, I suggest that we roll back the last patch, close this bug as the original > defect will be fixed, open an enhancement request for streamlining Eclipse exit > handling. As I pointed out in comment #5, this isn't sufficient as you have to disable the code that hides menu items referencing org.eclipse.ui.file.exit (CocoaUIHandler#redirectHandledMenuItem()). My suggestion is: 1. Fix the IDE quit handler definition as per comment #12 2. Add a new command parameter ("neverConfirm" defaulting to false) 3. Change File > Exit to pass that parameter I think that keeps everybody happy, doesn't it?
Created attachment 215145 [details] Patch to add optional confirmation parameter I've attached a patch that adds an optional command parameter to org.eclipse.ui.file.exit called "confirm" (actually, org.eclipse.ui.file.exit.confirm). If "never", indicates that the exit should never prompt the user to confirm. This seemed better than a boolean if we wanted to encode other possibilities (e.g., "last" for if last-window). We can specify a set of parameter values if anybody thinks that's helpful.
(In reply to comment #21) > We can specify a set of parameter values if anybody thinks that's helpful. I wasn't sure of the language to use. We could get away with just the first parameter value, I suppose. + <values + class="org.eclipse.ui.commands.ExtensionParameterValues"> + <parameter + name="Never prompt for confirmation" + value="never"> + </parameter> + <parameter + name="Prompt for confirmation based on workbench preference" + value="preference"> + </parameter> + </values>
(In reply to comment #19) > I think we need to rollback the "Alternative approach" patch in RC1, but I > don't think we should do anything else here for 4.2. > > This will get us in a state where the actual issue reported (see comment 0) is > fixed and, as far as I can tell, we'll match 3.8 behavior. > > I do agree that Eclipse exit handling could be simplified, but this does not > strike me as something that needs to be done in a Release Candidate phase. > > For the impact on e4 RCP apps, it is easily mitigated. Using contacts demo as > an example, all that is needed is to add prompt to the ExitHandler - if RCP > author thinks that prompt is necessary. > > In both 3.x and 4.x worlds prompting code resides at IDE levels. I don't see > moving that code to the JFace as a good task for the RCx. > > So, I suggest that we roll back the last patch, close this bug as the original > defect will be fixed, open an enhancement request for streamlining Eclipse exit > handling. Given RC1, that's also what I'd suggest.
(In reply to comment #20) > That *was* true. It won't be true if we revert to your original patch, > unhooking Cmd-Q. They'll need to either add an SWT.Close handler to the Yes, you are correct. (In reply to comment #21) > Created attachment 215145 [details] > Patch to add optional confirmation parameter > I am not an expert on commands, so I am not sure of this is good: + <!-- Use count=* since we want this handler to always be active --> + <activeWhen> + <count + value="*"> + </count></activeWhen> This looks like a workaround build on the workaround from the previous patch. Even aside from being in RC, if we are to follow this line and RCP apps start to rely on it, we'll have to continue to support it in future. That in turn will complicate making a real solution and allowing E4 RCP apps hook into the shutdown sequence.
(In reply to comment #24) > I am not an expert on commands, so I am not sure of this is good: > > + <!-- Use count=* since we want this handler to always be active --> > + <activeWhen> > + <count > + value="*"> > + </count></activeWhen> > > This looks like a workaround build on the workaround from the previous patch. Good point. Short of building an always-true property tester, I didn't see a different way to always override a defaut handler. But perhaps I should just build the always-true propery tester.(In reply to comment #24) I'm fine with backing out the patch to remove the quit-rebinding. Just please remember to do the following: > As I pointed out in comment #5, this isn't sufficient as you have to disable > the code that hides menu items referencing org.eclipse.ui.file.exit > (CocoaUIHandler#redirectHandledMenuItem()).
Eric, McQ and I had a hallway discussion on what we want to do here. We want to limit changes to the code at this point of the release cycle. This means that we won't be providing a "real" fix to the shutdown lifecycle for 4.2. On the other hand, we don't want to break whatever works now. So, I'll rollback both changes checked in for this bug and we'll get back to the original status quo (no exit prompt on Mac, but RCP apps can easily intercept exit). This comes with a *disclaimer*: once we provide proper hooks into shutdown (4.3+), RCP apps might need to change their code. The current behavior is something that "just happened" and once the proper solution is in place it might have to be altered. Brian, if you feel comfortable providing a small fix for 4.2, you are welcome. Re comment 25 - I am not sure what is the effect of removing "org.eclipse.ui.file.exit" overrides. The only visible effect seems to be the application name ("E4 contacts demo" vs. "Eclipse") that appears for the app menu and app exit submenu. It does not seem to help with prompting on exit.
Both code changes rolled back, Git references: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=6545d9832a15f1b9a5568acfe46f78c7652ccbf0 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=741a0a346fe199d5a3c1a8e967f7ac49d7675988
Oleg, I'll +1 this but still want to know if the current behavior ends up saving all the dirty elements...
(In reply to comment #28) > Oleg, I'll +1 this but still want to know if the current behavior ends up > saving all the dirty elements... Yes, it does.
How about just remove this Key Binding on the Mac? Having a keystroke that I can't change and I can accidentally hit isn't on my list of happy thoughts. On the PC, I used to use a resource editor to remove the keybinding. Now on my Mac, I can't change it nor can I delete it. I occasionally hit Command-Q when I mean Control-Q and it makes me crazy!!! Still not fixed, btw. Ray
I forgot about this bug entirely. Marking for Mars.
Hey, Any update on this one ? I accidentally close my eclipse :) Thibault
Hey, I have tested this behaviour on windows today and alt+F4 show the "confirm exit" dialog, also this command is not bind with the menu, IMO we should adopt the same behaviour on osX
New Gerrit change created: https://git.eclipse.org/r/47490
For the Mars release, I did some more testing on the Mac, and this bug was a major pain again. I've submitted a new fix on current master. Brian, could you please approve/review/submit for RC1?
Gerrit change https://git.eclipse.org/r/47490 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f4fa21210a01ea26d4aaa58f09f8a4df48ae68a5
Thanks Markus.
Verified in 4.5.0.I20150510-2000 (Mac and Windows).
The fix seems scoped only for Mac. We should also consider the "Confirm exit when closing last window" on other platforms, if we select File -> Exit. This will be handled by Bug 512857. I hope that I can also remove the Mac specific coding with this bug.
(In reply to David Weiser from comment #39) > The fix seems scoped only for Mac. We should also consider the "Confirm exit > when closing last window" on other platforms, if we select File -> Exit. > This will be handled by Bug 512857. I hope that I can also remove the Mac > specific coding with this bug. Eclipse follows the principle to honor OS behavior and not to make them all work/look the same.
