Community
Participate
Working Groups
Build Id: Eclipse 3.6m4 The StringVariableSelectionDialog, which typically is behind a "Variables..." button in a number of places, currently displays all defined Eclipse variables. This is problematic, because many variables are valid in certain contexts only. The ${build_project} variable, for instance, is only valid in context of external tool builders. End users who are prompted to pick a variable from the StringVariableSelectionDialog get the impression that they can use all the variables -- and once they picked one that fails to work in their context, they do not understand why it fails to work. Proposed solutions: ------------------- 1.) In the simplest case, the StringVariableSelectionDialog should allow product builders to explicitly hide variables which are known to fail in the context where the "Variables..." button is used. For instance: svsd = new StringVariableSelectionDialog(parent); svsd.hide(new String[] { "build_project" }); int code = dialog.open(); This simple improvement would already allow product builders to get it right in a "closed" environment for the occurrences of a "Variables..." button that they can control. 2.) For more advanced and configurable environments, the org.eclipse.core.variables.dynamicVariables extension point should add an attribute that allows variable providers define a list of String context name in which the variable can be resolved. Variables without context name are expected to be universally available (to provide backward compatibility). The StringVariableSelectionDialog can then support filtering by a list of context names that are known to work in the concrete situation.
FYI, bug 300989 talks about the concrete scenario of ${build_project} failing when used in the context of Launches for instance.
This is actually an enhancement request, and not a bug... I'm wondering if anybody could give an initial comment on the idea? Again, the lack of such a filtering capability leads to UI dialogs which offer variables to choose that cannot work in a context. This leads to bad usability, which makes us avoid the variables dialog in our product (and just document what variables are known in particular context). Even a very simple filtering would be sufficient for our needs: - pass in a list of String ID's for variables that are known to be supported - Have a checkbox for "show all" in case a user needs one that we don't think is supported (similar to Capabilities). I could contribute a simple patch if Debug is interested.
(In reply to comment #0) > Proposed solutions: > ------------------- > 1.) In the simplest case, the StringVariableSelectionDialog should allow > product builders to explicitly hide variables which are known to fail > in the context where the "Variables..." button is used. For instance: > svsd = new StringVariableSelectionDialog(parent); > svsd.hide(new String[] { "build_project" }); > int code = dialog.open(); > This simple improvement would already allow product builders to get it > right in a "closed" environment for the occurrences of a "Variables..." > button that they can control. Note that an API already exists to limit the displayed variables to a specific set. For example, one can use org.eclipse.ui.dialogs.ElementListSelectionDialog.setElements(Object[]). The objects passed in are the variables themselves. By default, all are displayed. However, when this API is used, there is no indication to the user that the displayed variables have been limited, and there is no way to show all. But, this API could be used to limit the set of displayed variables. > 2.) For more advanced and configurable environments, the > org.eclipse.core.variables.dynamicVariables > extension point should add an attribute that allows variable providers > define a list of String context name in which the variable can be > resolved. Variables without context name are expected to be universally > available (to provide backward compatibility). The > StringVariableSelectionDialog can then support filtering by a list of > context names that are known to work in the concrete situation. This sounds reasonable. >Even a very simple filtering would be sufficient for our needs: > - pass in a list of String ID's for variables that are known to be supported > - Have a checkbox for "show all" in case a user needs one that we don't think > is supported (similar to Capabilities). For this, I don't think any new API is required. Simply use the existing #setElements(...), and the dialog could provide a "show all" check box when it determines some variables are missing. For contexts, new API is required, but it would be fairly simple. We can review any contributions, noting the API freeze is M6 (March 12th). The real deadline is the week before the milestone in order to itegrate/reveiw for the test pass.
(In reply to comment #3) > set. For example, one can use > org.eclipse.ui.dialogs.ElementListSelectionDialog.setElements(Object[]). > The objects passed in are the variables themselves. Thanks for the pointer (I missed the #setElements() method since it's inherited from the base class). We tried going with this, but ran into an issue: In the dialog, the "Edit" button allows adding String variables, and after adding a new variable the filter applied by #setElements() is gone. And, if implementation were changed to keep the filtered element list even after an edit, we might run into a situation where user adds a new variable but after pressing OK that variable is not visible and thus not selectable. Since String Variables are always static, I see no point in restricting String variables anyways; instead, I propose adding a callback API for filtering: class VariableFilter { /** Return true to hide given variable */ public boolean isFiltered(IDynamicVariable var) { return false; } }; /** Set given variable filter. Pass null to reset. */ public void setVariableFilter(VariableFilter filt) This allows clients to add any kind of filtering on dynamic variables. The default implementation could filter by Capabilities, for instance. A "Show All" checkbox could be added when a filter has been provided and it has been determined that some variable was hidden. We can provide a patch along these lines if the general idea is approved. > For contexts, new API is required, but it would be fairly simple. What about this, for instance: /** return context name for variable if known, or null if unknown */ String IVariableManager#getContextFor(IStringVariable var); public final static String CONTEXT_UI_PROMPT = "ui_prompt"; public final static String CONTEXT_SELECTION = "selection"; public final static String CONTEXT_BUILDER = "builder"; public final static String CONTEXT_EXTERNAL_TOOL_BUILD = "ext_builder"; public final static String CONTEXT_GLOBAL = "global"; This API could be leveraged by the variable filters mentioned above. Now that's not bad and does provide some added value for filtering (since known context sets are extendable), but it's still possible to construct cases that are not represented really well. In our experience, the ${project_loc} variable has led to most confusion, since it evaluates the "selection" context, while people expect it to work in the context of any builder (ie they expect the "selection" to be the resource being built). Plus, with an argument the variable is universally valid. Perhaps it would lead to better usability if the client calling a variable resolver could "inject" a selection that it considers proper. Thoughts & comments?
(In reply to comment #4) > Since String Variables are always static, I see no point in restricting String > variables anyways; instead, I propose adding a callback API for filtering: > class VariableFilter { > /** Return true to hide given variable */ > public boolean isFiltered(IDynamicVariable var) { > return false; > } > }; > /** Set given variable filter. Pass null to reset. */ > public void setVariableFilter(VariableFilter filt) > This allows clients to add any kind of filtering on dynamic variables. The > default implementation could filter by Capabilities, for instance. A "Show All" > checkbox could be added when a filter has been provided and it has been > determined that some variable was hidden. Even though value variables often don't need filtering, we might as well be flexible, i.e. allow to filter any/all variables, not just dynamic ones. class VariableFilter { /** Return true to hide given variable */ public boolean isFiltered(IStringVariable var) { return false; } }; Perhaps the "context" issue is more complicated than I originally thought. I think we should open a new bug for that, so the filtering API can proceed. I envisioned that each extension would provide one or more optional "context" attributes, defining the contexts it knows about. Then IStringVariable would have a method like: /** * Returns the contexts defined for this variable or <code>null</code> * if none. * * @return known contexts or <code>null</code> */ public String[] getContexts(); That part is simple. The more tricky part is defining a set of known contexts in the platform, and what they mean. Perhaps these are really properties or attributes rather than contexts... it looks like you also want to encapsulate other behavioral information like "does the variable cause a prompt in the UI thread"?
(In reply to comment #5) > Even though value variables often don't need filtering, we might as well be > flexible, i.e. allow to filter any/all variables, not just dynamic ones. I tend to disagree, since filtering value variables causes the issue that a user might press the "Edit" button to add a String Variable for convenience, and when returning into the dialog it is not visible. String variables are always context-less, and they are typically added by the User (not the system!) in the Preference page, so they should be honored. I really don't see any value in filtering String variables, I only see problems, so I'd vouch for not providing such a filter (thus also avoiding ugly instanceof code in the filter). If it should ever be needed in the future, the baseclass VariableFilter can be enhanced by adding an isFilteredStringVariable() method with default implementation, in an API compatible manner. > envisioned that each extension would provide one or more optional "context" > attributes, defining the contexts it knows about. I was thinking about the context as a means of grouping dynamic variables with similar behavior, such that I can filter-out all variables of a group in one step if I know that particular behavior is not acceptable for me. Note that this context could also be interesting for the end-user and displayed in the variable selection dialog in a separate column or as part of the description ("where does this variable get its data from? What needs to be known for this to work?"). But I personally don't feel very much like pursuing that contexts idea, so for now I'm also fine with ditching it, but others may feel free to follow up in a new bug. > Then IStringVariable would have a method like: Oh right, since it's @noimplement we could add the method and it would be more convenient than going via the IVariableManager. Anyways the more interesting piece of the API would be the additional slots in the extension point schema, which should provide both an internal ID as well as a translatable label for the contexts.
(In reply to comment #6) > I really don't see any value in filtering String variables, I only see > problems, so I'd vouch for not providing such a filter (thus also avoiding ugly > instanceof code in the filter). If it should ever be needed in the future, the > baseclass VariableFilter can be enhanced by adding an > isFilteredStringVariable() method with default implementation, in an API > compatible manner. True, I can live with this. Patch for filtering is welcome.
Created attachment 160306 [details] Patch for dynamic variable filtering Attached is a first patch for review. A variable filter can only be set before the dialog is opened; The "Show All" checkbox is only rendered when a filter has been set, this keeps UI changes restricted to clients leveraging the new functionality. When filtering is on, I somewhat dislike the position of the "Show All" checkbox as well as its interaction with the dynamic text filter on top (when I text filter for "build" but then select the "Show All" checkbox, would I expect my text filter to go away?) I'm also wondering whether there should be some indication in the listbox when filtering is on, e.g. a dummy entry like "... 5 more (filtered)" I was considering adding a 2nd callback for "filterStringVariable()" right away, but then refrained from it. Note that I'm not the original author but reviewed the contribution and I'm attaching it here on the author's behalf. Comments and ideas are welcome.
Review comments: * Perhaps the API should support > 1 filter via #addFilter(ViewerFilter) or #setFilters(ViewerFilter[]), to allow a client to use multiple filters. * Javadoc for the new class needs to be more verbose/complete - i.e. need to document API method with parameters, and should add typical "Clients may extend this class" description. * When no filter is in use, the alignment of the "Edit Variables..." button appears to be off (will attach screen shot). * 'Show All' button needs an mnemonic. * I would suggest to add a filter summary below the selection list (above the "Show All" button). Something like "(N variables filtered)". * I'm not sure the "Show All" check box should effect the filter text at the top. I don't think the user would expect this - the preferences dialog has a button to clear the filter... but I'm not sure we have a pattern to follow.
Created attachment 160663 [details] screen shot showing alignment problem
(In reply to comment #9) Thanks Darin. I agree with all your comments, except > * Perhaps the API should support > 1 filter via #addFilter(ViewerFilter) or > #setFilters(ViewerFilter[]), to allow a client to use multiple filters. what would be the use-case or value for using multiple filters? This bears the risk of semantic questions (would multiple filter do intersection or Union?). Also, I don't see multiple clients interact with the dialog at the same time so there is no case for an add... method. One way how clients could chain filters today is by delegation: class MyFilter extends VariableFilter { private VariableFilter fChained; public MyFilter(VariableFilter chained) { fChained = chained; } public boolean isFiltered(IDynamicVariable var) { boolean filtered = fChained.isFiltered(var); filtered |= /*my expression for filtering*/ return filtered; } } This pattern would allow the client to decide about union versus intersection. Other builder patterns would be possible too, e.g. class UnionVariableFilter extends VariableFilter { public UnionVariableFilter(VariableFilter a, VariableFilter b) { //....
I think we should be consistent with the notion of filters in JFace viewers that developers will already be familiar with: /** * Adds the given filter to this viewer, and triggers refiltering and * resorting of the elements. If you want to add more than one filter * consider using {@link StructuredViewer#setFilters(ViewerFilter[])}. * * @param filter a viewer filter * @see StructuredViewer#setFilters(ViewerFilter[]) */ public void addFilter(ViewerFilter filter) { org.eclipse.jface.viewers.StructuredViewer.addFilter(ViewerFilter) JFace filtering is a union of filters - i.e. an element is filtered if any filter filters it.
Created attachment 160904 [details] revised patch Reworked the patch according to comment #9. * I would suggest to add a filter summary below the selection list (above the "Show All" button). Something like "(N variables filtered)". Skipped this one. * I'm not sure the "Show All" check box should effect the filter text at the top. I don't think the user would expect this - the preferences dialog has a button to clear the filter... but I'm not sure we have a pattern to follow. The current implementation is to not affect the text filter, so it will apply regardless of the Variable filters.
Created attachment 160922 [details] revised patch After discussion with Martin Oberhuber we came to the conclusion to rename the "Show all" checkbox and name it "Hide non applicable" with inverse behaviour. This way the text filter and the hide button may not be misinterpreted. Additionally the information about the numbers of variables filtered can be skipped easier.
Thanks Hans! I would suggest renaming "fHideNonApplicableButton" --> "fHideFilteredButton". Then we remain in the same language as the rest of the code, while the acutally visible and translated String can change. We might go for "Hide non applicable" or "hide discouraged" or "hide variables unknown to this context" for instance, but the concept of "fHideFiltered" would remain the same. Can you please also rename fShowAllSelected -> fHideNonFiltered (and flip its semantics), because otherwise the code is hard to read and understand. In #setFilters() I would suggest allowing a null argument since that's easier for clients and not more work for us. In line 70, put the Javadocs one line deeper (inside the doc comment). In line 137, there is one space char too much. In line 170, the variable bFiltered seems unnecessary.
Fixed. Released. Verified. I will open a bug to deal with remaining implementation issues. However, we want the API in for M6. I renamed the filter method to #isFiltered(IDynamicVariable), which feels simpler, and is still qualified by the argument type.
Verified.
