Bug 309815 - Polish Resource Filter properties UI
Summary: Polish Resource Filter properties UI
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.6   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.6 M7   Edit
Assignee: Serge Beauchamp CLA
QA Contact:
URL:
Whiteboard:
Keywords: polish
Depends on: 301820
Blocks:
  Show dependency tree
 
Reported: 2010-04-20 10:25 EDT by Dani Megert CLA
Modified: 2010-04-23 11:01 EDT (History)
4 users (show)

See Also:


Attachments
New dialog layout, with better spacing. (39.78 KB, image/png)
2010-04-20 15:26 EDT, Serge Beauchamp CLA
no flags Details
New error handling (39.08 KB, image/png)
2010-04-20 15:27 EDT, Serge Beauchamp CLA
no flags Details
Patch (17.57 KB, patch)
2010-04-20 15:32 EDT, Serge Beauchamp CLA
no flags Details | Diff
Date chooser (31.73 KB, image/png)
2010-04-20 15:38 EDT, Serge Beauchamp CLA
no flags Details
No border for Date Chooser (7.47 KB, image/png)
2010-04-21 06:33 EDT, Dani Megert CLA
no flags Details
New patch (33.44 KB, patch)
2010-04-21 07:21 EDT, Serge Beauchamp CLA
no flags Details | Diff
Latest dialog UI (53.44 KB, image/png)
2010-04-21 07:22 EDT, Serge Beauchamp CLA
no flags Details
Latest dialog UI (30.18 KB, image/png)
2010-04-21 09:38 EDT, Serge Beauchamp CLA
no flags Details
New patch (43.41 KB, patch)
2010-04-21 09:40 EDT, Serge Beauchamp CLA
no flags Details | Diff
New patch (42.29 KB, patch)
2010-04-21 10:05 EDT, Serge Beauchamp CLA
no flags Details | Diff
Latest dialog UI (67.27 KB, image/png)
2010-04-21 17:32 EDT, Serge Beauchamp CLA
no flags Details
New patch (50.69 KB, patch)
2010-04-21 17:33 EDT, Serge Beauchamp CLA
no flags Details | Diff
New patch (52.11 KB, text/plain)
2010-04-22 08:23 EDT, Serge Beauchamp CLA
no flags Details
New patch (52.73 KB, text/plain)
2010-04-22 09:36 EDT, Serge Beauchamp CLA
no flags Details
Latest dialog UI (45.94 KB, image/png)
2010-04-22 09:37 EDT, Serge Beauchamp CLA
no flags Details
Final tweaks. (4.28 KB, patch)
2010-04-23 06:25 EDT, Serge Beauchamp CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2010-04-20 10:25:08 EDT
The previous version went through quite some polishing which now has to restart. The new version has many issues.

Maybe Susan can jump in and help with the polish.

Some immediate issues:
- UI looks noisy due to mix of elements in groups and not in groups
- alignments not right
- too little spacing between checkboxes
- too much empty space in some groups
- non-standard input validation: the dialog should show errors when entering the
  wrong value and not throw a dialog at the end when I click 'OK'
- since I can choose file and folder attributes to search, it is unclear what
  we search for when 'Regular Expression' or 'String Matcher' is selected. For
  the 'Regular Expression' case it says: "Matches objects based..." but it does
  not say how. Does it match the name only or also file length and other 
  attributes?
- date chooser has no border and is not centered
- I need two steps to add a filter: first I need to select the type (exclude or
  include in the list) and then click 'Add'
Comment 1 Martin Oberhuber CLA 2010-04-20 10:47:46 EDT
Do I get this right - you agree with the basic concepts, workflow and UI layout of the dialog as per bug 301820, but you now want to polish and fine-tune the details?

> - I need two steps to add a filter: first I need to select the type (exclude or
>   include in the list) and then click 'Add'

So you'd suggest 2 separate "add" buttons for "Add inclusion" and "Add exclusion" respectively?
Comment 2 Dani Megert CLA 2010-04-20 11:40:27 EDT
>Do I get this right - you agree with the basic concepts,
It depends what you mean with "basic". The fact that I can select the kind/attribute in the same drop down as the matching strategy is just wrong whether basic or not ;-)

>So you'd suggest 2 separate "add" buttons for "Add inclusion" and "Add
>exclusion" respectively?
No, I would add it back on the dialog because in the previous version it was very easy to switch between include and exclude (e.g. if I made a mistake). Now this is a pain since I have to delete the wrong rule and re-enter the values into the new one.
Comment 3 Serge Beauchamp CLA 2010-04-20 13:48:42 EDT
(In reply to comment #2)
> >Do I get this right - you agree with the basic concepts,
> It depends what you mean with "basic". The fact that I can select the
> kind/attribute in the same drop down as the matching strategy is just wrong
> whether basic or not ;-)
> 

This is because 'String Matcher' and 'Regular Expression' are filter matchers, while the 'File and Folder Attributes' is also a matcher which supports string matcher and regular expression matching.

The 2 matchers are there for legacy reasons, and I'd be more than willing to remove them, since their functionality is super-seeded by the FilesAndFolderAttributes matcher, but the reason I kept them is that it would cause some incompatibility if someone was already using them.

That being said, imagine another kind of matcher "CVS Status", and you can see that it fits properly in the same combo box as the 'File and Folder Attributes' one.

> >So you'd suggest 2 separate "add" buttons for "Add inclusion" and "Add
> >exclusion" respectively?
> No, I would add it back on the dialog because in the previous version it was
> very easy to switch between include and exclude (e.g. if I made a mistake). Now
> this is a pain since I have to delete the wrong rule and re-enter the values
> into the new one.

