Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 538211

Summary: Refactoring Wizard's Preview shows light background in dark theme
Product: [Eclipse Project] JDT Reporter: Matthias Becker <ma.becker>
Component: UIAssignee: Matthias Becker <ma.becker>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: daniel_megert, Lars.Vogel, noopur_gupta, sarika.sinha, twolf
Version: 4.9   
Target Milestone: 4.10 M3   
Hardware: All   
OS: All   
See Also: https://git.eclipse.org/r/127942
https://bugs.eclipse.org/bugs/show_bug.cgi?id=533879
https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=0c40c1089b8338ffe690c09c4be80401d9ddda92
Whiteboard:
Attachments:
Description Flags
screen shot showing the issue
none
screen shot showing the fix none

Description Matthias Becker CLA 2018-08-23 10:52:25 EDT
Created attachment 275506 [details]
screen shot showing the issue

The last page of the "Externalize String" wizard shows a summary of all changes to be performed. When new files are created here the preview of the new file has light background in the dark theme.
Comment 1 Eclipse Genie CLA 2018-08-23 10:55:55 EDT
New Gerrit change created: https://git.eclipse.org/r/127942
Comment 2 Matthias Becker CLA 2018-08-23 10:56:46 EDT
Created attachment 275507 [details]
screen shot showing the fix
Comment 3 Matthias Becker CLA 2018-08-24 06:17:37 EDT
This is fixed from my side. Can a JDT-Committer pls. review and merge?
Comment 4 Dani Megert CLA 2018-08-24 13:09:51 EDT
This change is too big for RC1.
Comment 5 Matthias Becker CLA 2018-09-18 03:29:17 EDT
Can a JDT committer pls. merge the change?
Comment 6 Matthias Becker CLA 2018-09-25 05:19:29 EDT
@Thomas: Is there still something to do from my side before you merge it?
Comment 7 Noopur Gupta CLA 2018-09-25 10:49:00 EDT
Thomas, can you please review the change and confirm that the fix works fine for both light and dark themes on all the platforms (Windows, Mac, Linux)?
Comment 8 Thomas Wolf CLA 2018-09-25 11:00:29 EDT
(In reply to Noopur Gupta from comment #7)
> Thomas, can you please review the change and confirm that the fix works fine
> for both light and dark themes on all the platforms (Windows, Mac, Linux)?

I have no development environment set up for working on JDT, so it would take quite a bit of work to even get to the point where I could check out and run this change. I do not have a Windows machine. I have a Mac and two Linux (Ubuntu/CentOS) VMs on that. 

So: no, I'm sorry, I cannot do that.

I can confirm that the setColors() code is the same as we are using in EGit for 3 years now.[1] That's all I can say.

[1] https://git.eclipse.org/c/egit/egit.git/tree/org.eclipse.egit.ui/src/org/eclipse/egit/ui/internal/dialogs/HyperlinkSourceViewer.java#n334
Comment 9 Noopur Gupta CLA 2018-09-25 11:11:06 EDT
Thanks, Thomas.

Matthias, please update the patch to follow JDT UI coding conventions (https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions). For example, we use the compact assignment form (a= b, i.e. no space on the left of equals).

Also, can you please test and confirm that the fix works fine for both light and dark themes on all the platforms (Windows, Mac, Linux)?
Comment 10 Matthias Becker CLA 2018-09-25 11:20:41 EDT
(In reply to Noopur Gupta from comment #9)
> Thanks, Thomas.
> 
> Matthias, please update the patch to follow JDT UI coding conventions
> (https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions). For
> example, we use the compact assignment form (a= b, i.e. no space on the left
> of equals).
> 
> Also, can you please test and confirm that the fix works fine for both light
> and dark themes on all the platforms (Windows, Mac, Linux)?

Dear Noopur,

I can confirm this works on macOS as I developed it there. I don't have a windows or linux at hand.
Regarding the coding conventions: I am very short on time and have a lot of other (more  important stuff to do). So it might be faster that you adapt the patch to match your conventions.
Comment 11 Noopur Gupta CLA 2018-10-05 05:35:02 EDT
Out of time for M1.
Comment 12 Matthias Becker CLA 2018-10-06 05:09:55 EDT
(In reply to Noopur Gupta from comment #11)
> Out of time for M1.

from my side it was already ready for the last release
Comment 13 Matthias Becker CLA 2018-10-17 08:12:23 EDT
(In reply to Noopur Gupta from comment #9)
> Thanks, Thomas.
> 
> Matthias, please update the patch to follow JDT UI coding conventions
> (https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions). For
> example, we use the compact assignment form (a= b, i.e. no space on the left
> of equals).
--> Done.
> 
> Also, can you please test and confirm that the fix works fine for both light
> and dark themes on all the platforms (Windows, Mac, Linux)?
-> Tested on macOS.
Comment 14 Matthias Becker CLA 2018-10-17 09:03:04 EDT
(In reply to Matthias Becker from comment #13)
> (In reply to Noopur Gupta from comment #9)
> > Thanks, Thomas.
> > 
> > Matthias, please update the patch to follow JDT UI coding conventions
> > (https://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions). For
> > example, we use the compact assignment form (a= b, i.e. no space on the left
> > of equals).
> --> Done.
> > 
> > Also, can you please test and confirm that the fix works fine for both light
> > and dark themes on all the platforms (Windows, Mac, Linux)?
> -> Tested on macOS.

And tested on windows.
can pls. somebody else test on linux.
Comment 15 Lars Vogel CLA 2018-10-17 15:41:08 EDT
(In reply to Matthias Becker from comment #14)
> can pls. somebody else test on linux.

Looks fine on Linux (dark and white).
Comment 17 Sarika Sinha CLA 2018-10-18 01:41:37 EDT
Thanks for the contribution, Matthias, Thomas and Lars!!

Please verify in the next I build on Windows, Linux and MacOS.

Do we need a N&N entry ?
Comment 18 Matthias Becker CLA 2018-10-18 05:22:48 EDT
(In reply to Sarika Sinha from comment #17)
> Thanks for the contribution, Matthias, Thomas and Lars!!
> 
> Please verify in the next I build on Windows, Linux and MacOS.
> 
> Do we need a N&N entry ?

Would be nice. But unfortunatelly I don't have the time for it. I'm in the office the last day today and then I am on vacation for 4 weeks. Sorry.
Comment 19 Thomas Wolf CLA 2018-10-18 05:27:18 EDT
This is basically a bug fix. New or massively improved functionality should have a N&N, but simple bug fixes like this don't need one in my opinion.
Comment 20 Lars Vogel CLA 2018-10-18 05:35:19 EDT
(In reply to Thomas Wolf from comment #19)
> This is basically a bug fix. New or massively improved functionality should
> have a N&N, but simple bug fixes like this don't need one in my opinion.

+1, I don't think this is N&N worthy.