Allow to ignore explicitly defined disabled icons#4002
Conversation
29a2bec to
b8ed790
Compare
Test Results 864 files ±0 864 suites ±0 51m 29s ⏱️ + 1m 20s For more details on these failures, see this check. Results for commit a48c2e5. ± Comparison against base commit 6c53fec. ♻️ This comment has been updated with latest results. |
HannesWell
left a comment
There was a problem hiding this comment.
Thanks for this. I think this totally makes sense and helps to fade out explicit/dedicated disabled icons.
I just have one suggestions for their label/description.
| ViewsPreference_ignoreDisabledIcons = Ignore pre-generated disabled icons | ||
| ViewsPreference_ignoreDisabledIcons_tooltip = When enabled ignores pre-generated disabled icons in favor of generating consistently styled disabled icons on the fly |
There was a problem hiding this comment.
What about labling these icons as explicit disabled icons or dedicated disabled icons?
| ViewsPreference_ignoreDisabledIcons = Ignore pre-generated disabled icons | |
| ViewsPreference_ignoreDisabledIcons_tooltip = When enabled ignores pre-generated disabled icons in favor of generating consistently styled disabled icons on the fly | |
| ViewsPreference_ignoreDisabledIcons = Ignore explicit disabled icons | |
| ViewsPreference_ignoreDisabledIcons_tooltip = When enabled ignores explicit disabled icons in favor of generating consistently styled disabled icons on the fly |
But I'm not sure if explicit could be read with other meaning.
There was a problem hiding this comment.
Regarding the suggestion: maybe we should clarify with a native English speaker if it must be named "explicitly disabled".
There was a problem hiding this comment.
But I'm not sure if
explicitcould be read with other meaning.
That's why I tried to avoid the term in user-facing descriptions. I am sure I would not have understood the option when using that word if I was not involved in the development.
maybe we should clarify with a native English speaker if it must be named "explicitly disabled"
I think that's a good example, why we should avoid the term "explicit", because "explicitly disabled" would be incorrect in terms of semantics.
I have no really good proposal for an alternative either. Just though about something like "manually generated".
|
For me a radio button style option would be more appropriate than a checkbox ... the current terms seem to technical for anyone to understand that is not familiar with the technical details. Something similar to:
(where the last option would e.g. use the "native" disabled image appearance if we want to offer such option) |
I agree that the description is currently too technical. That's something we need to solve by proper naming. A radio button alone will not solve that, in particular because there are only the two options "always generate" or "consider pre-generated/explicit/manually generated/...". "never" is not a possible option without starting a new feature for supporting that option. Maybe just taking your proposed text for the radio buttons would be fine as something like: |
|
I don't thing "generating" is a good therm at all, but the whole preference page lacks a bit the user perspective - I must confess most of this is already unclear what it really does and this pseudo headlines are really horrible. So first think I would ask is does it really only affect actions (or is it more buttons that trigger an action?) - and only toolbar or also menus? Second I think, that it is more that we derive a disabled icon from the original one. So in the end I would possibly still be better to have a radio option to make clear what are the choices here. One might also ask if this should really be a user setable option (e.g. how likely is it a user will ever change this?) |
|
Maybe even more fundamental question. Should the user decide this at all? I imagine product owners will be better to judge on this front... |
Those are absolutely valid points. In my opinion, the goal should be that the automated mechanism for generating disabled images on the fly is so good / configurable that no one not only needs to but not even wants to provide and maintain custom-generated disabled versions of icons. I see two potential reasons to consider regarind configurability:
While in my opinion (1) needs to be supported, I am not sure if (2) will really be a relevant use case. And with that insights, I agree that providing a configuration via preference may not be the best way of configuration here. Instead, we may simply properly share the information about this change via release N&N and provide a fallback via system property. But then we should be consistent and also deprecate all API for defining disabled icons (like was done for hover icons). |
65387e8 to
bf2f122
Compare
|
I have now simplified the change to not introduce any preference but only a system property to re-enable usage of disabled icons. I hope that resolves the raised concerns. |
HannesWell
left a comment
There was a problem hiding this comment.
Maybe even more fundamental question. Should the user decide this at all? I imagine product owners will be better to judge on this front...
That's indeed a good question and I share your opinion respectively what Heiko answered to it.
With that I'm again fine with this, just have two minor suggestions/questions below.
| public static void setUseDisabledIcons(boolean useDisabledIcons) { | ||
| USE_DISABLED_ICONS = useDisabledIcons; | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this just for testing or should this also be used by down-stream consumers?
There was a problem hiding this comment.
Currently it's only used for testing, but it may also validly be used to programmatically configure the behavior instead of using the system property. I would be fine with only making it available for tests but properly doing it would not be that easy with our test setup. We could mark it as @noreference though. What do you think?
There was a problem hiding this comment.
Currently it's only used for testing, but it may also validly be used to programmatically configure the behavior instead of using the system property.
If we definitively want to support that, then I think it makes sense to have this public. But if we don't or are not sure, then I suggest to not make it an 'API' and only elevate it when desired/necessary. I think it just should be a clear decision, but I'm not for one or the other.
doing it would not be that easy with our test setup. We could mark it as
@noreferencethough.
Yes, in that case it we could do that or make it (package) private and just call it reflectively or set the field reflectively. But that's of course not very elegant.
There was a problem hiding this comment.
I've updated the PR with the added @noreference annotation in Javadoc. Reflective access would be rather ugly and doing it properly (making the tests actual unit tests via test fragment or test integrated into the production bundle or putting the setter into some internal package with the tests defined as x-friends) is, in my opinion, not worth the effort. Then you could rather think about if it's worth to keep the part of the test case for disabled images at all.
05918b7 to
a95d7b4
Compare
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
|
When I as a developer set a disabled icon, I have a reason for doing so, and I want that icon to be used. Period. I don't see the need to remove this functionality at all. |
The main reasons for doing that so far was that disabled icons could not be properly generated on the fly. That has changed with recent improvements to SWT, as now on-the-fly generation is sufficient for generating consistent results. Do you have uses cases where you explicitly want to provide disabled icons that derivate from what the on-the-fly generation delivers? |
|
The only potential problem I see is if someone deliberately sets the widget to some entirely different image - it's all hypothetical (as I don't have such case) but maybe it's worth thinking/searching of such cases. |
That's correct. I have listed the types of cases I see in this comment already: #4002 (comment) Also note that there is no plan to remove the functionality overall, it's just that by default explicitly provided disabled icons will be ignored. You can still use the provided system property to keep using such icons. Without this, we will never be able to change (or rather allow to change) the overall appearance of disabled icons because there may be extensions that still deploy such icons which will then not fit to the other icons anymore. In addition, providing an entirely different image as disabled image is, to put it mildly, a questionable idea in terms of UX. |
The algorithm for generating disabled icons on-the-fly has recently been enhanced. Most custom disabled icons that have been embedded into bundles conform to what now can be generated on-the-fly. In addition, the algorithm is interchangeable, allowing custom stylings of disabled icons. However, when there are still bundles, such as extensions out of own control, that still contain custom disabled icons, exchanging the algorithm for disabled icons will lead to inconsistent appearance as only the on-the-fly generated icons will adhere to that. This change allows to ignore custom disabled icons, such that even if some bundles defined disabled icons, they will be ignored and on-the-fly generated disabled icons according to the selected algorithm will be used instead. This is enabled by default and can be configured via system property.
87586aa to
a48c2e5
Compare
Yes. Consider command contributions to a toolbar in a view. If the handler is set to be active only when the view is active (to avoid handler conflicts), you may get the strange situation that the button shows disabled, but when clicked performs the command anyway since on click the view is first activated and then the button receives the click event. Case is question is the refresh button in the Git Repositories view. It has a disabledIcon that is the same as the enabled icon (i.e., not a grayscale version). However, I just noticed that this doesn't work quite as expected; somehow the platform sometimes still shows only a faded version of that icon. We hit the same case with the toolbar top right in the Commit Editor in EGit (the one that opens when you select a commit in the history view and then choose "Open in Commit Viewer" from the context menu). See for instance commit 5ad9f043 in EGit; the commit message explains the problem. (Though that commit uses a different work-around. But that work-around doesn't work if you populate the toolbar via contributions from plugin.xml.) As a plugin developer I have no control over what system properties might be set when the product using the plugin is started. So system properties are not really a solution. |
|
Thank you for those pointers. That's very interesting input.
That's an interesting case. But it actually rather sounds like a framework bug or a usage mistake. On the framework side, from a UX POV, the click on a disabled button should not work. On the usage side (at the example of the Refresh button in the Git repositories view): isn't the platform's refresh action retargettable, so shouldn't the general refresh action be retargeted when the repositories view is active rather defining another handler for an EGit-specific refresh? I have to admit that I am not completely sure about these mechanisms, but since refresh actions are available at other places as well (e.g., in the "Call Hierarchy View"), I wonder if EGit couldn't solve that in the same way. Is it because the action is not specific to the selected element but refreshes the view (I have to admit that I do not even understand what it is supposed to do)? Then the behavior is different from other places where F5 does a refresh of the currently selected element, so a behavior that may be questioned from UX POV. So in my opinion, solving these issues by overwriting the disabled icon with an enabled one is a quite hacky workaround and not a proper solution to the issue. Also note that the UX of this approach is quite bad, as the button does not give hover feedback (as it is disabled) even though it looks enabled. I remember that I was once confused why some button was shown enabled but provided no feedback when hovering (maybe it was the Refresh button of the Git repositories view) but forgot to follow-up on that. So I think we have some other issues to adress here, but I still do not see a reasonable use case for "overwriting" disabled icons. |
I don't agree for the view toolbar. Maybe you're right for the toolbar inside the commit editor. But a button (or rather, ToolItem) in a view toolbar should not require two clicks to work (first click to activate the view, second click to execute the command). OTOH perhaps it's just not doable with a command contribution. |
But then the button should actually be enabled and not just partially mimic a enabled look while actually being disabled and only being enabled when clicking. Please consider my notes on the degraded UX with that approach as well. I.e., the framework would have to show that button as enabled even though the used handler is disabled when the view is not active. So what is the proposal to proceed here? I don't expect that anyone disagrees that the cases mentioned so far are misusages of custom disabled icons, so I am not sure if that should block further progress. At least there was no disgreement to my last post on that. |
|
It will break the use case in the EGit "Git Repositories" view. I don't understand why one would want to disable this feature at all. If the current disabled icons that just use a grayscale image are not good enough, one could simply remove these icons and get the auto-generated behavior. But if some plugin does set a disabled icon, why have extra code to ignore that? As I wrote above, the plugin developer had a reason for setting the icon, and wants that icon to be shown. Incidentally I notice that #3760 got exactly one comment, and that one goes in the same direction. A system property is beyond the control of plugin developers and with the default of BTW, I wrote:
On Mac. On Windows it works as expected. |
|
I agree with @HeikoKlare that showing a disabled button as enabled because it actually is not disabled sounds like a bug that must be fixed (independently). I also agree with @akurtakov and @iloveeclipse case that one might define a disabled icon that looks completely different but also hardly can find a good example where it would be useful, in contrast for example a toogle-button (where it as far as I know is not supported out of the box). The most limiting factor I see is more that this only ever will work for action contributions, so there might because where people construct on toolbars that then look differently, so consequently this more has to be placed at the JFace level or even SWT level to be really consistent. |
|
Tbh, I don't quite understand the "showing a disabled button as enabled because it actually is not disabled" so I would appreciate a pure SWT snippet showing what is meant by that. |
As this is a jface feature (or even platform.ui) I don't think that is really possible, SWT does has no notation of "disabled" image for buttons. A button can be enabled/disabled but this has no effect on the icon as otherwise one can not set an own icon anyways. |
|
Of course we've digressed from the topic of the title, But to me also it seems that some JFace problem related to the proper initial state of a tool button is a separate problem that should be addressed separately... |
To clarify this again: this contributes to the continuous efforts of enabling enhanced look and feel for Eclipse products. Without this change you are unable to properly exchange the algorithm for generating disabled icons (and potentially even exchanging icon themes #2870).
As explained above, the common reason so far was that it was not possible to generate disabled icons on-the-fly, which is why every icon had an explicit disabled verson of itself. The developer would have been totally happy if that icon was generated on-the-fly instead of investing manual effort to generate and set it (which is now possible). So to repeat my previous question: how should we proceed? Are there any concerns regarding fixing the actual underlying root causes for the reported issue(s) and proceeding with this proposal? |
|
I don't see this as a work-around. But if the consensus here is that disabledIcons are not to be used, then implement that: rip out/deprecate/make no-ops anything related to disabled icons (except the auto-generation). Just drop the feature instead of adding code to make it "configurable". |
This change allows to ignore explicitly defined disabled icons (as SWT provides a proper way to do it on-the-fly since 2025-06) and enabled that behavior by default.
This is an extract of:
In that PR, no objections have been raised on always using the on-the-fly generation of disabled icons. I thus intend to merge this at the beginning of 4.41 development in case no objections are raised until then.
@HannesWell we had discussed the extraction of this rather non-controversial change offline. Maybe you want to have a quick a look.
Description
The algorithm for generating disabled icons on-the-fly has recently been enhanced. Most explicit disabled icons that have been embedded into bundles conform to what now can be generated on-the-fly. In addition, the algorithm is interchangeable, allowing custom stylings of disabled icons. However, when there are still bundles, such as extensions out of our control, that still explicitly define disabled icons, exchanging the algorithm for disabled icons will lead to inconsistent appearance as only the on-the-fly generated icons will adhere to that.
This change allows to ignore explicitly defined disabled icons, such that even if some bundles defined disabled icons, they will be ignored and on-the-fly generated disabled icons according to the selected algorithm will be used instead. An according preference that can be configured via the appearance tab is added.
How to test
The enhancement is configurable via preference, thus can be tested by enabling/disabling it:

As an example, this is what the main toolbar looks like with different configurations:
Existing
No pregenerated disabled icons