I'm not against moving the radio button back in the dialog.  

Note, though, that if you made a mistake (create an include instead of exclude), you can just drag and drop them between the 'Include Only' and 'Exclude All' notes, and their types will change accordingly.

Does that information change your opinion / anyone else has an opinion on that?
Comment 4 Martin Oberhuber CLA 2010-04-20 14:01:49 EDT
(In reply to comment #3)
> The 2 matchers are there for legacy reasons, and I'd be more than willing to
> remove them, since their functionality is super-seeded by the
> FilesAndFolderAttributes matcher, but the reason I kept them is that it would
> cause some incompatibility if someone was already using them.

Are these API, or are you just talking about existing project settings already using them? If they are not API, then just go and ditch them. No mercy for project settings created with a milestone build.

> Note, though, that if you made a mistake (create an include instead of
> exclude), you can just drag and drop them between the 'Include Only' and
> 'Exclude All' notes, and their types will change accordingly.

Hm, that's a cool feature but apparently it was not very obvious.

I'm a little torn here. I found the include vs exclude control in the original dialog confusing. On the other hand, I can understand Dani's point that having to make the include vs exclude decision before entering the dialog may cause issues.

In some sense, I like the concept of seeing the include vs exclude decision as one aspect of the pattern, which should therefore be controlled close to the pattern. I like the (undocumented) feature of our Wind River Search engine which allows filename extension patterns to switch between include/exclude mode just with an exclamation mark, e.g.
  
  *.c*, !*.cbak

but that's something for experts and not really intuitive (semantics not really clear). If somebody has an idea how that could be put into the dialog in a nice non-confusing way I'd be all for it.
Comment 5 Serge Beauchamp CLA 2010-04-20 14:45:21 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > The 2 matchers are there for legacy reasons, and I'd be more than willing to
> > remove them, since their functionality is super-seeded by the
> > FilesAndFolderAttributes matcher, but the reason I kept them is that it would
> > cause some incompatibility if someone was already using them.
> 
> Are these API, or are you just talking about existing project settings already
> using them? If they are not API, then just go and ditch them. No mercy for
> project settings created with a milestone build.
> 

They are not Java APIs, but they were in M6.  

That means that if someone has a filter using that matcher with M6, and they move the project to M7 and it doesn't have that matcher anymore, the filters won't be effective anymore.

I'm not sure if that qualifies as API or not.
Comment 6 Serge Beauchamp CLA 2010-04-20 15:26:57 EDT
Created attachment 165486 [details]
New dialog layout, with better spacing.
Comment 7 Serge Beauchamp CLA 2010-04-20 15:27:52 EDT
Created attachment 165487 [details]
New error handling
Comment 8 Serge Beauchamp CLA 2010-04-20 15:32:55 EDT
Created attachment 165488 [details]
Patch

Patch, now fixes (see my comments in parenthesis):

- alignments not right (if not, please be more specific)
- too little spacing between checkboxes
- non-standard input validation: the dialog should show errors when entering
the
  wrong value and not throw a dialog at the end when I click 'OK'
- since I can choose file and folder attributes to search, it is unclear what
  we search for when 'Regular Expression' or 'String Matcher' is selected. For
  the 'Regular Expression' case it says: "Matches objects based..." but it does
  not say how. Does it match the name only or also file length and other 
  attributes?  (Regular expression now says: "Matches file and folder names with a regular expression")
- date chooser has no border and is not centered (I can't see what's the problem with that widget, but I changed the GridData to centered just in case).

Does not fix:

- UI looks noisy due to mix of elements in groups and not in groups (I went back and forth many times regarding this, all elements in groups look very strange, does someone has a better idea?)
- too much empty space in some groups (are you referring to the description area?  if not please be more specific).
- I need two steps to add a filter: first I need to select the type (exclude or
  include in the list) and then click 'Add'
Comment 9 Serge Beauchamp CLA 2010-04-20 15:38:45 EDT
Created attachment 165490 [details]
Date chooser

See attached screenshot, first, there's no SWT.BORDER flag for the 'DataTime' widget, then, I don't see the center problem.
Comment 10 Susan McCourt CLA 2010-04-20 15:44:00 EDT
(In reply to comment #0)
> The previous version went through quite some polishing which now has to
> restart. The new version has many issues.
> 
> Maybe Susan can jump in and help with the polish.

Sorry for not jumping in to the original bug, as I think what I'd like to see is a compromise between the two approaches.  My thoughts:

1.  Include/Exclude.  I appreciate the work that has gone into the tree, but I'm not sure I buy into include/exclude as the parent/main separation.  
- one man's include is another's exclude..."exclude files before 1/1/2010" is the same as "include files after 1/1/2010."  I think you need to see what can be done (in the add dialog) before you make the distinction. 
- I think the page message could be reworked.  It is quite dense (and passive) and forces me to understand include and exclude before I really grok what a filter is.
- Likewise those top level items in the tree are confusing, at first glance I wonder if there are already filters defined.  The list should be blank when there is no filters
- if a rule is selected, the add... button is disabled.  I think "add..." should always be enabled

Suggestion:
- Return to the table style.  
- Ideally support sorting by column header.  User can sort by include/exclude if that is helpful.
- I prefer "Type" over "Mode" as the name for the include/exclude property
- simplify the property page message
"Resource filters determine which file system resources are added to the workspace during a refresh operation."
- add button is always enabled
- move the choice back into the dialog as the first group.  I agree that it was confusing the old way with the word "mode" and the terse phrasing.  Could we use more descriptive phrasing, such as

Filter type
<*> Exclude all resources that match this filter
< > Include only resources that match this filter

2.  I would like to see the files/folders choices grouped vertically to help reduce clutter.  I also think we could use checkboxes for file/folder and simply gray the rest of the dialog if the user actually deselects both of them.  This also allows us to move the "apply recursively" where it belongs.  Something like

Applies to
[ ] Files
[ ] Folders
     [ ] Apply recursively to folder structure


3.  I agree that the "file and folder attributes" type of filter replaces those other filters.  I don't understand the legacy concerns, but I think from a user point of view, there is no need to surface them as special types.  So I would suggest we retrieve the filter types (currently only one?) and use a label if there is only one, or a combo if there is more than one.  I also don't think we need an additional label, "condition."  It's a bit confusing and makes for poor alignment because the combo should align with the detail box below.  So it would look something like:

File and Folder Attributes  
[box with detail] 

or if there are indeed multiple filter types contributed

[File and Folder Attributes]   (combo)
[box with detail]


The "box with detail" could then just be
[Name]  [Matches] [                ]
  [ ] Case Sensitive
  [ ] Regular Expression

I suggest putting the checkboxes below the main line so that the general pattern is "Attribute" "Relationship" "Value"
and any detail needed (whether it is additional checkboxes or just the parenthetical explanation) is added below.

Right now, when you change the attribute in the combo box, the entire dialog resizes.  I can see that it should expand if you make a choice that won't fit, but it shouldn't shrink back if you change to something smaller.  


4.  I agree with Dani that we would want the error validation to happen in the dialog itself rather than a separate dialog at the end

I think these changes could eliminate the "noise" problems with mixed groups and non-groups, because everything would then be grouped (where the last group has no label because the label or combo suffices).  Also I think the vertical groups are just easier to read and don't seem so cluttered.

The grouping is a separate issue.  
I'm not sure I understand the concept entirely.  
- Isn't a "NOT" really just making an include behave like an exclude?
- aren't the filters assumed to be OR'ed in the absence of AND?  Why would I need to create a group that is an OR?
- Why is the "Applies to" choice available for groups?  wouldn't the choice made in the child filters apply rather than an overall choice?  (Maybe I'm misunderstanding the intention here)

Because it feels to me like we just need a "Combine" button that the user could push when multiple filters are selected, and this would combine them into an AND rule...
Comment 11 Susan McCourt CLA 2010-04-20 15:46:25 EDT
Hi, Serge, my comments collided with your changes.
In general I think my suggestions still apply as far as the bigger unaddressed items.

I like the use of that extra space as the place to show errors!
However I think you should keep the text black and just use the error icon to grab the user's attention that there is an error.  

Incidentally, I didn't notice a date/time border problem on win7
Comment 12 Serge Beauchamp CLA 2010-04-20 15:56:18 EDT
(In reply to comment #11)
> I like the use of that extra space as the place to show errors!
> However I think you should keep the text black and just use the error icon to
> grab the user's attention that there is an error.  
> 

Actually, I stole that UI example from the Search dialog, where it shows the text in red without an icon.

Should we keep it like that to be consistent with the Search dialog UI?
Comment 13 Serge Beauchamp CLA 2010-04-20 16:08:23 EDT
(In reply to comment #10)
>I also think we could use checkboxes for file/folder and
> simply gray the rest of the dialog if the user actually deselects both of them.

The valid options are:  Either files, folder, or files and folders (a tri-state attribute)

Neither files and folders is not a valid choice.  It seems strange to me to choose widgets whose states doesn't correspond to the underlying model.

The tri-state attribute with 3 radio buttons is also pervasively used in the Eclipse preferences.

Also, the icon for 'Files and Folders' matches the icon that is displayed in the tree in such case, which makes it consistent with the user choice.

Is the idea behind this suggestion simply to reduce the amount of widgets?

>  This also allows us to move the "apply recursively" where it belongs. 
> Something like
> 
> Applies to
> [ ] Files
> [ ] Folders
>      [ ] Apply recursively to folder structure
> 

I think there's a mis-understandment of the feature here, because the 'Apply recursively' has nothing to with the  files/folders selection.

The recursive notion has to do with the filter that applies to children of the current container.

So if you filter out .txt files, for example, having recursive will filter out .txt files from all children of the current container in the workspace.

(more to come...)
Comment 14 Serge Beauchamp CLA 2010-04-20 17:53:44 EDT
(In reply to comment #10)
> I'm not sure I understand the concept entirely.  
> - Isn't a "NOT" really just making an include behave like an exclude?

Not exactly, no. 

Lets take this example:

I have the following in my container:
   bar.cpp
   bar.txt
   foo.txt

And the following filters:

Include only:  
          Name matches bar.*
Exclude All:
          Name matches *.cpp

So the only thing will be shown is the following:
 bar.txt

Since foo.txt got excluded by the include only pass, and bar.cpp by the exclude all pass.

If the user would make instead the following filters:

Include only:  
          Name matches bar.*
          NOT ( Name matches *.cpp )
Exclude All:

This will have as effect to include all 3 files in the container, since the 3 files either start with "bar." or not end with ".cpp".

The proper conversion would be the following:

Include only:  
         (Name matches bar.* AND NOT ( Name matches *.cpp ) )
Exclude All:

This is much more complicated to specify for the user.  

Here also I'm assuming both targets (file/folder) is the same.  If the exclude filter has a different target than the include filter, it's impossible to combine them into one equivalent logical expression represented by one filter, for example

Include only:  
          [files] Name matches bar.*
Exclude All:
          [folders] Name matches CVS

The usage of the include/exclude filters is described by the text correctly, I think:

"A file system object will be added ... if it matches *any* of the include filters *and* doesn't match *any* of the exclude filters".

> - aren't the filters assumed to be OR'ed in the absence of AND?  Why would I
> need to create a group that is an OR?

Yes, the logical operation between filters of the same type (include/exclude) is OR.

The user would need to use the OR operator for more complex expressions, such as:

Include only:  
         (Name matches bar.* AND ( Creation Date matches Apr 8, 2010 OR  Creation Date matches Apr 9, 2010) )

> - Why is the "Applies to" choice available for groups?  wouldn't the choice
> made in the child filters apply rather than an overall choice?  (Maybe I'm
> misunderstanding the intention here)
> 

Only 'top level' elements (the ones directly under the include/exclude nodes) are filters.

A filter can apply for either files or folders or both.

So if I have a filter that says:

Include Only:
  (Name matches foo AND location matches c:/*)

We have to know "what" objects are the target of that filter.  Should the files be affected by this filter, or the folders as well?

> Because it feels to me like we just need a "Combine" button that the user could
> push when multiple filters are selected, and this would combine them into an
> AND rule...

As I mentioned above, it couldn't do that if the filters are of different type (include/exclude) or have different targets (file/folder).

The basic design is:

One filter (a top level item) determines uniquely if a file or folder passes through the refresh operation.

If I have existing filers, with complex expression, and I want to exclude files that end with ".txt", I don't have to start thinking about what is the logical operation, and where should I put the condition as if I had to carefully build a perl expression, and risk of breaking some older statement.

I just add an "exclude all filter for [files] name matches *.txt", and that's it.
Comment 15 Serge Beauchamp CLA 2010-04-20 18:03:59 EDT
(In reply to comment #10)
> - if a rule is selected, the add... button is disabled.  I think "add..."
> should always be enabled
> - add button is always enabled

It used to be that way, but then we changed it so the user can use it to add sub-elements into groups.

Before, it used to be that the only way to add a sub-element into a group was to right click on it, and select "Add..." (and also with drag and drop, but that's not obvious).

So it was reported that the "Add..." from the popup menu when right clicking was different than the "Add..." on the left.

Because the "Remove" and "Edit..." button applied to the tree selection, but the "Add..." and "Add Group..." did not, it was considered inconsistent.

So to make the "Add..." and "Add Group..." buttons consistent with both the other buttons on the left (basically, applies to the current tree selection), and consistent with the state of the same action items in the popup menu when right-clicking on the selected item, they now apply to the selection.

Are you saying that the "Add..." and "Add Group..." button should revert to disregard the current tree selection (as opposed to the other buttons on the left) and add only top level items?

In such case, the only way to create sub-elements into groups would be to use right-click and drag and drops.
Comment 16 Serge Beauchamp CLA 2010-04-20 18:20:44 EDT
(In reply to comment #10)
> (In reply to comment #0)
> 1.  Include/Exclude.  I appreciate the work that has gone into the tree, but
> I'm not sure I buy into include/exclude as the parent/main separation.  

There's several reasons why we changed it to being grouped by include/exclude instead of being a flat list.

1) There's a UI precedent of that in the JDT, in the 'Java Build Path' property page, in the source tab, when expanding one folder.

2) The include and exclude filters are grouped together logically. This is because the filters:

Include Only:
     Name matches *.java
     Name matches *.xml
Exclude All:
     Name matches foo.*

Is logically equivalent to:

 Include only ( ( Name matches *.java OR Name matches *.xml) AND NOT (Name matches foo.*)

So displaying it in a list in random order:

Include Only Name matches *.java
Exclude All  Name matches foo.*
Include Only Name matches *.xml

Makes it much harder to understand what's happening, and requires a mental exercise of re-interpreting the logical operations.

So we would need to keep them always sorted by type:

Include Only Name matches *.java
Include Only Name matches *.xml
Exclude All  Name matches foo.*

But then it becomes very redundant, seeing "Include Only" repeated multiple times, and overloads the presentation with repetitive information, hence the point of grouping them.

And it also matches the general presentation of the JDT include/exclude elements.

Does that make sense?

> - Return to the table style.  

The reason why we removed the columns, is partially because of grouping by include/exclude, but also to simplify the presentation.

When we use grouping by include/exclude, some column items don't apply in all rows (for example, none apply in the include/exclude rows).

Then with groups and sub-elements, most column elements don't apply either (the recursive flag, and the file/folder property).

So because the column items only applied to the top level filter, we made them as  part of the top level row instead (with a file/folder icon and optionally the recursive icon).

So only the appropriate information is displayed for a given row.

> - one man's include is another's exclude..."exclude files before 1/1/2010" is
> the same as "include files after 1/1/2010."  I think you need to see what can
> be done (in the add dialog) before you make the distinction. 

As I mentioned in comment #14, they are not equivalent.

> - Ideally support sorting by column header.  User can sort by include/exclude
> if that is helpful.

That's not helpful as far as I can see, since the order of the filters do not matter.  

The only thing that matters is the grouping include/exclude.

> - simplify the property page message
> "Resource filters determine which file system resources are added to the
> workspace during a refresh operation."

As I described in comment #14, I think this would skip over some important information, what do you think?
Comment 17 Serge Beauchamp CLA 2010-04-20 18:34:55 EDT
(In reply to comment #10)
> (In reply to comment #0)
> > The previous version went through quite some polishing which now has to
> - move the choice back into the dialog as the first group.  I agree that it was
> confusing the old way with the word "mode" and the terse phrasing.  Could we
> use more descriptive phrasing, such as
> 
> Filter type
> <*> Exclude all resources that match this filter
> < > Include only resources that match this filter
> 

That sounds fine to me, granted that the "Add.." button (see comment #15) ins't selection sensitive anymore, or if we don't display the groups when there's no filters, the 'Add..." in such case only would display the type widgets?

Or should we display the types even if the 'Add' button is selection sensitive (but then pre-selected to the proper type according to selection)?

> 2.  I would like to see the files/folders choices grouped vertically to help
> reduce clutter.  

Sounds good.

> 3.  I agree that the "file and folder attributes" type of filter replaces those
> other filters.  I don't understand the legacy concerns, but I think from a user
> point of view, there is no need to surface them as special types.  So I would
> suggest we retrieve the filter types (currently only one?)

I'd love to remove the legacy matchers, I'm not not sure we're allowed at this point.  

Maybe we can show them only if the user uses them already?  Or convert them to the new matcher equivalent automatically?

>and use a label if
> there is only one, or a combo if there is more than one.  I also don't think we
> need an additional label, "condition."  It's a bit confusing and makes for poor
> alignment because the combo should align with the detail box below.  So it
> would look something like:
> 
> File and Folder Attributes  
> [box with detail] 
> 
> or if there are indeed multiple filter types contributed
> 
> [File and Folder Attributes]   (combo)
> [box with detail]
> 

So should the box label be 'File and Folder Attributes'? or be a label above the box?

> 
> The "box with detail" could then just be
> [Name]  [Matches] [                ]
>   [ ] Case Sensitive
>   [ ] Regular Expression
> 
> I suggest putting the checkboxes below the main line so that the general
> pattern is "Attribute" "Relationship" "Value"
> and any detail needed (whether it is additional checkboxes or just the
> parenthetical explanation) is added below.
>

Here I'm following the 'File Search' Dialog UI.  

The thing is, those two checkboxes apply only when some items of the selection are set.  Other selection (such as the 'date modified' item) doesn't show those checkboxes.  

What do you think?
 
> Right now, when you change the attribute in the combo box, the entire dialog
> resizes.  I can see that it should expand if you make a choice that won't fit,
> but it shouldn't shrink back if you change to something smaller.  
> 

Fair enough.
Comment 18 Serge Beauchamp CLA 2010-04-20 18:36:17 EDT
Thanks Suzan for your insightful comments.

I know we have little time left before finalizing this UI, I look forward for your replies on the questions I raised above.
Comment 19 Serge Beauchamp CLA 2010-04-20 18:39:05 EDT
(In reply to comment #10)
> - Likewise those top level items in the tree are confusing, at first glance I
> wonder if there are already filters defined.  The list should be blank when
> there is no filters

That sounds good, especially with having the widgets to select the type in the dialog.
Comment 20 Susan McCourt CLA 2010-04-20 18:44:49 EDT
(In reply to comment #13)
> (In reply to comment #10)
> >I also think we could use checkboxes for file/folder and
> > simply gray the rest of the dialog if the user actually deselects both of them.
> 
> The valid options are:  Either files, folder, or files and folders (a tri-state
> attribute)
> 
> Neither files and folders is not a valid choice.  It seems strange to me to
> choose widgets whose states doesn't correspond to the underlying model.

This doesn't feel tri-state at all to me.  It's not blue, red, or green.  It's blue, red, or "all of the above."
> 
> The tri-state attribute with 3 radio buttons is also pervasively used in the
> Eclipse preferences.
> 
> Also, the icon for 'Files and Folders' matches the icon that is displayed in
> the tree in such case, which makes it consistent with the user choice.
> 
> Is the idea behind this suggestion simply to reduce the amount of widgets?

No, not exactly.  I was coming from the "don't make me think" point of view.   That third icon made me think something special, a true third choice, was in play.  But the third choice is really just saying "all of the above."  

That said, if the user wants folders instead of files, it's two clicks in the radio button model, instead of just one.  So I understand the argument for making it a third state.  I find the icons distracting, though.  Users know what a file and folder are, I don't think we need graphics to help make the point.

> >  This also allows us to move the "apply recursively" where it belongs. 
> > Something like
> > 
> > Applies to
> > [ ] Files
> > [ ] Folders
> >      [ ] Apply recursively to folder structure

You are right.  I misunderstood.
Honestly, I find the use of all these icons is making things harder to parse.  I saw the recursive icon and the folder icon, and in my mind these things were related.  I liked the icons in the advanced section of the new folder wizard, where we are helping show how choices relate to decorators seen in the explorer view.  But here, it seems more gratituous, and connects things in my mind that aren't related.

Maybe what's needed, though, is to move that checkbox above the main filter-writing group.  So that the things that determine "how the rule is applied" are kept together.  What about

Applies to
 < > Files
 < > Folders
 < > Files and folders

 [ ] Apply recursively to folder structure
Comment 21 Susan McCourt CLA 2010-04-20 18:57:18 EDT
(In reply to comment #17)
> That sounds fine to me, granted that the "Add.." button (see comment #15) ins't
> selection sensitive anymore, or if we don't display the groups when there's no
> filters, the 'Add..." in such case only would display the type widgets?
> 
> Or should we display the types even if the 'Add' button is selection sensitive
> (but then pre-selected to the proper type according to selection)?

If the tree roots stay as is, I think always display the types and  preselect according to the selection.  Much like how a creation wizard chooses the location based on the selection going into the wizard, but the user can always change it.  

> I'd love to remove the legacy matchers, I'm not not sure we're allowed at this
> point.  
> 
> Maybe we can show them only if the user uses them already?  Or convert them to
> the new matcher equivalent automatically?

That's what I was thinking...convert them automatically.


> So should the box label be 'File and Folder Attributes'? or be a label above
> the box?

Originally I was thinking label outside the box, but I realize now that the single/multiple case depends on the installation, not the individual resource, right?   In a particular install, there will always be the same number of filter types available in the properties dialog.  So we don't need to worry about making the single/multiple case consistent with each other.

Given all that, I think:
- label the group with the "File and Folder Attributes" if that is the only filter type.
- if there is more than one, use the combo and put the word "Filter Details" in the group.  

> Here I'm following the 'File Search' Dialog UI.  
> 
> The thing is, those two checkboxes apply only when some items of the selection
> are set.  Other selection (such as the 'date modified' item) doesn't show those
> checkboxes.  
> 
> What do you think?

Yeah, I noticed, though, that some of the other selections have static text detail (like explaining how the size is specified).  So I thought that checkboxes, static text, etc...all of that goes below, so that I can consistently see the three-part expression on the main row.
Comment 22 Susan McCourt CLA 2010-04-20 19:06:12 EDT
(In reply to comment #16)
> (In reply to comment #10)
> > (In reply to comment #0)
> > 1.  Include/Exclude.  I appreciate the work that has gone into the tree, but
> > I'm not sure I buy into include/exclude as the parent/main separation.  
> 
> There's several reasons why we changed it to being grouped by include/exclude
> instead of being a flat list.

Thanks for bring me up to date, sorry if all of this was discussed in the old bugs.  Given the reasoning, and the lateness of all of this, I'm fine with keeping as is, as long as we change the enablement of the Add button as discussed previously.  

> As I described in comment #14, I think this would skip over some important
> information, what do you think?

What if we had tooltips on the include and exclude tree roots? 
Since the user doesn't have to decide on include/exclude as their first order of business, I don't see that we have to explain it on the main page message.  For example, we don't explain "recursive" or "files and folders" or the other choices, and to me, this is just one of those kinds of choices.

Thanks for your patience, Serge...
I'm on IRC as susanmccourt if you want to have a more dynamic conversation, but hopefully I've answered the questions you had.
Comment 23 Dani Megert CLA 2010-04-21 06:26:42 EDT
I would entirely remove the condition selection (combo) since we agree that the other two are legacy and introduce confusion. This simplifies the UI quite a bit.

Though not having read all the arguments to switch to a tree I also agree with Susan that the tree is overkill here and that the usage of subtle different icons is hard to grasp. For example the 'Inheritable' flag is immediately recognized in the previous table based version while now, it's an icon that doesn't tell me much. The table was easier to read for me.

>- if a rule is selected, the add... button is disabled.  I think "add..."
>should always be enabled
Agree.

>2.  I would like to see the files/folders choices grouped vertically to help
+1

Maybe, we should do less grouping, e.g. replace the 'Applies to' group with:

Filter type:
(o) Exclude all resources that match this filter
( ) Include only resources that match this filter

Resources to filter: [Combo with the three options]
[ ] Apply filter to all children


>Actually, I stole that UI example from the Search dialog, where it shows the
>text in red without an icon.
Correct, Search and Find/Replace do it that way. However, since you also have to report other problems (like invalid numbers), we should use the approach used in wizards where we show the problem at the top or bottom with an icon and black text.
Comment 24 Dani Megert CLA 2010-04-21 06:33:47 EDT
Created attachment 165550 [details]
No border for Date Chooser

Note that this is with the 'Windows Classic' theme.
Comment 25 Serge Beauchamp CLA 2010-04-21 06:52:57 EDT
(In reply to comment #20)
> (In reply to comment #13)
> > (In reply to comment #10)
> > >I also think we could use checkboxes for file/folder and
> > > simply gray the rest of the dialog if the user actually deselects both of them.
> > 
> > The valid options are:  Either files, folder, or files and folders (a tri-state
> > attribute)
> > 
> > Neither files and folders is not a valid choice.  It seems strange to me to
> > choose widgets whose states doesn't correspond to the underlying model.
> 
> This doesn't feel tri-state at all to me.  It's not blue, red, or green.  It's
> blue, red, or "all of the above."
> > 
> > The tri-state attribute with 3 radio buttons is also pervasively used in the
> > Eclipse preferences.
> > 
> > Also, the icon for 'Files and Folders' matches the icon that is displayed in
> > the tree in such case, which makes it consistent with the user choice.
> > 
> > Is the idea behind this suggestion simply to reduce the amount of widgets?
> 
> No, not exactly.  I was coming from the "don't make me think" point of view.  
> That third icon made me think something special, a true third choice, was in
> play.  But the third choice is really just saying "all of the above."  
> 
> That said, if the user wants folders instead of files, it's two clicks in the
> radio button model, instead of just one.  So I understand the argument for
> making it a third state.  I find the icons distracting, though.  Users know
> what a file and folder are, I don't think we need graphics to help make the
> point.
> 

Fair enough, I'll remove the icons for the file and folders.
Comment 26 Serge Beauchamp CLA 2010-04-21 07:04:33 EDT
Regarding the comment:

> Filter type
> <*> Exclude all resources that match this filter
> < > Include only resources that match this filter

First, what is filtered is not 'resources', but file system objects, that's why the matcher is called 'File and Folder Attributes' instead of 'Resource Attributes', because we can't filter on Resource attributes (markers, derived state, etc..).

Then, doesn't it sound repetitive or redundant?
Comment 27 Serge Beauchamp CLA 2010-04-21 07:21:16 EDT
Created attachment 165554 [details]
New patch

Changes:

General:
- Legacy matchers are now automatically converted by the property page.

Dialog:
- The dialog now shows the types (include/exclude).
- The file/folders/file and folders widgets are aligned vertically
- The file/folders/file and folders widgets don't have icons anymore
- the 'condition' label is removed
- The 'matcher' combo only shows above the 'Filter Details' box if there's more than 1 matcher available.
- If there's only 1 matcher, the box's label takes the matcher name instead. 
- If there's an error reported in the 'condition' group, the 'OK' button is disabled.
- The dialog now resizes only larger to accommodate matcher UI - not smaller.
- The 'Apply recursively' is now part of the 'Applies to' box.
- The dialog caption and icon changes according to the type selection.  Does this look strange?

Tree:
- The include/exclude groups are not shown if there's no elements
- The 'Add.." buttons are enabled in there's no selection

Right now, I'm waiting for a reply regarding the Add.. buttons selection sensitiveness/consistency issue.
Comment 28 Serge Beauchamp CLA 2010-04-21 07:22:13 EDT
Created attachment 165555 [details]
Latest dialog UI

Shows the updated dialog UI so far
Comment 29 Dani Megert CLA 2010-04-21 08:01:38 EDT
>Created an attachment (id=165555) [details] [diff]
>Latest dialog UI
Looks better. I still don't like the icons, especially in front of the checkbox. We never do this in the Eclipse UI and the UI would look more standard without them.

The title looks wrong. It should not mention include or exclude.

Maybe we should only use 3 columns instead of 4: the text field is a bit small.

The help text for the text field looks somehow lost. Should be moved closer to the widgets above.

>- The include/exclude groups are not shown if there's no elements
Sounds strange to me.

I agree with Susan that 'Add..' should always be enabled.
Comment 30 Serge Beauchamp CLA 2010-04-21 09:36:28 EDT
(In reply to comment #29)
> >Created an attachment (id=165555) [details] [details] [diff]
> >Latest dialog UI
> Looks better. I still don't like the icons, especially in front of the
> checkbox. We never do this in the Eclipse UI and the UI would look more
> standard without them.
> 

You mean for the 'Apply recursively' ?  I don't mind removing all, but in such case, the user might not see that the same icon in the treeview represents that flag.

> The title looks wrong. It should not mention include or exclude.
> 
ok
> Maybe we should only use 3 columns instead of 4: the text field is a bit small.
> 

sound good.

> The help text for the text field looks somehow lost. Should be moved closer to
> the widgets above.
> 

will do.

> >- The include/exclude groups are not shown if there's no elements
> Sounds strange to me.
> 

This change was from Suzan's comment:

(In reply to comment #10)
> - Likewise those top level items in the tree are confusing, at first glance I
> wonder if there are already filters defined.  The list should be blank when
> there is no filters


> I agree with Susan that 'Add..' should always be enabled.

So are you saying that the 'Add..' button should not be used for adding sub-elements, or that if the selection can't have sub-elements, or that there's no selection, the 'Add...' button should still be enabled, and add top level filters instead?
Comment 31 Serge Beauchamp CLA 2010-04-21 09:38:23 EDT
Created attachment 165574 [details]
Latest dialog UI
Comment 32 Serge Beauchamp CLA 2010-04-21 09:40:43 EDT
Created attachment 165575 [details]
New patch

Now changed:

- The dialog caption doesn't include 'include/exclude'.
- All items, including the DataTime widget are properly centered.
- The case-sensitive/regular expression checkboxes are now below the text (3 columns instead of 4)
- the text is more up, using less space.
Comment 33 Serge Beauchamp CLA 2010-04-21 10:05:16 EDT
Created attachment 165580 [details]
New patch

remove unnecessary change.
Comment 34 Dani Megert CLA 2010-04-21 10:08:52 EDT
> I don't mind removing all, but in such
>case, the user might not see that the same icon in the treeview represents that
>flag.
Right, as said before: even with the icon being there I had no clue what it meant when I saw it first in the tree ==> use the table with a column that shows the inheritable state.

At first, I didn't even know that 'Add...' hat two different usages. So maybe we show two buttons, one to add top-level elements and one to add sub-elements.
Comment 35 Serge Beauchamp CLA 2010-04-21 16:57:14 EDT
(In reply to comment #34)
> > I don't mind removing all, but in such
> >case, the user might not see that the same icon in the treeview represents that
> >flag.
> Right, as said before: even with the icon being there I had no clue what it
> meant when I saw it first in the tree ==> use the table with a column that
> shows the inheritable state.
> 

Well, as discussed before, the problem with the column is that it doesn't apply to most rows, and even to none of the rows most of the time.

It could be represented in the tree in other ways than the icon - for instance, with a "(recursive)" suffix after the description

What do you think?

My concern is mostly about renaming that item.  Confusion comes from the word 'folder' as opposed to 'container' or resources, the user (as Suzan did) might believe it has to do with the folder that the filter applies to, as opposed to the resource container that contains the filter.

Also, since that item is now in the 'Applies to' group, repeating 'Apply...' is redundant.

So what about:

'All the resource hierarchy under project 'foo'

If the current container is a project, and 

'All the resource hierarchy under folder 'foo'

If the current container is a folder.

What do you think?

> At first, I didn't even know that 'Add...' hat two different usages. So maybe
> we show two buttons, one to add top-level elements and one to add sub-elements.

In such case, we'd need 2 addition buttons - one for 'Add Child...' and another for 'Add Child Group...'
Comment 36 Serge Beauchamp CLA 2010-04-21 17:32:14 EDT
Created attachment 165661 [details]
Latest dialog UI

Now shows the 'recursive' flag with the (recursive) text in the tree view.

Removes the icons from the dialog, rename the 'Recursive' label.
Comment 37 Serge Beauchamp CLA 2010-04-21 17:33:13 EDT
Created attachment 165662 [details]
New patch

Most issues are addressed I believe, beside the 'Add..' / selection.
Comment 38 Szymon Brandys CLA 2010-04-22 04:29:09 EDT
(In reply to comment #37)
Looking at the comments, I'm not sure whether 'Add..' is supposed to be selection sensitive or not.
Comment 39 Serge Beauchamp CLA 2010-04-22 06:50:20 EDT
(In reply to comment #38)
> (In reply to comment #37)
> Looking at the comments, I'm not sure whether 'Add..' is supposed to be
> selection sensitive or not.

Fair enough, lets make it not selection sensitive after all.
Comment 40 Serge Beauchamp CLA 2010-04-22 08:23:40 EDT
Created attachment 165744 [details]
New patch

Add buttons are now always enabled, and only add top level items (not selection sensitive anymore).
Comment 41 Dani Megert CLA 2010-04-22 08:42:12 EDT
Latest version looks much better!
I would
- put the resource type and name into the title (like it's already in the
  Properties dialog), e.g.
  Edit Resource Filter for Project testing
- try to find a better wording for
    'All the resource hierarchy under project 'foo'
    'All the resource hierarchy under folder 'foo'
  ==> [ ] All children (recursive)
  This has the advantage that we use the same string "(recursive)" as in the tree - align the case and regex widget to the right (below text field)


>In such case, we'd need 2 addition buttons - one for 'Add Child...' and another
>for 'Add Child Group...'
I think it's overkill. I would simply keep 'Add...' enabled all the time and if a leaf filter is selected we add a new top-level filter.

We could also trigger the dialog when double-clicking on the Include Only /Exclude all node.
Comment 42 Serge Beauchamp CLA 2010-04-22 09:36:29 EDT
Created attachment 165759 [details]
New patch

Now includes:

- put the resource type and name into the title
- use "[ ] All children (recursive)"
- Add...  always enabled, adds new sub-element if a eligible node is selected, otherwise add a new filter
- Triggers the add filter dialog when double-clicking on the include/exclude nodes
- Re-align the reg-ex and case sensitive checkboxes with the text field.
Comment 43 Serge Beauchamp CLA 2010-04-22 09:37:26 EDT
Created attachment 165760 [details]
Latest dialog UI

Latest UI that makes it current with all suggestions.
Comment 44 Serge Beauchamp CLA 2010-04-22 16:45:01 EDT
 A whole day without new comments!  Lets celebrate and close this bug!

Now fixed on head.
Comment 45 Dani Megert CLA 2010-04-23 03:24:21 EDT
Almost perfect now ;-)

These issues are left:
1) We don't use ' in the titles (see e.g. title of properties dialog)
2) The date field is centered now but it still has no border as shown in
   attachment 165550 [details]
3) The height of the text widget is too small (compared to the combo)
4) I would make the second combo always same width so that the UI doesn't
   resize when the user changes the value in the first combo
Comment 46 Dani Megert CLA 2010-04-23 03:25:38 EDT
.
Comment 47 Serge Beauchamp CLA 2010-04-23 06:25:41 EDT
Created attachment 165893 [details]
Final tweaks.

Now changed:

- We don't use ' in the titles (see e.g. title of properties dialog)
- The date field now has a border 
 -The height of the text widget is too small (compared to the combo)
- The second combo always same width so that the UI doesn't
resize when the user changes the value in the first combo.

Note that in order to make the text widget the same height as the combo, I had to hard-code an offset.

Somehow, when I did heightHint = combo.computeSize(SWT.DEFAULT).y; // which is 21

The Text widget ended up being 27 - 6 pixels taller than the combo.  So what's now in the code is 

heightHint = combo.computeSize(SWT.DEFAULT).y - 6;

Which causes the Text height ends up being exactly the same as the combo - 21.

Not sure what's happening here, if anyone has an idea.
Comment 48 Serge Beauchamp CLA 2010-04-23 06:25:56 EDT
Now fixed on head
Comment 49 Dani Megert CLA 2010-04-23 10:44:19 EDT
Using a fixed amount of pixels is not a good idea (might be different on different platforms and with other fonts).

The problem was that different SWT layout flags (CENTER vs. FILL) were used and hence the controls didn't get the same size. I've also fixed this for the Date chooser which wasn't aligned even with your latest fix.

The whole class probably needs a review/polish for the correct usage of the SWT layout flags but I'm currently completely out of time. Maybe Susan can jump in here but I that can wait until 3.7.
Comment 50 Serge Beauchamp CLA 2010-04-23 11:01:34 EDT
(In reply to comment #49)
> The problem was that different SWT layout flags (CENTER vs. FILL) were used and
> hence the controls didn't get the same size. I've also fixed this for the Date
> chooser which wasn't aligned even with your latest fix.
> 
Oh, I see.  thanks!