Bug 511198 - Save Resource should use "Save" / "Don't save" instead of Yes / No
Summary: Save Resource should use "Save" / "Don't save" instead of Yes / No
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.7   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.7 M7   Edit
Assignee: Patrik Suzzi CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on: 511626
Blocks: 514230 514466
  Show dependency tree
 
Reported: 2017-01-27 09:29 EST by Lars Vogel CLA
Modified: 2017-03-30 06:07 EDT (History)
5 users (show)

See Also:


Attachments
Image: the new save dialog buttons (20.97 KB, image/png)
2017-02-02 02:54 EST, Patrik Suzzi CLA
no flags Details
image: Save list of resources is not changed (30.39 KB, image/png)
2017-02-02 03:05 EST, Patrik Suzzi CLA
no flags Details
image: also saveable list now displays Save/don't save (27.87 KB, image/png)
2017-02-03 20:54 EST, Patrik Suzzi CLA
no flags Details
iamge: updated save resources list selection dialog (25.29 KB, image/png)
2017-02-05 19:49 EST, Patrik Suzzi CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Vogel CLA 2017-01-27 09:29:55 EST
We should use verbs instead of yes, no dialogs.

If an editor is changed, and if you close it, a "Do you want to Save" dialog is shown. This dialog should say save no save (see Microsoft user guide for good wording).
Comment 1 Patrik Suzzi CLA 2017-02-02 02:54:16 EST
Created attachment 266584 [details]
Image: the new save dialog buttons

+1 as with "Save"/"Don't Save" dialog will be more consistent.

Code change will follow shortly

https://msdn.microsoft.com/en-us/library/windows/desktop/dn742470(v=vs.85).aspx
Comment 2 Eclipse Genie CLA 2017-02-02 03:02:10 EST
New Gerrit change created: https://git.eclipse.org/r/90151
Comment 3 Patrik Suzzi CLA 2017-02-02 03:05:13 EST
Created attachment 266585 [details]
image: Save list of resources is not changed

Please, note: as you can see in the image above, the "Save Resources" (list) dialog is not (yet) changed. 

