Bugzilla – Full Text Bug Listing
|
Description
Philipp Kursawe
Hmmm, the CCI code should be asking for the disabled image using a iconStyle of IMAGE_STYLE_TOOLBAR. if there's only one disabledImage registered for that command then it should have been returned early in getImageDescriptor(final String commandId, final int type, final String style). I'll investigate. PW It's using the correct images ... SWT is not honouring them. PW Created attachment 186167 [details]
SWT Snippet reproducing the problem
Created attachment 186168 [details]
nav images the snippet uses
Created attachment 186169 [details]
Image of the created toolbar
The code looks like:
item1.setImage(nav);
item1.setDisabledImage(nav_dis);
item2.setImage(nav_dis);
item2.setDisabledImage(nav);
item3.setImage(nav);
item3.setDisabledImage(nav_dis);
item3.setEnabled(false);
item4.setImage(nav_dis);
item4.setDisabledImage(nav);
item4.setEnabled(false);
The images I expected were:
nav nav_dis nav_dis nav
PW
My tests were on 3.7 I20110104-0920 PW What platform are testing on ? It worked for me on Windows 7 and XP. The only code change I did was: Image nav = new Image (display, "JavaCode/next_nav.gif"); Image nav_dis = new Image(display, "JavaCode/next_nav_dis.gif"); (removed ImageDescriptor) (In reply to comment #7) > What platform are testing on ? When I ran my tests, it was on linux-gtk: gtk2-2.18.9-4.el6.x86_64 PW Right, ToolItem#setDisabledImage () is not implemented in GTK. Phil, could you run my SWT attachment in your system where you reported the problem? Silenio, if there's already a bug for the SWT/Linux/setDisabledImage(*) problem, please just send this bug back to Platform/UI. PW I could not find another swt bug, so we are keeping this one. This issue is still reproducible. Gerrit change https://git.eclipse.org/r/149861 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=832213bb5bbc69bf725090388a341e391b16f27e (In reply to Eclipse Genie from comment #13) > Gerrit change https://git.eclipse.org/r/149861 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=832213bb5bbc69bf725090388a341e391b16f27e In master now. I'll prepare a N&N entry tomorrow. New Gerrit change created: https://git.eclipse.org/r/149915 Gerrit change https://git.eclipse.org/r/149915 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=514a9397f42ba9b0efc88f391d02880a6a6f23c9 (In reply to Eclipse Genie from comment #16) > Gerrit change https://git.eclipse.org/r/149915 was merged to [master]. > Commit: > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/ > ?id=514a9397f42ba9b0efc88f391d02880a6a6f23c9 N&N entry done. Verified in I20191008-0600. (In reply to Eclipse Genie from comment #13) > Gerrit change https://git.eclipse.org/r/149861 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=832213bb5bbc69bf725090388a341e391b16f27e This one (together with patch for bug 551586) causes regression described in bug 551586 comment 11. I believe I have a patch. New Gerrit change created: https://git.eclipse.org/r/152649 @Eric, I was looking at why the new ToolItem.test_setDisabledImage() test was failing on Mac. The test expects item.getImage() to return the disabled image when item is disabled which doesn't look right. On Windows and Mac: item.getImage() returns the image set by item.setImage() item.getDisabledImage() returns the image set by item.setDisabledImage(). The enabled state of the item doesn't affect the return value of item.setImage(). Essentially, when the item is disabled, the image on the widget needs be updated but Item.image should not be changed to the disabled image. (In reply to Lakshmi Shanmugam from comment #21) > @Eric, > I was looking at why the new ToolItem.test_setDisabledImage() test was > failing on Mac. The test expects item.getImage() to return the disabled > image when item is disabled which doesn't look right. > On Windows and Mac: > item.getImage() returns the image set by item.setImage() > item.getDisabledImage() returns the image set by item.setDisabledImage(). > The enabled state of the item doesn't affect the return value of > item.setImage(). > > Essentially, when the item is disabled, the image on the widget needs be > updated but Item.image should not be changed to the disabled image. Lakshmi, could you please check my patch on Mac: https://git.eclipse.org/r/152649 BTW, Console view shows also the problem: the drop-down button "Display selected console" is also "blank" if disabled (== only one console is shown). > Lakshmi, could you please check my patch on Mac: > https://git.eclipse.org/r/152649 @Andrey, I checked the patch, it still fails on Mac. Item.getImage() is not expected to return the disabled image even if the item is disabled, so the test will not work. > Essentially, when the item is disabled, the image on the widget needs be > updated but Item.image should not be changed to the disabled image. I think modifying the code based on this could fix the problem you mentioned in comment#19. (In reply to Lakshmi Shanmugam from comment #24) > > Lakshmi, could you please check my patch on Mac: > > https://git.eclipse.org/r/152649 > > @Andrey, I checked the patch, it still fails on Mac. Item.getImage() is not > expected to return the disabled image even if the item is disabled, so the > test will not work. Thanks, so it is a different problem as mine. > > Essentially, when the item is disabled, the image on the widget needs be > > updated but Item.image should not be changed to the disabled image. Not sure I can follow. This bug is initially about using "better" disabled images, if one is provided. So you say it is not OK? Is this a test-only issue? But org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_ToolItem.test_setDisabledImage() is already GTK only? > I think modifying the code based on this could fix the problem you mentioned > in comment#19. https://git.eclipse.org/r/152649 fixes my problem already. Gerrit change https://git.eclipse.org/r/152649 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=d55af1c18f895e76223f6eb1d61e49dc8196d582 (In reply to Andrey Loskutov from comment #25) > > > Essentially, when the item is disabled, the image on the widget needs be > > > updated but Item.image should not be changed to the disabled image. > > Not sure I can follow. This bug is initially about using "better" disabled > images, if one is provided. So you say it is not OK? setDisabledImage() shouldn't call setImage() because it modifies Item.image to the disabled image. It should only update the image on ToolItem without modifying the Item.image field. > Is this a test-only issue? But > org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_ToolItem. > test_setDisabledImage() is already GTK only? The test is GTK only as it fails on Windows and Mac. The test incorrectly expects getImage() to return the disabled image when the item is disabled. getImage() always returns the image set by setImage(). > > I think modifying the code based on this could fix the problem you mentioned > > in comment#19. > > https://git.eclipse.org/r/152649 fixes my problem already. I think modifying the code in setDisabledImage() to update the image without calling setImage() will fix the problem. (In reply to Lakshmi Shanmugam from comment #27) > (In reply to Andrey Loskutov from comment #25) > > > > Essentially, when the item is disabled, the image on the widget needs be > > > > updated but Item.image should not be changed to the disabled image. > > > > Not sure I can follow. This bug is initially about using "better" disabled > > images, if one is provided. So you say it is not OK? > > setDisabledImage() shouldn't call setImage() because it modifies Item.image > to the disabled image. It should only update the image on ToolItem without > modifying the Item.image field. OK, thanks, now I see that we have there more then one "enabled" image :-) @Eric, I haven't looked deeper (my problem from comment 19 is fixed via I20191114-1800), but it sounds that the original patch breaks some ToolItem vs Item semantics I do not understand fully yet. Could you please check this? (In reply to Andrey Loskutov from comment #28) > (In reply to Lakshmi Shanmugam from comment #27) > > (In reply to Andrey Loskutov from comment #25) > > > > > Essentially, when the item is disabled, the image on the widget needs be > > > > > updated but Item.image should not be changed to the disabled image. > > > > > > Not sure I can follow. This bug is initially about using "better" disabled > > > images, if one is provided. So you say it is not OK? > > > > setDisabledImage() shouldn't call setImage() because it modifies Item.image > > to the disabled image. It should only update the image on ToolItem without > > modifying the Item.image field. > > OK, thanks, now I see that we have there more then one "enabled" image :-) > > @Eric, I haven't looked deeper (my problem from comment 19 is fixed via > I20191114-1800), but it sounds that the original patch breaks some ToolItem > vs Item semantics I do not understand fully yet. Could you please check this? I'll take a look today. (In reply to Lakshmi Shanmugam from comment #27) > setDisabledImage() shouldn't call setImage() because it modifies Item.image > to the disabled image. It should only update the image on ToolItem without > modifying the Item.image field. Let's say this happens: toolItem.setImage(a) toolItem.setDisabledImage(b) toolItem.setEnabled(false) a client who then calls toolItem.getImage() should expect image "a"? Even if the receiver is displaying image "b"? (In reply to Eric Williams from comment #30) > (In reply to Lakshmi Shanmugam from comment #27) > > setDisabledImage() shouldn't call setImage() because it modifies Item.image > > to the disabled image. It should only update the image on ToolItem without > > modifying the Item.image field. > > > Let's say this happens: > > toolItem.setImage(a) > toolItem.setDisabledImage(b) > toolItem.setEnabled(false) > > a client who then calls toolItem.getImage() should expect image "a"? Even if > the receiver is displaying image "b"? Yes. getImage() should just return the image set by setImage() not the image displayed by the receiver. On Windows and Mac too, it has been implemented this way. (In reply to Lakshmi Shanmugam from comment #31) > (In reply to Eric Williams from comment #30) > > (In reply to Lakshmi Shanmugam from comment #27) > > > setDisabledImage() shouldn't call setImage() because it modifies Item.image > > > to the disabled image. It should only update the image on ToolItem without > > > modifying the Item.image field. > > > > > > Let's say this happens: > > > > toolItem.setImage(a) > > toolItem.setDisabledImage(b) > > toolItem.setEnabled(false) > > > > a client who then calls toolItem.getImage() should expect image "a"? Even if > > the receiver is displaying image "b"? > > Yes. getImage() should just return the image set by setImage() not the image > displayed by the receiver. On Windows and Mac too, it has been implemented > this way. The Javadoc for Item.getImage() reads: "Returns the receiver's image if it has one, or null if it does not." This implies that the image returned by getImage() is the image that is displayed by the receiver, no? Are there other cases in SWT similar to this? It seems incorrect to me that a client should have to check the state of a widget before querying the image it is displaying. (In reply to Eric Williams from comment #32) > > The Javadoc for Item.getImage() reads: > > "Returns the receiver's image if it has one, or null if it does not." > > This implies that the image returned by getImage() is the image that is > displayed by the receiver, no? I agree that it could imply that for most Widgets because in general there is only one image to be displayed, the one set by setImage(). But, in case of ToolItem, it can display either one of the 3 images: enabled, disabled and hot. To avoid confusion and to be able to access all the 3 images irrespective of widget state, we have 3 pairs of APIs get/setImage, get/setDisabledImage and get/setHotImage. This javadoc is in Item class, if it helps we could override and improve the javadoc of ToolItem.getItem(). > Are there other cases in SWT similar to this? AFAIK the only other Widget that has an API to disabled image is CTabItem, it's deprecated but is similar in implementation. > It seems incorrect to me that a client should have to check the state of a > widget before querying the image it is displaying. It's just not enabled or disabled image, the displayed image could also be the hot image. :) Item.getImage() is an existing API. From the Windows and Mac implementation, the getImage() API was designed to return the image set by setImage(). GTK's implementation should be consistent here. The bigger problem is that the current GTK implementation of setDisabledImage() breaks the Item.getImage() API. For example: Some existing client code that uses ToolItem.getImage() and depends on the behaviour to return 'enabled' image irrespective of the state, will now get *either* the 'enabled' or 'disabled' image based on the enabled state. If you think it's important for the tests or if there is a client request, we can create a new API to return the image being displayed. (In reply to Lakshmi Shanmugam from comment #33) > (In reply to Eric Williams from comment #32) > > > > The Javadoc for Item.getImage() reads: > > > > "Returns the receiver's image if it has one, or null if it does not." > > > > This implies that the image returned by getImage() is the image that is > > displayed by the receiver, no? > > I agree that it could imply that for most Widgets because in general there > is only one image to be displayed, the one set by setImage(). > But, in case of ToolItem, it can display either one of the 3 images: > enabled, disabled and hot. To avoid confusion and to be able to access all > the 3 images irrespective of widget state, we have 3 pairs of APIs > get/setImage, get/setDisabledImage and get/setHotImage. > This javadoc is in Item class, if it helps we could override and improve the > javadoc of ToolItem.getItem(). Do you mean ToolItem.getImage()? > > It seems incorrect to me that a client should have to check the state of a > > widget before querying the image it is displaying. > It's just not enabled or disabled image, the displayed image could also be > the hot image. :) > > Item.getImage() is an existing API. From the Windows and Mac implementation, > the getImage() API was designed to return the image set by setImage(). GTK's > implementation should be consistent here. > > The bigger problem is that the current GTK implementation of > setDisabledImage() breaks the Item.getImage() API. > For example: Some existing client code that uses ToolItem.getImage() and > depends on the behaviour to return 'enabled' image irrespective of the > state, will now get *either* the 'enabled' or 'disabled' image based on the > enabled state. IMO just because things have been done a certain way in the past doesn't mean it's the right way. If we are going to change GTK's ToolItem.setDisabledImage() behaviour back to what Mac/Windows do then the Javadocs should be modified -- it's far too ambiguous as it's written right now. A client shouldn't have to guess which image they are going to get when calling something generic like "getImage", especially when the Javadocs are equally ambiguous. > If you think it's important for the tests or if there is a client request, > we can create a new API to return the image being displayed. As it's written right now, I would expect Item.getImage() to return this. Adding additional API seems redundant. (In reply to Eric Williams from comment #34) > (In reply to Lakshmi Shanmugam from comment #33) > > (In reply to Eric Williams from comment #32) > > > > > > The Javadoc for Item.getImage() reads: > > > > > > "Returns the receiver's image if it has one, or null if it does not." > > > > > > This implies that the image returned by getImage() is the image that is > > > displayed by the receiver, no? > > > > I agree that it could imply that for most Widgets because in general there > > is only one image to be displayed, the one set by setImage(). > > But, in case of ToolItem, it can display either one of the 3 images: > > enabled, disabled and hot. To avoid confusion and to be able to access all > > the 3 images irrespective of widget state, we have 3 pairs of APIs > > get/setImage, get/setDisabledImage and get/setHotImage. > > This javadoc is in Item class, if it helps we could override and improve the > > javadoc of ToolItem.getItem(). > > Do you mean ToolItem.getImage()? Sorry, yes. > > > > > It seems incorrect to me that a client should have to check the state of a > > > widget before querying the image it is displaying. > > It's just not enabled or disabled image, the displayed image could also be > > the hot image. :) > > > > Item.getImage() is an existing API. From the Windows and Mac implementation, > > the getImage() API was designed to return the image set by setImage(). GTK's > > implementation should be consistent here. > > > > The bigger problem is that the current GTK implementation of > > setDisabledImage() breaks the Item.getImage() API. > > For example: Some existing client code that uses ToolItem.getImage() and > > depends on the behaviour to return 'enabled' image irrespective of the > > state, will now get *either* the 'enabled' or 'disabled' image based on the > > enabled state. > > IMO just because things have been done a certain way in the past doesn't > mean it's the right way. Honestly, I can't see what is wrong with the API. However, I would not like to debate further if it's right or not. What matters is that we can't change behaviour of existing API (ToolItem.getImage()) as it can break client code. > If we are going to change GTK's > ToolItem.setDisabledImage() behaviour back to what Mac/Windows do then the > Javadocs should be modified -- it's far too ambiguous as it's written right > now. A client shouldn't have to guess which image they are going to get when > calling something generic like "getImage", especially when the Javadocs are > equally ambiguous. > Yes, I agree. I suggested the same in above comment. Okay I'll prepare something for RC1. New Gerrit change created: https://git.eclipse.org/r/152962 Gerrit change https://git.eclipse.org/r/152962 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=076c8373de27a918ae94e3a6b29303af6ba615c6 (In reply to Eclipse Genie from comment #38) > Gerrit change https://git.eclipse.org/r/152962 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=076c8373de27a918ae94e3a6b29303af6ba615c6 Shouldn't this have +1 approval from the project lead first? (In reply to Andrey Loskutov from comment #39) > (In reply to Eclipse Genie from comment #38) > > Gerrit change https://git.eclipse.org/r/152962 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > > ?id=076c8373de27a918ae94e3a6b29303af6ba615c6 > > Shouldn't this have +1 approval from the project lead first? Lakshmi is project co-lead for SWT, so no further approval needed IIRC. In reply to Eclipse Genie from comment #38) > Gerrit change https://git.eclipse.org/r/152962 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=076c8373de27a918ae94e3a6b29303af6ba615c6 > Thanks Eric for the fix! (In reply to Lakshmi Shanmugam from comment #41) > Thanks Eric for the fix! No problem! Verified in I20191126-0600. |