Bug 518737 - [Win32] Background switches to light if tree widget is disabled
Summary: [Win32] Background switches to light if tree widget is disabled
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.7   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 4.8 M4   Edit
Assignee: Conrad Groth CLA
QA Contact: Niraj Modi CLA
URL:
Whiteboard:
Keywords:
Depends on: 511355
Blocks: 41353 497562
  Show dependency tree
 
Reported: 2017-06-23 18:19 EDT by Conrad Groth CLA
Modified: 2017-12-06 06:24 EST (History)
7 users (show)

See Also:


Attachments
Picture showing broken tree (13.18 KB, image/png)
2017-10-27 10:37 EDT, Dani Megert CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Conrad Groth CLA 2017-06-23 18:19:05 EDT
The same behavior described in Bug 516365 happens for the Tree widget, i.e. the background gets a system default color when the widget is disabled. Can be easily reproduced with the ControlExample.
Comment 1 Niraj Modi CLA 2017-10-09 02:46:26 EDT
(In reply to Conrad Groth from comment #0)
> The same behavior described in Bug 516365 happens for the Tree widget, i.e.
> the background gets a system default color when the widget is disabled. Can
> be easily reproduced with the ControlExample.

Hi Conrad,
With fix for Table widget bug 516365 in place, we can proceed with Tree changes.
For consistency between Table and Tree widgets please target M3 itself. Thanks!
Comment 2 Niraj Modi CLA 2017-10-11 03:13:53 EDT
Recent note from Conrad, regarding his availability in M3:
---------------------------------------------------------------- 
Hi Niraj,

 

> With fix for Table widget bug 516365 in place, we can proceed with Tree changes.

> For consistency between Table and Tree widgets please target M3 itself. Thanks!

 

Iā€™m on vacation until October 17th. So I can only provide the Tree patch after that date.

 

Best regards

Conrad
Comment 3 Eclipse Genie CLA 2017-10-22 07:45:16 EDT
New Gerrit change created: https://git.eclipse.org/r/110486
Comment 5 Niraj Modi CLA 2017-10-23 04:36:51 EDT
(In reply to Eclipse Genie from comment #4)
> Gerrit change https://git.eclipse.org/r/110486 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=dd3790a1446afe8dae6516c0cd3905ef4d295a8a

Thanks Conrad for the fix, resolving now.
Comment 6 Niraj Modi CLA 2017-10-24 07:51:38 EDT
Verified with 4.8 M3 candidate Eclipse Build: I20171023-2000 on Win10.
Comment 7 Dani Megert CLA 2017-10-27 09:59:55 EDT
Very bad! This breaks makes the sort column dark gray and removes the row lines for that column.

We might have to respin M3.
Comment 8 Dani Megert CLA 2017-10-27 10:06:35 EDT
This bug should never have been pushed to M3. Only regressions found in the test pass and blockers should be committed after a test pass. Why was this pushed?
Comment 9 Dani Megert CLA 2017-10-27 10:37:44 EDT
Created attachment 271216 [details]
Picture showing broken tree
Comment 10 Conrad Groth CLA 2017-10-27 15:36:29 EDT
In fact this behavior wasn't introduced by this patch only. It was introduced by some other custom-coloring bug (I caouldn't find the number quickly) and already reported as bug 511355. I already proposed a patch for that ticket, but unfortunately I lost that patch from my radar totally. Sorry for that. Will provide an updated patch for bug 511355 shortly and that will also address Table and Tree coloring issue.
Comment 11 Dani Megert CLA 2017-10-29 05:26:24 EDT
(In reply to Dani Megert from comment #8)
> This bug should never have been pushed to M3. Only regressions found in the
> test pass and blockers should be committed after a test pass. Why was this
> pushed?

Sorry, I looked at the wrong date. The fix was pushed before the test pass. I wonder why no one detected this.
Comment 12 Dani Megert CLA 2017-10-29 05:33:47 EDT
(In reply to Conrad Groth from comment #10)
> In fact this behavior wasn't introduced by this patch only. It was
> introduced by some other custom-coloring bug (I caouldn't find the number
> quickly) and already reported as bug 511355. I already proposed a patch for
> that ticket, but unfortunately I lost that patch from my radar totally.
> Sorry for that. Will provide an updated patch for bug 511355 shortly and
> that will also address Table and Tree coloring issue.

The PMC decided to respin M3 to provide a fix. I tested that reverting the change  brings back the old look. Another approach is to provide the fix for bug 511355 today or early tomorrow, so that we can respin on early on Monday. Reverting is probably less risky.
Comment 13 Conrad Groth CLA 2017-10-29 05:49:38 EDT
(In reply to Dani Megert from comment #12)
> The PMC decided to respin M3 to provide a fix. I tested that reverting the
> change  brings back the old look. Another approach is to provide the fix for
> bug 511355 today or early tomorrow, so that we can respin on early on
> Monday. Reverting is probably less risky.

I prefer reverting, as it's the easiest and least risky option.
Comment 14 Conrad Groth CLA 2017-10-29 05:51:04 EDT
I cannot revert the commit as I'm no committer. This has to be done by someone else.
Comment 15 Eclipse Genie CLA 2017-10-30 01:28:23 EDT
New Gerrit change created: https://git.eclipse.org/r/110710
Comment 16 Niraj Modi CLA 2017-10-30 02:32:27 EDT
(In reply to Eclipse Genie from comment #15)
> New Gerrit change created: https://git.eclipse.org/r/110710

We will revert the fix, via above gerrit shortly.
Comment 18 Niraj Modi CLA 2017-10-30 02:57:15 EDT
(In reply to Eclipse Genie from comment #17)
> Gerrit change https://git.eclipse.org/r/110710 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=6d54623bc929a1ff670c6cde1f13b511105ad805

Reverted, moving the bug to M4.
Comment 19 Dani Megert CLA 2017-10-30 06:18:40 EDT
(In reply to Niraj Modi from comment #18)
> (In reply to Eclipse Genie from comment #17)
> > Gerrit change https://git.eclipse.org/r/110710 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> > ?id=6d54623bc929a1ff670c6cde1f13b511105ad805
> 
> Reverted, moving the bug to M4.

Verified the fix on Windows with eclipse-SDK-4.7.1-win32-x86_64.
Comment 20 Dani Megert CLA 2017-10-30 06:22:00 EDT
(In reply to Dani Megert from comment #19)
> (In reply to Niraj Modi from comment #18)
> > (In reply to Eclipse Genie from comment #17)
> > > Gerrit change https://git.eclipse.org/r/110710 was merged to [master].
> > > Commit:
> > > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> > > ?id=6d54623bc929a1ff670c6cde1f13b511105ad805
> > 
> > Reverted, moving the bug to M4.
> 
> Verified the fix on Windows with eclipse-SDK-4.7.1-win32-x86_64.

I tested with eclipse-SDK-I20171030-0400-win32-x86_64.
Comment 21 Niraj Modi CLA 2017-11-07 10:02:48 EST
(In reply to Conrad Groth from comment #10)
> In fact this behavior wasn't introduced by this patch only. It was
> introduced by some other custom-coloring bug (I caouldn't find the number
> quickly) and already reported as bug 511355. I already proposed a patch for
> that ticket, but unfortunately I lost that patch from my radar totally.
> Sorry for that. Will provide an updated patch for bug 511355 shortly and
> that will also address Table and Tree coloring issue.

Combined tested, the latest patch from bug 511355 along with the original fix proposed for this bug 518737 and it doesn't have the problem w.r.t. Tree/Table Sort columns mentioned in https://bugs.eclipse.org/bugs/show_bug.cgi?id=518737#c7

Will push the changes for both these bugs shortly.
Comment 22 Eclipse Genie CLA 2017-11-07 10:05:57 EST
New Gerrit change created: https://git.eclipse.org/r/111122
Comment 24 Niraj Modi CLA 2017-11-08 04:54:50 EST
Thanks Conrad for your contribution. Resolving now.
Comment 25 Lars Vogel CLA 2017-11-08 06:26:50 EST
(In reply to Niraj Modi from comment #24)
> Thanks Conrad for your contribution. Resolving now.

Could we downport this to 4.7.2?
Comment 26 Dani Megert CLA 2017-11-10 12:06:36 EST
(In reply to Lars Vogel from comment #25)
> (In reply to Niraj Modi from comment #24)
> > Thanks Conrad for your contribution. Resolving now.
> 
> Could we downport this to 4.7.2?

-1. 4.7.3 could be an option though. Not sure how much it depends on other changes that were made in that area.
Comment 27 Dani Megert CLA 2017-11-10 12:09:48 EST
(In reply to Dani Megert from comment #26)
> (In reply to Lars Vogel from comment #25)
> > (In reply to Niraj Modi from comment #24)
> > > Thanks Conrad for your contribution. Resolving now.
> > 
> > Could we downport this to 4.7.2?
> 
> -1. 4.7.3 could be an option though. Not sure how much it depends on other
> changes that were made in that area.

See https://bugs.eclipse.org/bugs/showdependencytree.cgi?id=518737&hide_resolved=0 which shows 4 dependent bugs.
Comment 28 Conrad Groth CLA 2017-11-12 12:51:41 EST
(In reply to Lars Vogel from comment #25)
> Could we downport this to 4.7.2?

I'm not interested in it and I will not do it.
Comment 29 Niraj Modi CLA 2017-12-06 06:24:31 EST
Verified using Eclipse Build id: I20171205-2000 on Win7