I think we could change also the "Ok" / "Cancel" button labels with "Save" / "Don't Save". Do you agree?
Comment 4 Lars Vogel CLA 2017-02-02 04:03:56 EST
(In reply to Patrik Suzzi from comment #3)

> I think we could change also the "Ok" / "Cancel" button labels with "Save" /
> "Don't Save". Do you agree?

+1
Comment 5 Lars Vogel CLA 2017-02-03 14:26:57 EST
(In reply to Lars Vogel from comment #4)
> (In reply to Patrik Suzzi from comment #3)
> 
> > I think we could change also the "Ok" / "Cancel" button labels with "Save" /
> > "Don't Save". Do you agree?
> 
> +1

I think the Gerrit does not yet contain this change. Any plans to update it?
Comment 6 Patrik Suzzi CLA 2017-02-03 20:54:35 EST
Created attachment 266629 [details]
image: also saveable list now displays Save/don't save

Also the SaveableList$MyListSelectionDialog displays "Save" / "Don't
save" instead of "Yes" / "No"
Comment 7 Lars Vogel CLA 2017-02-03 23:16:44 EST
(In reply to Patrik Suzzi from comment #6)
> Created attachment 266629 [details]
> image: also saveable list now displays Save/don't save
> 
> Also the SaveableList$MyListSelectionDialog displays "Save" / "Don't
> save" instead of "Yes" / "No"

I think the Cancel label should be preserved, or even better called "Cancel Exit", if only used during existing the workbench.
Comment 8 Patrik Suzzi CLA 2017-02-05 19:34:22 EST
I(In reply to Lars Vogel from comment #7)
> I think the Cancel label should be preserved, or even better called "Cancel
> Exit", if only used during existing the workbench.

Thanks for pointing this. SaveableList seems generic. I'm rolling back "Don't save" to "Cancel"
Comment 9 Patrik Suzzi CLA 2017-02-05 19:49:36 EST
Created attachment 266650 [details]
iamge: updated save resources list selection dialog
Comment 11 Patrik Suzzi CLA 2017-02-05 21:31:45 EST
The change is now merged in master. Adding N&N shortly.
Comment 12 Eclipse Genie CLA 2017-02-05 21:31:59 EST
New Gerrit change created: https://git.eclipse.org/r/90379
Comment 14 Markus Keller CLA 2017-02-06 09:01:32 EST
These buttons still need mnemonics.

Make sure you don't introduce a conflict with "&Save" and "&Select All". Since "&Save" should use the standard mnemonic S [1], try to change "&Select All" to "Select &All". Check all existing users of that label for further conflicts.

[1] See e.g. table "Access keys" in https://msdn.microsoft.com/en-us/library/windows/desktop/dn742465(v=vs.85).aspx
Comment 15 Dani Megert CLA 2017-02-09 05:49:15 EST
http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=975698231155df5cf90dc72556c62b736a9702ee

This reverts the change since changing the labels for the MessageDialogWithToggle also changes the return codes and hence the mapping to ISaveablePart2.YES|NO|CANCEL fails.

Test Case:
1. open a file and modify it
2. open a new window
3. open the same file from step 1 (should be dirty)
4. close the editor
==> none of the buttons does something useful.


NOTE: "Don't save" must be title style, i.e. "Don't Save".
Comment 16 Lars Vogel CLA 2017-02-09 05:55:14 EST
(In reply to Dani Megert from comment #15)
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=975698231155df5cf90dc72556c62b736a9702ee
> 
> This reverts the change since changing the labels for the
> MessageDialogWithToggle also changes the return codes and hence the mapping
> to ISaveablePart2.YES|NO|CANCEL fails.

Looks like this also requires the changes in Bug 511626. Patrik, maybe you can review the related change and use it here?
Comment 17 Patrik Suzzi CLA 2017-02-14 09:05:35 EST
(In reply to Lars Vogel from comment #16)
> (In reply to Dani Megert from comment #15)
> > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> > ?id=975698231155df5cf90dc72556c62b736a9702ee
> > 
> > This reverts the change since changing the labels for the
> > MessageDialogWithToggle also changes the return codes and hence the mapping
> > to ISaveablePart2.YES|NO|CANCEL fails.
> 
> Looks like this also requires the changes in Bug 511626. Patrik, maybe you
> can review the related change and use it here?

Reviewed; indeed the change 90281 (for bug 511626) looks good. In practice, it adds a map to link button labels with proper return values. 

When that change is merged, I can push a new change, similar to the old change, but with these improvements: 
- "Save" and "Don't Save"
- mnemonics as suggested by Markus in comment #14
- label-to-value mappings: "Save"-> YES , "Don't Save" -> NO
Comment 18 Eclipse Genie CLA 2017-03-05 23:57:50 EST
New Gerrit change created: https://git.eclipse.org/r/92332
Comment 19 Dani Megert CLA 2017-03-06 11:56:38 EST
(In reply to Eclipse Genie from comment #18)
> New Gerrit change created: https://git.eclipse.org/r/92332

Too late for M6. I've removed the N&N entry.
Comment 20 Patrik Suzzi CLA 2017-03-06 20:53:38 EST
(In reply to Eclipse Genie from comment #18)
> New Gerrit change created: https://git.eclipse.org/r/92332

The current version of the proposed change fixes the bug and addresses Dani's issue from #comment 15

About Markus' issue from #comment 14, "These buttons still needs mnemonics", (see https://bugs.eclipse.org/bugs/attachment.cgi?id=266650), I suggest addressing this in a following independent change, where we can edit 
SelectionDialog_selectLabel = Select &All

And where we can introduce:
SaveableHelper_Save = &Save
SaveableHelper_Cancel = &Cancel
SaveableHelper_Dont_Save = &Don't Save

Having a new, independent change helps in separating the effects.
Comment 21 Eclipse Genie CLA 2017-03-10 09:23:03 EST
New Gerrit change created: https://git.eclipse.org/r/92782
Comment 22 Lars Vogel CLA 2017-03-10 09:27:48 EST
(In reply to Patrik Suzzi from comment #20)

> About Markus' issue from #comment 14, "These buttons still needs mnemonics",
> (see https://bugs.eclipse.org/bugs/attachment.cgi?id=266650), I suggest
> addressing this in a following independent change, where we can edit 
> SelectionDialog_selectLabel = Select &All
> 
> And where we can introduce:
> SaveableHelper_Save = &Save
> SaveableHelper_Cancel = &Cancel
> SaveableHelper_Dont_Save = &Don't Save
> 
> Having a new, independent change helps in separating the effects.

Sounds good to me. Can you please provide a Gerrit for the usages?
Comment 24 Lars Vogel CLA 2017-03-27 04:44:26 EDT
(In reply to Lars Vogel from comment #22)
> (In reply to Patrik Suzzi from comment #20)
> 
> > About Markus' issue from #comment 14, "These buttons still needs mnemonics",
> > (see https://bugs.eclipse.org/bugs/attachment.cgi?id=266650), I suggest
> > addressing this in a following independent change, where we can edit 
> > SelectionDialog_selectLabel = Select &All
> > 
> > And where we can introduce:
> > SaveableHelper_Save = &Save
> > SaveableHelper_Cancel = &Cancel
> > SaveableHelper_Dont_Save = &Don't Save
> > 
> > Having a new, independent change helps in separating the effects.
> 
> Sounds good to me. Can you please provide a Gerrit for the usages?

Opened Bug 514230 for tracking the mnemonics.
Comment 25 Eclipse Genie CLA 2017-03-27 07:51:39 EDT
New Gerrit change created: https://git.eclipse.org/r/93890
Comment 26 Eclipse Genie CLA 2017-03-27 07:52:32 EDT
New Gerrit change created: https://git.eclipse.org/r/93891
Comment 28 Lars Vogel CLA 2017-03-27 07:54:19 EDT
(In reply to Eclipse Genie from comment #27)
> Gerrit change https://git.eclipse.org/r/93890 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=65caa5209bb06d4a6d1ee8466d09621c9a56c961

Reverted to adjust the change to the changes of Bug 514058
Comment 29 Eclipse Genie CLA 2017-03-27 07:56:27 EDT
New Gerrit change created: https://git.eclipse.org/r/93892
Comment 31 Lars Vogel CLA 2017-03-28 08:49:20 EDT
(In reply to Markus Keller from comment #14)
> These buttons still need mnemonics.
> 
> Make sure you don't introduce a conflict with "&Save" and "&Select All".
> Since "&Save" should use the standard mnemonic S [1], try to change "&Select
> All" to "Select &All". Check all existing users of that label for further
> conflicts.

SelectionDialog_selectLabel ("Select All") is used by abstract class SelectionDialog hence we do not know who is using that already. David will define new label constants for it to avoid collisions with existing extenders.