Bugzilla – Full Text Bug Listing
| Summary: | setBackground does not work in DateTime with SWT.CALENDAR | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Bogdan Gheorghe <gheorghe> | ||||||||||
| Component: | SWT | Assignee: | Arun Thondapu <arunkumar.thondapu> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Carolyn MacLeod <carolynmacleod4> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | carolynmacleod4 | ||||||||||
| Version: | 3.7 | ||||||||||||
| Target Milestone: | 4.2 M3 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Bogdan Gheorghe
Arun, please take a look at this. Created attachment 186976 [details]
Proposed patch
I had to override the setBackgroundColor method to get the desired behavior as stated in the Description, specifically to ensure that the calendar header doesn't change its color. I have tested the patch with GTK 2.20. Please review the changes, thanks! In setBackgroundColor, you get gtk_rc_style_get_color_flags but don't do anything with it in the end. Is this needed? Created attachment 189852 [details]
Updated patch
The flags variable is indeed not being used and is not required. I missed it somehow while creating the patch, I have removed the relevant portions of code now. Thanks for pointing it out!
Please review the revised patch. Thanks!
Created attachment 190437 [details] new patch that follows framework pattern and includes getter Hi, Arun. Here is the patch I would like to go with. It overrides Control's setBackgroundColor and getBackgroundColor, and is similar to overrides that have been done in Text, Spinner, Table, List, etc. to set the base color. I will release this after you have finished updating the patch in bug 329291. Thanks for the new patch Carolyn! I just realized there is a JUnit failure with this patch and I'll look into it.(In reply to comment #6) > Created attachment 190437 [details] > new patch that follows framework pattern and includes getter > > Hi, Arun. Here is the patch I would like to go with. It overrides Control's > setBackgroundColor and getBackgroundColor, and is similar to overrides that > have been done in Text, Spinner, Table, List, etc. to set the base color. > I will release this after you have finished updating the patch in bug 329291. The patch works as expected when a proper color is chosen but it doesn't work when the color is set to null (doesn't revert to default). This is what is causing the JUnit failure too. I'm trying to figure out how to fix this. (In reply to comment #7) > Thanks for the new patch Carolyn! > I just realized there is a JUnit failure with this patch and I'll look into > it.(In reply to comment #6) > > Created attachment 190437 [details] [details] > > new patch that follows framework pattern and includes getter > > > > Hi, Arun. Here is the patch I would like to go with. It overrides Control's > > setBackgroundColor and getBackgroundColor, and is similar to overrides that > > have been done in Text, Spinner, Table, List, etc. to set the base color. > > I will release this after you have finished updating the patch in bug 329291. Created attachment 191604 [details]
modified patch
Hi Carolyn,
I modified your patch by adding a separate check for color being set to null.
In the original patch, the overridden setBackgroundColor method doesn't get called when the color is being set to null and so the background color doesn't get updated.
The reason the overridden method is not getting called is that the following statement of code in Control#setBackground(Color) method doesn't get evaluated to true.
set = (OS.gtk_rc_style_get_color_flags (style, OS.GTK_STATE_NORMAL) & OS.GTK_RC_BG) != 0;
I tried adding code to set the flags appropriately in the overridden methods but that did not give me the desired effect.
I decided to add this separate check for null since I didn't want to change any code in the Control class.
All JUnit tests pass.
Please let me know your comments/suggestions. Thanks!
Fixed http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=8d66c54f1806e5f4886545c525b731676efd0ad4 Thanks, Arun! I committed the patch in comment 9, except that I added an extra pair of { .. } for the new if statement. Sorry to take so long to commit this - I spent quite a bit of time getting up to speed with git...... Thanks Carolyn! |