Community
Participate
Working Groups
The bluish gradient of the console does not comply to any Eclipse views look and feel. Users also don't like it. Therefore this feature should be probably dropped
Created attachment 209145 [details] Patch to remove gradient This patch adds a minimal flag to turn gradient on/off and default to off.
Hi Brian, Thanks for the proposing the patch! I have some considerations about it: Introducing a private final boolean flag to indicate whether to draw the gradient or not and initializing it with "false" effectively introduces dead code. This is not a good thing and I would rather prefer deleting it than leaving it dead. On the other hand, as per our SAP eclipse based developer studio usability experts requirements, it should be possible to turn the gradient on. Therefore I think that it makes sense to introduce some kind of a preference to turn gradient on/off. Going a step further, the preference property could be initialized by the branding plugin so that the gradient initial state (on or off) could be preconfigured depending on the distribution Getting into more desing details I am thinking about getting the gradient related implementation out of the SearchConsoleView class and moving it to a dedicated "decoration" utility. A factory could instantiate proper decoration implementation based on the preferences value and apply its decoration to the search console/search favorites view. It might be necessary search console and search favorites view to implement a special interface (e.g. "IGradientable" :) ) so that the decoration utility to be able to apply its decoration implementation. What do you think? Thanks and regards, Danail
Created attachment 209465 [details] Patch to add prefs page and gradient preference This patch adds a new preference page that is queried when the view is created. I think it's definitely a better approach.
I think that a dedicated "decoration" utility is probably a good idea to do down the line, but at this point with just the two views it might be overkill unless you want users to change the gradient color, style, or whatnot.
By the way, the preference page shows up in the General->Search Console path in the Preference dialog. I figure if the Search prefs page gets priority, so should the Search Console.
Hi Brian, >I think that a dedicated "decoration" utility is probably a good idea to do >down the line, but at this point with just the two views it might be overkill >unless you want users to change the gradient color, style, or whatnot. Yes, you are probably right. >By the way, the preference page shows up in the General->Search Console path in >the Preference dialog. I figure if the Search prefs page gets priority, so >should the Search Console. I agree The main idea of the search console implementation is to consist of classes which are highly decoupled and have a clear separation between "controller" and "view" Putting the preference values implementation directly into the view classes (SearchConsoleView and SearchFavoritesView) as your patch suggests, has the consequences that - they gain additional business logic which is not pure UI related - increases the complexity of the implementation which makes the code less readable In order to keep the view-controller separation I would suggest the interfaces via which controllers communicates with views (ISearchConsoleControllerOutputView/ISearchFavoritesControllerOutputView) to get a parent interface with a method enableGradient(boolean) On the other hand the controller should observe changes in the preferences and call the new method with appropriate argument at the appropriate time (view opens, preferences change). The corresponding implementation in the view implementation would react accordingly via registering gradient listeners. In order to keep classes small and readable I would suggest putting the preference observation implementation in separate classes referenced by controller. The same applies for the implementation which adds the gradient listeners to the composites of the view. And another thing - the search console has been implemented in a test-driven development mode which means that each feature has automated tests with high code coverage. We want to keep this in future. Therefore whenever implementing a change please do also provide automated tests Thanks! Danail
This must not surface as a preference in the UI but rather configured by the product itself (if really needed).
Ok, so before I do further coding and have it shot down, here's what I'm hearing... The main problem is that the Eclipse views for the Search Console both have a bluish gradient and Eclipse (i.e. Dani) don't want the gradient. The solution that's being proposed by Danail is: * Add a parent interface - ISearchConsoleViewWithGradient, which has a method enableGradient (boolean) * Extend ISearchConsoleViewWithGradient from ISearchConsoleControllerOutputView and ISearchFavoritesControllerOutputView * Implement enableGradient(boolean) in the two view classes and default it to false I'm fine with this. Where there seems to be some disagreement is whether the gradient setting should be visible to the user. Dani is suggesting that there's a command-line preference that could be set to override the gradient setting and thus show the gradient. The preference could then be initialized to something other than false by passing the -pluginCustomization command-line argument to the eclipse command with an appropriate preference ini file. So before I do anything else, is this approach ok with everybody?
(In reply to comment #8) > So before I do anything else, is this approach ok with everybody? For me yes. Note that you don't need a separate command line argument. "pluginCustomization" is used to set preferences, hence you only need to add a preference (key) for the gradient setting.
Hi Brian, Dani, Just for the sake of having the information here, I found an article explaining how plugin customization works: http://www.eclipse.org/articles/Article-Branding/branding-your-application.html The proposal to not let the user decide whether to enable/disable gradient probably makes sense. In this case, having an argument "enable" to an "enableGradient" method probably makes no sense since one have to implement additional logic to handle the "false" case value which in the end would be dead code. I had a look at the current implementation and I think that we don't have to introduce new methods or interface in the very view. Here are the details: When the search console/favorites view starts, the integration plugin (org.eclipse.platform.discovery.integration.internal.plugin.DiscoveryIntegrationPlugin) takes care of instantiating a controller, configure the view and bind things together. Part of the view configuration occurs in the org.eclipse.platform.discovery.integration.internal.plugin.DiscoveryIntegrationPlugin.customizeView() method where the view gets a org.eclipse.platform.discovery.ui.api.IViewUiContext instance set. Now, if we extend the IViewUiContext interface to have a method like "boolean isGradientEnabled()", then the view would know that the gradient should be turned on. The implementation which enables gradient for composite and controls can be extracted to a designated class (something like "UiConfigurator"), whith method configure(Control/Composite, IViewUiContext). The implementation of this method could check whether the gradient is enabled (asking the IViewUiContext instance) and if yes, do the needed to get the gradient enabled. How does this sound to you? Dimitar, Martina: Do you have some thoughts on the topic? Thanks! Regards, Danail
So the approach we seem to be settling on is: * Add a "gradient" preference that defaults to false and can be enabled/disabled via -pluginCustomization at the Eclipse command line * Add "boolean isGradientEnabled()" to IViewUIContext * Alter "private IViewUIContext createViewUIContext()" in DiscoveryIntegrationPlugin to poll the preferences and return true/false based on the preference setting * Add "public IViewUiContent getUiContext()" to ICustomizableView (no way currently to get the view context from the view that I can find so it can be passed to the utility class below) * Create "UIConfigurator" utility class with method "configure(Control/Composite, IViewUIContext)" * Pass each control and composite to UIConfigurator.configure() as it's created in the view Though I understand adding the utility class might enable further customizations going forward, right now it's going to largely be an unnecessary step and add some (albeit minimal) overhead. Do we really need to go that far? Are we really expecting users to implement their own search and search favorites views?
Hi Brian, >* Create "UIConfigurator" utility class with method >"configure(Control/Composite, IViewUIContext)" I am wondering whether UIConfigurator sounds too general. How does "GradientDecoration" sounds like? I am really having hard time to think of a suitable name... >* Add "public IViewUiContent getUiContext()" to ICustomizableView (no way >currently to get the view context from the view that I can find so it can be >passed to the utility class below) How about the very view class to do what is necessary on setting the IViewUIContext instance? I imagine the implementation to go like that 1. In AbstractDiscoveryView: protected abstract void decorate(GradientDecoration decoration); AbstractDiscoveryView.setUiContext(IViewUiContext uiContext) { this.uiContext = uiContext; decorate(new GradientDecoration(uiContext)); } 2. In the concrete extenders (SearchConsoleView and SearchFavoritesView) the implementation of the decorate method would pass the controls which need gradient to the decoration instance >Though I understand adding the utility class might enable further >customizations going forward, right now it's going to largely be an unnecessary >step and add some (albeit minimal) overhead. Do we really need to go that far? The benefits of getting the gradient stuff out of the view classes are 1. Reduce the size of the the classes. They are pretty big already 2. Come closer to the "single responsibility" principle via moving the gradient management to another class 3. The code for managing gradients would become unit-testable I know that these might look like minor things but a great stuff always consists of minor pieces :) >Are we really expecting users to implement their own search and search >favorites views? It is possible. Although this might sound like science fiction at the moment, I have talked with some SAP colleagues who are kind of interested in embedding the search console in a wizard flow. Having the clear separation between UI, controller and integration layer, such a feature is, I believe, very possible. In the concrete example, the integration layer would be the wizard itself. Regards, Danail
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it and remove the stalebug whiteboard tag. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. --
