Community
Participate
Working Groups
I always have a difficult time finding the correct preference on this page - scrolling and twisties are painful. (Plus we do not have mnemonics for all the preferences - I don't care for mnemonics too much but someone else might) Possible solutions: 1. Sub pages for each category - Preferences in each category (except potential programming problems) fit in one screen, hence (almost) no scrolling required. - Lesser mouse clicks - Opening and closing a twistie is 2 mouse clicks, selecting a sub page would be only one mouse click Pros - Simple to implement! Cons - Names of 3 categories are longer than 2 words, hence the subpages names in the tree will not look too good. - You still have to know the category to find the preference quickly - and except for very frequently used preferences this will not be true. 2. Create a filter box in this page - This might be useful in other preference pages as well? 3. Use the existing filter box on the left side to reveal and highlight individual preferences. - I do not know how easy or difficult it is going to be with existing UI 4. Mozilla provides Power User preference management where you can filter by single pref. Maybe we can add a node 'All' at the bottom of the tree in left pane, which just lists all the preferences available in a filterable table containing 3 columns - key, type, default value?, value. - A command can be added to directly go to this 'All' page - This 'All' page is something similar to what is suggested for Properties View on http://wiki.eclipse.org/index.php/UI_Best_Practices_v3.x#Properties_View. My favorite is option 4, but would settle for Option 2 as well.
We once had tabs on that page, but we had to remove them, because the page became too wide. On some platforms (Mac), TabFolders are even just cut off if they don't fit on a single line. > 1. Sub pages for each category With sub-pages, we have the problem that we want all these problems together as one group of project-specific preferences. It would be confusing if one such set spanned multiple pages. > 3. Use the existing filter box If we wanted to reuse the global filter field, we would need API to hook us in there, and we would need a way to highlight matches in the shown page. But I think it would be confusing to reuse the tree filter field for this. And it would create a problem with lazy loading (currently, page names and keywords are contributed in the plugin.xml, and plug-ins that contribute a preference page are only loaded when the preference page is shown). > 4. Mozilla provides Power User preference management where you can filter by > single pref. That would need a model for the preferences and is on the E4 radar, see bug 255371. > 2. Create a filter box in this page That's the only viable option for now. I think that could be useful, but it's also quite some work (we e.g. also need to show/hide slave options in a way that makes sense).
(In reply to comment #1) > 2. Create a filter box in this page > That's the only viable option for now. I think that could be useful, but it's > also quite some work (we e.g. also need to show/hide slave options in a way > that makes sense). Agree, let me try this out then and see how much work it is. Except for 'Deprecated API' preference we already have some logic for enabling/disabling slave options, I am hoping to be able to use the same here as well.
The master of a slave option always must be visible, and I think it would also make sense to always show all slaves when a master is shown, i.e. we only show/hide complete master/slaves blocks. Implementation-wise, I guess you could just call setVisible(false) on the categories and preference controls that don't match the filter. If easily possible, please try to implement the filtering in OptionsConfigurationBlock, so that we can use the same implementation if we later want to add filtering to other pages as well.
Created attachment 173202 [details] v0.1 The code was written just to see how the feature will work, and it can be simplified and generalized to a great extent. But I am pretty happy with the filtering behavior with this patch.
Created attachment 173328 [details] v0.5 Pretty much the same filtering behavior as the last patch. But the code is much simpler, lesser and I think quite generic as well.
Created attachment 173423 [details] v0.6 Made a few small changes - added support for Text fields - moved the filter field outside the scrolled area - some small bug fixes (I also quickly implemented the filtering for Java > Compiler > Building preference page to test the Text fields and catch any other errors.)
I've played with the patch a bit, and I like the feature. The update delay feels good. Problems I saw in the UI (did not look at the code): - The final solution needs a filter field that has all the bells and whistles of the FilteredTree (e.g. use native SWT.SEARCH etc. where available, gray "type filter text", clear icon). We can discuss in the call how to proceed with this. - When filter matches a group title, the children should also be shown (e.g. "shadow" gives me a title but no options) - Scrolling needs some work, e.g. scrollbar should become smaller as matches are found, and while filtering, the last selected (or the top item) in the scrolled area should stay where it is, if possible. I think a common scenario is that you filter, find an interesting option, and then remove the filter to see what other options are in the vicinity. This works fine in the FilteredTree because you can select an item and the tree keeps showing the selection even if you remove the filter.
Created attachment 173455 [details] v0.6.1 (In reply to comment #7) > - When filter matches a group title, the children should also be shown (e.g. > "shadow" gives me a title but no options) Done > - Scrolling needs some work, e.g. scrollbar should become smaller as matches > are found, and while filtering, the last selected (or the top item) in the > scrolled area should stay where it is, if possible. > I think a common scenario is that you filter, find an interesting option, and > then remove the filter to see what other options are in the vicinity. This > works fine in the FilteredTree because you can select an item and the tree > keeps showing the selection even if you remove the filter. With this patch the scroll bar gets bigger (i.e. the height of the scrolled area is reduced) as the items are filtered out - this fixes some of the problems. However, currently it is not possible to keep an item 'selected' and then filter. About keeping the top item in the scrolled area in its position - what do we do when this is also filtered out? Also I do not know if this is possible.
I also like it (no code review)! What would also be cool is to apply the filter on the values so that I can see all options that are e.g. set to 'Ignore'. The other discussion is whether this new UI only applies to our 'Errors/Warnings' page or whether this should become a more general solution (PDE preference pages come to mind immediately).
Created attachment 173569 [details] v0.7 (In reply to comment #9) > What would also be cool is to apply the filter > on the values so that I can see all options that are e.g. set to 'Ignore'. Done. e.g. '~ignore' will show the combo boxes with value set to ignore, '~true' will show checkboxes which are selected. Why '~'? - It is not used in any preference label. - I have seen it used in other places to change the meaning of filter/search text (e.g Google) > The other discussion is whether this new UI only applies to our > 'Errors/Warnings' page or whether this should become a more general solution > (PDE preference pages come to mind immediately). I think it should become a more general solution, even I had thought of the PDE preference pages as they also have a large number of preferences. (In reply to comment #7) > - The final solution needs a filter field that has all the bells and whistles > of the FilteredTree (e.g. use native SWT.SEARCH etc. where available, gray > "type filter text", clear icon). Added the gray text. So the last item remaining is to add the other bells and whistles of FilteredTree.
>- The final solution needs a filter field that has all the bells and whistles >of the FilteredTree (e.g. use native SWT.SEARCH etc. where available, gray >"type filter text", clear icon). We can discuss in the call how to proceed with >this. See bug 293230.
Created attachment 173784 [details] v0.9 > - The final solution needs a filter field that has all the bells and whistles > of the FilteredTree (e.g. use native SWT.SEARCH etc. where available, gray > "type filter text", clear icon). Done. I copied over the relevant code from FilteredTree into a new class FilterTextControl (tested on Win XP). Once Bug 293230 is fixed we can simply get rid of this class and start using the hint flags directly.
(In reply to comment #12) > Created an attachment (id=173784) [details] [diff] > v0.9 Tested this patch on Linux GTK as well - the filter field has the native look and feel.
The overall code structure looks good. FilterTextControl: - The disabled state needs some work (project properties page with project specific settings disabled) - Static initializer and location of the images need to be fixed before this can be released. Depends on bug 293230. Maybe we'll just use our image registry as an intermediary solution until bug 293230 is done (would need a new bug for clean up before 3.7). OptionsConfigurationBlock: - The code needs a bit of documentation, e.g. PreferenceTreeNode constructor and fields should have Javadocs that tell which variables can be null. - If I saw that right, almost all usages of new PreferenceTreeNode(..) pass true for showAllChildren. You should create a constructor that always uses true and ripple it up through the call hierarchy. Same for non-null keys. - You cannot use the UI label as key in the 'data' field of the ExpandableComposites. At first sight, it looks like a clever trick, but it's hard to understand and can lead to various problems. Better use getLocalKey(...) with a unique argument. - FilteredPreferenceTree#excludeControl(Control, boolean) has a bad name. Can't you name this setVisible and also call control.setVisible() there? - In FilteredPreferenceTree, only offer add*() methods that are tested. I'm pretty sure addCheckBoxWithLink(..) wouldn't work. - The Job name must be NLSd. - fExpandedComposites has a typo. UI: - The page is hard to understand when all preferences are filtered. There's a "Select the severity...:" label immediately followed by the "Treat optional errors..." checkbox. Either include the checkbox in the filtering or add a message when all preferences are filtered. - filter for boolean value should work with terms "on"/"off" (like in the help) and with "enabled"/"disabled". "true"/"false" should not be used to describe checkbox state in the UI. - Message tweaks: Replace "(use ~ to filter on preference values. e.g. ~ignore)" with "(use ~ to filter on preference values, e.g. ~ignore or ~off)"
Created attachment 175348 [details] v1.0 (In reply to comment #14) > FilterTextControl: > - The disabled state needs some work (project properties page with project > specific settings disabled) DONE > - Static initializer and location of the images need to be fixed before this > can be released. Depends on bug 293230. Maybe we'll just use our image registry > as an intermediary solution until bug 293230 is done (would need a new bug for > clean up before 3.7). DONE. As discussed on sametime, image descriptors remain the same and the images are created and disposed manually in FilterTextControl. > OptionsConfigurationBlock: > - The code needs a bit of documentation, e.g. PreferenceTreeNode constructor > and fields should have Javadocs that tell which variables can be null. DONE > - If I saw that right, almost all usages of new PreferenceTreeNode(..) pass > true for showAllChildren. You should create a constructor that always uses true > and ripple it up through the call hierarchy. Same for non-null keys. Yes, you are right. I added FilteredPreferenceTree.addCheckBox(...) and FilteredPreferenceTree.addComboBox(...) methods which always use showAllChildren as true. I have not created a new constructor as that would result in lots of code duplication. > - You cannot use the UI label as key in the 'data' field of the > ExpandableComposites. At first sight, it looks like a clever trick, but it's > hard to understand and can lead to various problems. Better use > getLocalKey(...) with a unique argument. Yeah, this did not feel right to me as well. Fixed it now. > - FilteredPreferenceTree#excludeControl(Control, boolean) has a bad name. Can't > you name this setVisible and also call control.setVisible() there? DONE > - In FilteredPreferenceTree, only offer add*() methods that are tested. I'm > pretty sure addCheckBoxWithLink(..) wouldn't work. *hmm..I can do that but what is wrong with addCheckBoxWithLink(..)? Are you referring to the the fact that a Button is not returned from this method? But it can be easily obtained by using OptionsConfigurationBlock#getCheckBox(Key). This would be similar to what I am doing for ExpandableComposite. > - The Job name must be NLSd. > - fExpandedComposites has a typo. DONE > UI: > > - The page is hard to understand when all preferences are filtered. There's a > "Select the severity...:" label immediately followed by the "Treat optional > errors..." checkbox. Either include the checkbox in the filtering or add a > message when all preferences are filtered. DONE. Added a '(no option matches the filter)' label. NOTE: To workaround Bug 118659 I have used a trick. I have added a PaintListener to the empty composite which makes it invisible if the composite does not have any children. > - filter for boolean value should work with terms "on"/"off" (like in the help) > and with "enabled"/"disabled". "true"/"false" should not be used to describe > checkbox state in the UI. DONE > - Message tweaks: > Replace "(use ~ to filter on preference values. e.g. ~ignore)" > with "(use ~ to filter on preference values, e.g. ~ignore or ~off)" DONE
Created attachment 175599 [details] v1.1 When I did the last review, I changed the stuff I proposed (to see whether it really works). So in the end, I did the stuff I wrote below and a few more cleanups. FilterTextControl: - The public setEnabled(..) method is bad, since it does not set enablement. You should omit this method and inline the code in the anonymous Composite. The two anonymous classes should also be merged into one. - The patch contains too many hacks for FilteredPreferenceTree #getHeaderComposite(). And when I looked at the UI again, I found that the order of the label and the filter control was wrong in the first place. The label should be on top of the filter (like in other similar places: Open Type, New wizards, ...). Then you can also add a mnemonic to the label (&S). Code-wise, I would even move the creation of the label into the FilteredPreferenceTree (pass a string to the constructor, or null for no label). - In ProblemSeveritiesConfigurationBlock#createStyleTabContent(Composite), "int nColumns= 3;" should be moved down a bit. This only makes sense for controls inside the GridLayout that has nColumns. In OptionsConfigurationBlock, all the add*() methods have a hardcoded grid width of 3. It doesn't make sense to pass nColumns around in some places but not everywhere, and the same 'n' is used in different places. - In OptionsConfigurationBlock, I would make fields final if possible (use Clean Up). > > - In FilteredPreferenceTree, only offer add*() methods that are tested. I'm > > pretty sure addCheckBoxWithLink(..) wouldn't work. > *hmm..I can do that but what is wrong with addCheckBoxWithLink(..)? Are you > referring to the the fact that a Button is not returned from this method? But > it can be easily obtained by using OptionsConfigurationBlock#getCheckBox(Key). > This would be similar to what I am doing for ExpandableComposite. addCheckBoxWithLink(..) not only creates a Button (checkbox), but it also creates an intermediate Composite and a Link widget. Calling setVisible(false) on the Button will not hide the whole thing. => I didn't test this, and I guess you also didn't, so please remove the methods that are not used. Unused/untested code bloats the codebase and is a maintenance issue. These methods can easily be added later if necessary (probably with a separate fControlType constant).
Released to HEAD.
Reverted the change for JavaBuildConfigurationBlock.java, that was implemented only for testing purpose (and was not perfect), moreover that page does not have enough preferences to justify filtering.
