Community
Participate
Working Groups
If 'USE_COLOR_ICONS' is enabled (the default) ActionContributionItem uses the HoverImageDescriptor as the default icon for toolbar items and ignores the image itself (so no hover effect is visible at all) but the hover image is always shown. The "disabled" state is only represented by an explicitly set disabled image. Even more strange, if one uses ActionContributionItem.setUseColorIconsInToolbars(false); then it works in an even more confusing way: 1) If there is a regular image but no hover image, the regular image becomes the hover image and the default image gets converted to grayscale (and the disabled state becomes even more hard to recognize). 2) if there is both a default AND a hover image then no conversion to grayscale is performed at all and the toolbar indeed HAS colors and a hover effect as one would expects. I can't really find any hint in the javadoc for IAction or ActionContributionItem that explains this behaviour, at least its extremely confusion. I think the whole code can be simplified and work more like users would expect the following way: - throw away the if (USE_COLOR_ICONS) branch and simply fetch all image descriptors as-is - if hover image is null assign it the default image and vice versa - if disabled image is null use a gray scaled version of the default image - if USE_COLOR_ICONS is false, convert the default image to grayscale - at the end simply set the images to the tool item
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170827
As Andrey has pointed out in the review, there might be some investigation/discussion needed before doing actual change here, thus I adjusted the topic and will try to explain the problem in more detail. == What do I want to archive == Having a button in a Toolbar with a default image, the image should change if user hovers the button. IAction.setHoverImageDescriptor(ImageDescriptor) javadoc states: > Hover images will be used on platforms that support changing > the image when the user hovers over the item. sounds quite obvious to use this method on the supplied action to full-fill the desired task. == What I would expect to happen == The button in the toolbar now shows the default image and (if supported by the platform, at least on Linux it seems the SWT.FLAT style must be set to enable hover-images) a different image when I put the mouse cursor over the button. == What actually happens == The button is displayed with the hover image, the default image is completely ignore, the image does not change when I hover the button. == Does a workaround exits? == No, but one can show that hover actually works creating a Toolbar manually. == What else strange can happen == If one disables the global Flag > ActionContributionItem.setUseColorIconsInToolbars(false); there are the following cases: 1) no hover image specified -> the default image is used in gray and a hover changes it to a color 2) no default image -> the hover image is used as default as with case 1 2) hover AND default image is specified -> the default image is used (but not grayed!) and on hover the hover image is shown
Created attachment 284479 [details] Example Project The Example Project shows the problem-as-code to play with different settings.
Created attachment 284480 [details] How it is displayed in gtk3 Screenshot how it looks like in Linux/GTK3
Created attachment 284481 [details] Same on Windows And a screenshot from Windows, same behavior but windows display hover images regardless of FLAT style.
== What are the proposed changes == I would propose the following changes to make thinks more consistent with the API and even better maintainable: 1) fetch all three imagedescriptors from the action (default, hover, disabled) 2) if default *and* hover is null and forceImage is true use the MissingImageDescriptor 3) If default is null but hover is given use the hover image as the default 4) If hover is null but default is given use default as the hover image 5) If disabled image is null, generate a grayed variant out of the current default image 6) If USE_COLOR_ICONS is false, generate a grayed variant out of the current default image 7) set all images (if present) into the Toolitem The described flow can be seen in the gerrit [1]. == What would be the consequences of the change == It might be possible (if hover images where used) that a different image is shown, code that previously has lacked hover effects will now show (correct) hover effects. Only code that previously has relied on that the hover-image take precedence over the normal one would see the effect, even though I don't know what the use-case for this would be. Icons previously showed as colorful if both descriptors where set will now display correctly as grayed with a color hover effect if > ActionContributionItem.setUseColorIconsInToolbars(false); is used. [1] https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170827/1/bundles/org.eclipse.jface/src/org/eclipse/jface/action/ActionContributionItem.java
I have now checked several references in the platform itself and there are some places where only a hover image is declared, and some where different images are referenced but in the end mapping to the same filename. In some places even the same descriptor is set for the default and the hover image. I can't find any reason there why the current behavior was designed like this, so it seems it just got unnoticed because using (different) images for hover is uncommon in eclipse-platform. So any feedback or suggestion how we can proceed here is more than welcome.
I can reproduce the described behavior with the given snippet and agree that the behavior is confusing. It is indeed a bug that the hover image is shown when the mouse isn't hovering over the toolbar item. I'll add some findings to the Gerrit change.
(In reply to Christoph Laeubrich from comment #6) > 3) If default is null but hover is given use the hover image as the default I prefer to show missing image in this case. Showing hover image may hinder the developer to recognize that an image is missing. It is clearly a programming mistake if the default image is lacking and the hover image is available. I agree with the other cases. It is a nice fix.
(In reply to Wim Jongman from comment #9) > (In reply to Christoph Laeubrich from comment #6) > > 3) If default is null but hover is given use the hover image as the default > > I prefer to show missing image in this case. I no one disagrees here I can adjust the code, it just seems to be a special case for the grayscale mode, where one can set the hover-image only and a default one in grayscale is automatically generated as default. This might also be the origin for the "color-mode", I just have never heared/used the gray-mode so I can't really tell how usefull/widly used it is.
Created attachment 284504 [details] IDE toolbar Here is an example how this looks for the IDE toolbar. Some icons remain colored, since the hover image does not respect the setting ATM. With the current state of the patch the toolbar icons become grayscale and when hovering over them they become colored. I think this is also a flaw. The hover icons should be also be gray IMO, but maybe with a different background or grayscale. However, the screenshot shows that the current behavior is wrong.
(In reply to Karsten Thoms from comment #11) > toolbar icons become grayscale and when > hovering over them they become colored. > I think this is also a flaw. I think that was the original idea, so you can see what is hovered without providing an explicit hovering icon. > hover icons should be also be gray IMO, but maybe with a different > background or grayscale. The problem here is that "background" is a bit OS-specific and there is no easy way to generate with different grayscale.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170827 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9cc8c9a465eab62070dbfa63c62337c88c501a80
Thanks, Cristoph!
The JFace test added here fails both on Windows and Mac. Please fix the test so that it passes on both Mac and Windows too.
(In reply to Kalyan Prasad Tatavarthi from comment #15) > The JFace test added here fails both on Windows and Mac. Please fix the test > so that it passes on both Mac and Windows too. testActionImagesAreSet and testDefaultImageIsGray both tests fail on Mac and Windows in the nightly builds.
(In reply to Kalyan Prasad Tatavarthi from comment #16) > testActionImagesAreSet and testDefaultImageIsGray both tests fail on Mac and > Windows in the nightly builds. Can you provide a link to the failing build? Can I trigger a gerrit build on a Windows or Mac node to verify?
(In reply to Christoph Laeubrich from comment #17) > (In reply to Kalyan Prasad Tatavarthi from comment #16) > > testActionImagesAreSet and testDefaultImageIsGray both tests fail on Mac and > > Windows in the nightly builds. > > Can you provide a link to the failing build? Can I trigger a gerrit build on > a Windows or Mac node to verify? Please find below the links for test failures in the IBuild I20201028-1800 https://download.eclipse.org/eclipse/downloads/drops4/I20201028-1800/testresults/html/org.eclipse.jface.tests_ep418I-unit-mac64-java11_macosx.cocoa.x86_64_11.html https://download.eclipse.org/eclipse/downloads/drops4/I20201028-1800/testresults/html/org.eclipse.jface.tests_ep418I-unit-win32-java11_win32.win32.x86_64_11.html
It seems my native approach of comparing ImageData fails here because not the original ImageData is returned but a new one is constructed that has different properties (e.g. original ImageData has 24bit, but on windows a 32bit one is returned). I'll adjust this, but would need at least a way to trigger a gerrit for each individual platform for verification, anyone know how this can be archived?
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/171494 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2f50cc83ef83a0f9bf0b7b5b40b57cbe578f3bde
I have upgraded my product to 2020-12 and everything works like expected now regarding action icons.
(In reply to Christoph Laeubrich from comment #21) > I have upgraded my product to 2020-12 and everything works like expected now > regarding action icons. Thanks, Cristoph. Should'nt we have an N&N item for this?
(In reply to Eclipse Genie from comment #13) > Gerrit change > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/170827 was merged > to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=9cc8c9a465eab62070dbfa63c62337c88c501a80 That caused severe regression on Linux, see bug 569750.
