Community
Participate
Working Groups
Created attachment 265708 [details] Screenshot of SmartGit on Windows 10 Please see attached screenshot.
From Control#setForeground(Color): * Note: This operation is a hint and may be overridden by the platform.
Only SWT.READ_ONLY combos are ignoring their background and foreground color (left control on the screenshot). The right combo control in the screenshot shows the behavior of a 'normal' combo.
*** Bug 516539 has been marked as a duplicate of this bug. ***
(In reply to Conrad Groth from comment #2) > Only SWT.READ_ONLY combos are ignoring their background and foreground color > (left control on the screenshot). The right combo control in the screenshot > shows the behavior of a 'normal' combo. Conrad, can you look into fixing this bug?
The patch is ready for review. During testing I discovered the same problem as for the table header; the hot tracking doesn't work. The HOT flag isn't set in the DRAWITEMSTRUCT I get in the wmDrawChild method. If this is really needed I can investigate it later.
(In reply to Conrad Groth from comment #5) > The patch is ready for review. Hi Conrad, Behavior wise patch works fine at Win64bit, but seeing compilation failures on win32 bit(please refer suggested fix in gerrit comments). > During testing I discovered the same problem as for the table header; the > hot tracking doesn't work. The HOT flag isn't set in the DRAWITEMSTRUCT I > get in the wmDrawChild method. If this is really needed I can investigate it > later. Please raise a separate bug to track this. Thanks!
Ping!
Moved to M4, since given patch doesn't work on Windows 32bit.
(In reply to Niraj Modi from comment #8) > Moved to M4, since given patch doesn't work on Windows 32bit. Hi Conrad, The proposed patch works on 64bit, do you plan to update the patch with suggested changes in gerrit ?
Hi Conrad, Also, latest patch compiles fine on 32bit as well. Noticed one behavior difference w.r.t. to patch-set2, the READ_ONLY combo not have maintains the native look and only the drop-down shows the theming part. Appreciate if you could call out such behavior changes explicitly in bugzilla. Again in latest patch, was not expecting an API change introduced in latest patch. The FLAT style across platforms. We prefer discussing need for such a change in bugzilla as well. What would be the behavior if user only applied the FLAT style and no thmeing, suggest you to modify ControlExample as well in order to verify the part. Since a new Style is added to existing widget, I am moving this out to M5 for proper analysis. Thanks!
Moving out of M5
(In reply to Niraj Modi from comment #10) > Hi Conrad, > Also, latest patch compiles fine on 32bit as well. > > Noticed one behavior difference w.r.t. to patch-set2, the READ_ONLY combo > not have maintains the native look and only the drop-down shows the theming > part. > Appreciate if you could call out such behavior changes explicitly in > bugzilla. > > Again in latest patch, was not expecting an API change introduced in latest > patch. The FLAT style across platforms. > We prefer discussing need for such a change in bugzilla as well. > What would be the behavior if user only applied the FLAT style and no > thmeing, suggest you to modify ControlExample as well in order to verify the > part. > > Since a new Style is added to existing widget, I am moving this out to M5 > for proper analysis. Thanks! With the slightly changed table background in 4.8 M3 in mind, which caused the whole build chain to respin, I don't want to introduce another change that causes all Combos to lose their native look under Windows XP and 7. That's the reason why the custom coloring under Windows 7 and before should only be enabled if the developer allows it by setting the FLAT flag. If someone feels brave enough to set the FLAT flag on all combos in the Eclipse IDE afterwards it's his/her change to blame and not mine again :(
Changing all combo is not possible, as Eclipse is an extensible platform and we do not control everything. The change should work for the normal style flags.
(In reply to Lars Vogel from comment #13) > Changing all combo is not possible, as Eclipse is an extensible platform and > we do not control everything. The change should work for the normal style > flags. This is not possible without losing the native (button-like) look under Windows XP and 7, because the style flag can only be set during creation of the (native) control. A workaround would be, to recreate the control when the user sets a custom background. I don't have an idea, whether this is possible at all and -more important- what performance effect that has. Just for clarification: on Windows 8 and 10 no FLAT style is needed, as the (native) combo control always has a rectangular shape. My suggestion is: go ahead with this patch and evaluate possibilities for windows 7 and older in another patch.
Created attachment 272513 [details] READ_ONLY Combo theming requires FLAT style (In reply to Conrad Groth from comment #14) > (In reply to Lars Vogel from comment #13) > > Changing all combo is not possible, as Eclipse is an extensible platform and > > we do not control everything. The change should work for the normal style > > flags. Did a further testing of the patch. Here the scenario is slightly different as compared to Button(please refer attachment) With the patch applied, READ_ONLY Cobmo widget become FLAT even when without theming is applied, so in a way it justifies the need for explicit FLAT style. > This is not possible without losing the native (button-like) look under > Windows XP and 7, because the style flag can only be set during creation of > the (native) control. We don't support Win XP anymore, so don't worry about it. > A workaround would be, to recreate the control when > the user sets a custom background. I don't have an idea, whether this is > possible at all and -more important- what performance effect that has. Re-creating the widget is a bad idea anyways. > Just for clarification: on Windows 8 and 10 no FLAT style is needed, as the > (native) combo control always has a rectangular shape. > > My suggestion is: go ahead with this patch and evaluate possibilities for > windows 7 and older in another patch. OK, if we want to avoid SWT.FLAT style, we have 2 options: 1. Either we enable this READ_ONLY Combo theming functionality only for Win8 and above without explicit FLAT style. 2. or we change the look of standard READ_ONLY Combos to look flat even without theming (please refer attachment)
(In reply to Niraj Modi from comment #15) > > My suggestion is: go ahead with this patch and evaluate possibilities for > > windows 7 and older in another patch. > > OK, if we want to avoid SWT.FLAT style, we have 2 options: > 1. Either we enable this READ_ONLY Combo theming functionality only for Win8 > and above without explicit FLAT style. > 2. or we change the look of standard READ_ONLY Combos to look flat even > without theming > (please refer attachment) I think we can't change the default native look of the READ_ONLY Combos just like that. (It's equal to breaking change) May be option1 is better.
Hi Conrad, Had a discussion on this, lets break this enhancement in two parts: 1. Lets remove the FLAT style for now and support READ_ONLY Combobox theming for Win8 and onward OS only. 2. Since to support this enhancement on Win7 we will need to introduce the new FLAT style, we address this part separately only when we get an explicit request from community. So, suggest to update the patch to address the first part only.
(In reply to Niraj Modi from comment #17) > Hi Conrad, > Had a discussion on this, lets break this enhancement in two parts: > 1. Lets remove the FLAT style for now and support READ_ONLY Combobox theming > for Win8 and onward OS only. > So, suggest to update the patch to address the first part only. +1, sounds like a good pragmatic approach.
(In reply to Niraj Modi from comment #19) > Ping! Niraj, AFAICS Conrad updated the patch accordingly. Please review.
(In reply to Lars Vogel from comment #20) > (In reply to Niraj Modi from comment #19) > > Ping! > > Niraj, AFAICS Conrad updated the patch accordingly. Please review. Ok, will review for M7.
Hi Conrad, Tested the latest patch(set 7), largely it works fine except for one scenario. - On Win10 I can see the theming works on READ_ONLY combo. - Also didn't noticed any behavior change as tested on Win7. Breaking scenario on Win10: Without the patch:- The Combo widget without any fg/bg color, gets greys out when marked as 'READ_ONLY'. With latest patch:- The Combo widget without any fg/bg color, doesn't turn grey when marked as 'READ_ONLY'. It should have turned to grey in this case.
(In reply to Niraj Modi from comment #22) > Hi Conrad, > Tested the latest patch(set 7), largely it works fine except for one > scenario. > - On Win10 I can see the theming works on READ_ONLY combo. > - Also didn't noticed any behavior change as tested on Win7. > > Breaking scenario on Win10: > Without the patch:- The Combo widget without any fg/bg color, gets greys out > when marked as 'READ_ONLY'. > > With latest patch:- The Combo widget without any fg/bg color, doesn't turn > grey when marked as 'READ_ONLY'. It should have turned to grey in this case. I think, you mean the combo gets greyed out when it is disabled. I adapted that behaviour for the now custom-drawn READ_ONLY combo. The foreground gets grey but the background stay for disabled combos. The behaviour is now equal for READ_ONLY combos and 'normal' editable combos.
Created attachment 273826 [details] Win10_Standard(unthemed) READ_ONLY Combo behavior(with and without patch) (In reply to Conrad Groth from comment #23) > (In reply to Niraj Modi from comment #22) > > Breaking scenario on Win10: > > Without the patch:- The Combo widget without any fg/bg color, gets greys out > > when marked as 'READ_ONLY'. > > > > With latest patch:- The Combo widget without any fg/bg color, doesn't turn > > grey when marked as 'READ_ONLY'. It should have turned to grey in this case. > > I think, you mean the combo gets greyed out when it is disabled. I adapted > that behaviour for the now custom-drawn READ_ONLY combo. The foreground gets > grey but the background stay for disabled combos. The behaviour is now equal > for READ_ONLY combos and 'normal' editable combos. Tested the latest patch with ControlExample, I can still see the problem mentioned in comment 22 with standard(un-themed) READ_ONLY Combo seen on Win10. Attaching a screen shot(with and without your patch) highlighting the problem.
(In reply to Niraj Modi from comment #24) > Attaching a screen shot(with and without your patch) highlighting the > problem. Conrad, can you have a look soon? This is our last Photon development week.
(In reply to Niraj Modi from comment #24) > [...] > Tested the latest patch with ControlExample, I can still see the problem > mentioned in comment 22 with standard(un-themed) READ_ONLY Combo seen on > Win10. > Attaching a screen shot(with and without your patch) highlighting the > problem. I misunderstood your comment 22. In changeset 11 I also changed to grey background, if no custom colors are set. Unfortunately the grey color used by the OS has RGB=225,225,225 which corresponds to no color constant. The nearest color I could find was COLOR_3DLIGHT, which has RGB=227,227,227 on my windows 10 computer.
(In reply to Conrad Groth from comment #26) > (In reply to Niraj Modi from comment #24) > > [...] > > Tested the latest patch with ControlExample, I can still see the problem > > mentioned in comment 22 with standard(un-themed) READ_ONLY Combo seen on > > Win10. > > Attaching a screen shot(with and without your patch) highlighting the > > problem. > > I misunderstood your comment 22. In changeset 11 I also changed to grey > background, if no custom colors are set. Unfortunately the grey color used > by the OS has RGB=225,225,225 which corresponds to no color constant. The > nearest color I could find was COLOR_3DLIGHT, which has RGB=227,227,227 on > my windows 10 computer. Thanks, will verify this today.
Created attachment 273935 [details] Win10 Combo behavior comparison. (In reply to Conrad Groth from comment #26) > (In reply to Niraj Modi from comment #24) > > [...] > > Tested the latest patch with ControlExample, I can still see the problem > > mentioned in comment 22 with standard(un-themed) READ_ONLY Combo seen on > > Win10. > > Attaching a screen shot(with and without your patch) highlighting the > > problem. > > I misunderstood your comment 22. In changeset 11 I also changed to grey > background, if no custom colors are set. Unfortunately the grey color used > by the OS has RGB=225,225,225 which corresponds to no color constant. The > nearest color I could find was COLOR_3DLIGHT, which has RGB=227,227,227 on > my windows 10 computer. Tested the latest patch, it improves the behavior. But it's still a deviation from what we see currently in standard(un-themed) READ_ONLY Combo, sharing the screen shot for reference. [Compare 1st and 2nd images.] With the latest patch, the Drop-down area is not greyed out and standard(un-themed) READ_ONLY combo very much looks like a themed combo with grey background.[Compare 2nd and 3rd images.]
The drop-down-area cannot be custom-drawn. It always stays white in custom-draw-mode. I uploaded a patch that destroys the widget and recreates it with the new widget style. So without custom colors it is drawn by the OS, but as soon as foreground and/or background Color is set, the widget is recreated as custom-drawn. Also the recreation works vice versa, so upon clearing the custom Colors the combo changes back to native drawing. I did some quick tests and it looks better than expected, but this patch needs intensive testing, because AFAIK it's the first widget that gets recreated. Especially we have to ensure that no DISPOSE event is sent to someone and that we don't expose any references that get invalid upon recreation. So I set the target to 4.9
By the way: If you only set a custom foreground Color, the background will get grey but without the drop-down-area. I think, this is not a big problem, because foreground and background will be changed together.
(In reply to Conrad Groth from comment #29) > The drop-down-area cannot be custom-drawn. It always stays white in > custom-draw-mode. > > I uploaded a patch that destroys the widget and recreates it with the new > widget style. So without custom colors it is drawn by the OS, but as soon as > foreground and/or background Color is set, the widget is recreated as > custom-drawn. Also the recreation works vice versa, so upon clearing the > custom Colors the combo changes back to native drawing. I did some quick > tests and it looks better than expected, but this patch needs intensive > testing, because AFAIK it's the first widget that gets recreated. Especially > we have to ensure that no DISPOSE event is sent to someone and that we don't > expose any references that get invalid upon recreation. So I set the target > to 4.9 Hi Conrad, Am reviewing your latest patch. Can you verify if any additional events are being sent during recreation of the widget ?
Looks like Conrad is away, bug deferred to 4.10
New Gerrit change created: https://git.eclipse.org/r/131770
I have now spent some time investigating why read-only combo draws wrong, and if there are ways to counter that. I'd like to show the results of it, see https://git.eclipse.org/r/#/c/131770/ This is the best approach I found. It works by manipulating internal state flags in combo, causing it to choose different drawing logic. This approach has following benefits: 1) No noticeable visual problems on both Win7 and Win10 2) Does not require owner-drawing, which is quite a beast to take everything into account 3) Does not involve re-creating control. Of course, it's quite hacky. On the other hand, now that it works from Win7 to Win10, I wouldn't expect much problems. What do you think?
I implemented this patch very quickly in last minutes of working day, so consider it a demo. If you like the approach, I will add more hardening and code comments.
(In reply to Alexandr Miloslavskiy from comment #35) > I implemented this patch very quickly in last minutes of working day, so > consider it a demo. If you like the approach, I will add more hardening and > code comments. Niraj, can you please have a look and comment on the approach?
(In reply to Alexandr Miloslavskiy from comment #35) > I implemented this patch very quickly in last minutes of working day, so > consider it a demo. If you like the approach, I will add more hardening and > code comments. My initial testing, I see the patch works on both Win7 and Win10. One problem I see is the default colored READ_ONLY Combo now looks different, we should try to fix this. One code wise lets harden the code to avoid any possible failures and see if we can reduce on the magic numbers usage. Conrad: Let us know thoughts on the alternate patch.
Uploaded updated fix. 1) It no longer changes comboboxes with default colors. 2) Added hardening to account for possible changes of magic numbers - the fix will simply turn off. I noticed a problem: "background image" is not drawn in my approach * Previously it also wasn't drawn. * Conrad's patch also doesn't draw it, but it can be changed to draw. * My fix can't be changed to draw bitmap. * It's possible to draw bitmap at cost of not selecting text color. More info about changes in appearance when owner-drawn: * It is still there on Win10, just somewhat less noticeable. So it was a mistake to say that it's fixed since Win8. * To be specific, when combobox is owner-drawn, it looks like non-readonly combo. * This is due to code that looks like a bug to me, in Windows function 'comctl32!Combo_IsReadOnly'. Its pseudo-code is: if (0 == (style & CBS_DROPDOWNLIST)) return 0; // comboboxex is a different kind of control if (isOwnerDraw && !isComboBoxEx) return 0; return 1;
Gerrit change https://git.eclipse.org/r/131770 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=2982c4163ff033b48acfa8f9135083028f1a0900
(In reply to Alexandr Miloslavskiy from comment #38) > Uploaded updated fix. > > 1) It no longer changes comboboxes with default colors. > 2) Added hardening to account for possible changes of magic numbers - the > fix will simply turn off. Thanks Alexandr for the updated patch it good to go for the current use-case. > I noticed a problem: "background image" is not drawn in my approach > * Previously it also wasn't drawn. > * Conrad's patch also doesn't draw it, but it can be changed to draw. > * My fix can't be changed to draw bitmap. > * It's possible to draw bitmap at cost of not selecting text color. > > More info about changes in appearance when owner-drawn: > * It is still there on Win10, just somewhat less noticeable. So it was a > mistake to say that it's fixed since Win8. > * To be specific, when combobox is owner-drawn, it looks like non-readonly > combo. > * This is due to code that looks like a bug to me, in Windows function > 'comctl32!Combo_IsReadOnly'. Its pseudo-code is: > if (0 == (style & CBS_DROPDOWNLIST)) > return 0; > > // comboboxex is a different kind of control > if (isOwnerDraw && !isComboBoxEx) > return 0; > > return 1; Suggest to raise a separate bug to address this part.
Thanks, Niraj!
(In reply to Eclipse Genie from comment #39) > Gerrit change https://git.eclipse.org/r/131770 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=2982c4163ff033b48acfa8f9135083028f1a0900 Resolving now.
We will also have to add New and Noteworthy for this enhancement under below page: https://www.eclipse.org/eclipse/news/4.10/platform.php#StylingThemes
Thanks a bunch, Alexandr and Niraj. This will make the usage of the dark theme on Windows way better.
(In reply to Niraj Modi from comment #43) > We will also have to add New and Noteworthy for this enhancement under below > page: > https://www.eclipse.org/eclipse/news/4.10/platform.php#StylingThemes Added the required N&N contents on above page via below commit: https://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=2b182621ec57dd46e38c485f760b31d58d8c1a97
Verified in Eclipse Build id: I20181119-2315
New Gerrit change created: https://git.eclipse.org/r/132832
Gerrit change https://git.eclipse.org/r/132832 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=ba026076860942edd4107468f6e009afcb2d39ac
This enhancement has caused an issue in Combo tracked via bug 550423
