Bug 463262 - Editor Selection dialog should not show a single radio button
Summary: Editor Selection dialog should not show a single radio button
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.5   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.5 M7   Edit
Assignee: Andrey Loskutov CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 378485
Blocks: 486859
  Show dependency tree
 
Reported: 2015-03-27 04:47 EDT by Dani Megert CLA
Modified: 2016-01-29 12:18 EST (History)
1 user (show)

See Also:


Attachments
proposed change (49.93 KB, image/png)
2015-03-29 10:25 EDT, Andrey Loskutov CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2015-03-27 04:47:28 EDT
The Editor Selection dialog should not show a single radio button. This happens when the file name has not extension.

Even in the case where there's an extension I would not use radio buttons where one of them repeats the file name that's already written at the top of the dialog.

How about:
[x] Always use this editor
  [ ] Use it for all '*.<EXTENSION>' files

The second option would not appear for a file name without extension.
Comment 1 Eclipse Genie CLA 2015-03-29 10:20:18 EDT
New Gerrit change created: https://git.eclipse.org/r/44803
Comment 2 Andrey Loskutov CLA 2015-03-29 10:25:28 EDT
Created attachment 251973 [details]
proposed change

(In reply to Dani Megert from comment #0)
> How about:
> [x] Always use this editor
>   [ ] Use it for all '*.<EXTENSION>' files
> 
> The second option would not appear for a file name without extension.

See attached screenshot matching the patch https://git.eclipse.org/r/44803

Small change to the proposal: *both* checkboxes are not checked, since OpenWithMenu *always* remembers this *particular* file after opening the editor via file.setPersistentProperty(EDITOR_KEY, editorID);

So default appearance of the dialog is:

[ ] Always use this editor
   [ ] Use it for all '*.<EXTENSION>' files
Comment 4 Andrey Loskutov CLA 2015-03-31 14:22:17 EDT
Fixed via https://git.eclipse.org/r/44803
Comment 5 Dani Megert CLA 2015-04-01 07:45:18 EDT
(In reply to Andrey Loskutov from comment #4)
> Fixed via https://git.eclipse.org/r/44803

Hi Andrey, another reminder not to change unrelated code in a change: with the above change you removed the space from this line:

PreferencePageParameterValues_pageLabelSeparator = \ >\ 

This has the effect that the next line is considered as value for the above key.

Fixed with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=650e6808d884f84f38fb6ce6b3fc2bf3dbed7108
Comment 6 Andrey Loskutov CLA 2015-04-01 07:56:44 EDT
(In reply to Dani Megert from comment #5)
> (In reply to Andrey Loskutov from comment #4)
> > Fixed via https://git.eclipse.org/r/44803
> 
> Hi Andrey, another reminder not to change unrelated code in a change: with
> the above change you removed the space from this line:
> 
> PreferencePageParameterValues_pageLabelSeparator = \ >\ 
> 
> This has the effect that the next line is considered as value for the above
> key.

Sorry, again. This one was highly surprising. I just had "remove trailing spaces" on to avoid commits with trailing spaces.

> Fixed with
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=650e6808d884f84f38fb6ce6b3fc2bf3dbed7108

Thanks again.
Comment 7 Dani Megert CLA 2015-04-01 09:17:49 EDT
(In reply to Andrey Loskutov from comment #6)
> Sorry, again. This one was highly surprising. I just had "remove trailing
> spaces" on to avoid commits with trailing spaces.

Where did you have that on? Our standard save action only works in the Java editor.

Can you please check your latest commits regarding the same issue (removed spaces in properties files)? Thanks.
Comment 8 Dani Megert CLA 2015-04-01 11:34:20 EDT
The change is what I had in mind. The only thing that needs to be changed is availability of the sub-option: it must be disabled it the parent option is unchecked.
Comment 9 Andrey Loskutov CLA 2015-04-01 13:48:56 EDT
(In reply to Dani Megert from comment #8)
> The change is what I had in mind. The only thing that needs to be changed is
> availability of the sub-option: it must be disabled it the parent option is
> unchecked.

Why? One is for the exact file name, another one for all files of that type.
The options are not dependent on each other.
Let us use plugin.xml as example. If I want open it with xml editor *once* (so the first option is unchecked), but want always open all "*.xml" files with xml editor too, why this option should be disabled?
Comment 10 Dani Megert CLA 2015-04-02 04:22:59 EDT
(In reply to Andrey Loskutov from comment #9)
> (In reply to Dani Megert from comment #8)
> > The change is what I had in mind. The only thing that needs to be changed is
> > availability of the sub-option: it must be disabled it the parent option is
> > unchecked.
> 
> Why? One is for the exact file name, another one for all files of that type.
> The options are not dependent on each other.
> Let us use plugin.xml as example. If I want open it with xml editor *once*
> (so the first option is unchecked), but want always open all "*.xml" files
> with xml editor too, why this option should be disabled?

Because it will also use the XML editor for that (and all other) plugin.xml files, since they end with *.xml.
Comment 11 Dani Megert CLA 2015-04-02 09:30:48 EDT
(In reply to Dani Megert from comment #7)
> (In reply to Andrey Loskutov from comment #6)
> > Sorry, again. This one was highly surprising. I just had "remove trailing
> > spaces" on to avoid commits with trailing spaces.
> 
> Where did you have that on? Our standard save action only works in the Java
> editor.
> 
> Can you please check your latest commits regarding the same issue (removed
> spaces in properties files)? Thanks.

Any answer to those questions? Thanks.
Comment 12 Andrey Loskutov CLA 2015-04-02 09:40:42 EDT
(In reply to Dani Megert from comment #11)
> (In reply to Dani Megert from comment #7)
> > (In reply to Andrey Loskutov from comment #6)
> > > Sorry, again. This one was highly surprising. I just had "remove trailing
> > > spaces" on to avoid commits with trailing spaces.
> > 
> > Where did you have that on? Our standard save action only works in the Java
> > editor.

I'm consuming my own dog food - AnyEdit plugin has an option which allows to automatically trim trailing whitespace in any text editor on save (hook on save command).

> > Can you please check your latest commits regarding the same issue (removed
> > spaces in properties files)? Thanks.
> 
> Any answer to those questions? Thanks.

I will check my commits this weekend.
Comment 13 Dani Megert CLA 2015-04-02 09:43:57 EDT
(In reply to Andrey Loskutov from comment #12)
> (In reply to Dani Megert from comment #11)
> > (In reply to Dani Megert from comment #7)
> > > (In reply to Andrey Loskutov from comment #6)
> > > > Sorry, again. This one was highly surprising. I just had "remove trailing
> > > > spaces" on to avoid commits with trailing spaces.
> > > 
> > > Where did you have that on? Our standard save action only works in the Java
> > > editor.
> 
> I'm consuming my own dog food - AnyEdit plugin has an option which allows to
> automatically trim trailing whitespace in any text editor on save (hook on
> save command).

You should (at least by default) not do this for (Java) properties files. Or make sure that only really unnecessary spaces are removed.
Comment 14 Andrey Loskutov CLA 2015-04-03 14:45:08 EDT
(In reply to Dani Megert from comment #7)
> Can you please check your latest commits regarding the same issue (removed
> spaces in properties files)? Thanks.

I've validated back to first commit that there were no such problems.

(In reply to Dani Megert from comment #10)
> (In reply to Andrey Loskutov from comment #9)
> > (In reply to Dani Megert from comment #8)
> > > The change is what I had in mind. The only thing that needs to be changed is
> > > availability of the sub-option: it must be disabled it the parent option is
> > > unchecked.
> > 
> > Why? One is for the exact file name, another one for all files of that type.
> > The options are not dependent on each other.
> > Let us use plugin.xml as example. If I want open it with xml editor *once*
> > (so the first option is unchecked), but want always open all "*.xml" files
> > with xml editor too, why this option should be disabled?
> 
> Because it will also use the XML editor for that (and all other) plugin.xml
> files, since they end with *.xml.

I can't understand that logic, are we now back to the radio-button "either or" style? I understood this bug that checkbox based UI was to allow all possible combinations?

Originally we had:

(x) Always use this editor
  ( ) Use it for all '*.<EXTENSION>' files
( ) Always use this editor
  (x) Use it for all '*.<EXTENSION>' files

Now we have:

[x] Always use this editor
  [ ] Use it for all '*.<EXTENSION>' files
[ ] Always use this editor
  [x] Use it for all '*.<EXTENSION>' files
[x] Always use this editor
  [x] Use it for all '*.<EXTENSION>' files

Your request:

[x] Always use this editor
  [ ] Use it for all '*.<EXTENSION>' files
[x] Always use this editor
  [x] Use it for all '*.<EXTENSION>' files

But what is the reason remove one combination and to have the only last combination (both on) above? How (much) this differs from the original radio boxes and why should one always select "Always use this editor" to be able to select "Use it for all '*.<EXTENSION>' files"? 

Can it be that we just should change some wording:

[x] Use this editor for all '<filename>' files
  [ ] Use it for all '*.<EXTENSION>' files
[ ] Use this editor for all '<filename>' files
  [x] Use it for all '*.<EXTENSION>' files
[x] Use this editor for all '<filename>' files
  [x] Use it for all '*.<EXTENSION>' files

Back to the plugin.xml example:

[x] Use this editor for all 'plugin.xml' files
  [ ] Use it for all '*.xml' files
[ ] Use this editor for all 'plugin.xml' files
  [x] Use it for all '*.xml' files
[x] Use this editor for all 'plugin.xml' files
  [x] Use it for all '*.xml' files

So I'm for either original either/or radio style, or for current "all in" checkbox style (eventually with new wording?).
Comment 15 Dani Megert CLA 2015-04-13 08:24:17 EDT
(In reply to Andrey Loskutov from comment #14)
> (In reply to Dani Megert from comment #7)
> > Can you please check your latest commits regarding the same issue (removed
> > spaces in properties files)? Thanks.
> 
> I've validated back to first commit that there were no such problems.

Thanks!

 
> (In reply to Dani Megert from comment #10)
> Can it be that we just should change some wording:

That definitely helps!


> Back to the plugin.xml example:
> 
> [x] Use this editor for all 'plugin.xml' files
>   [ ] Use it for all '*.xml' files
> [ ] Use this editor for all 'plugin.xml' files
>   [x] Use it for all '*.xml' files
> [x] Use this editor for all 'plugin.xml' files
>   [x] Use it for all '*.xml' files


I assume you see that checking the second (all *.xml) invalidates the first one, that's why I see them dependent:

> [x] Use it for all '*.xml' files
>   [x] Use this editor for all 'plugin.xml' files

And yes, it boils down to what we had with radios, but accomplishes that with just two controls, while originally it was one checkbox plus 2 radios.


> 
> So I'm for either original either/or radio style, or for current "all in"
> checkbox style (eventually with new wording?).
Comment 16 Eclipse Genie CLA 2015-04-18 17:55:33 EDT
New Gerrit change created: https://git.eclipse.org/r/46020
Comment 17 Andrey Loskutov CLA 2015-04-18 17:57:52 EDT
(In reply to Eclipse Genie from comment #16)
> New Gerrit change created: https://git.eclipse.org/r/46020

This patch is how I understood comment 15.
Comment 19 Dani Megert CLA 2015-04-20 08:46:52 EDT
Thanks Andrey.
Comment 20 Andrey Loskutov CLA 2015-04-28 16:54:55 EDT
Verified in I20150428-0800