Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 483359 - Colors and Fonts preference page: Reset not correctly propagated
Summary: Colors and Fonts preference page: Reset not correctly propagated
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.5   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.6 M4   Edit
Assignee: Mickael Istria CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 483882
  Show dependency tree
 
Reported: 2015-12-01 04:17 EST by Dani Megert CLA
Modified: 2015-12-10 10:28 EST (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2015-12-01 04:17:16 EST 
4.5 M2

From Mickael in https://git.eclipse.org/r/#/c/61324/:

0. With a Java editor open, open preference page

1. Edit font for Java editor, set size to let's say 24; Apply; see editor updated

2. Edit font for text editors, set size to 24; Apply. Check Java font in the tree, it says "Set to Default"

3. Reset Text font; Apply; multiple issue -> The Java font says "set to default" and shows the default value -> The editor isn't updated and still shows a 24pts font.

4. Reopen preference page -> This time, the 2 values are "unlinked".
Comment 1 Mickael Istria CLA 2015-12-01 07:05:25 EST 
The ColorsAndFontsPreferencePage actually stores data in 3 structures:
* fontPreferencesToSet
* fontValuesToSet
* preferenceRegistry
The tricky part is to infer how each of this structured should be used. The issue is most likely caused by an inconsistency between some of them.
Comment 2 Dani Megert CLA 2015-12-01 12:05:22 EST 
The proposed change got merged.
Comment 3 Mickael Istria CLA 2015-12-04 04:07:40 EST 
This change breaks 2 tests:

* junit.framework.AssertionFailedError: expected:<2> but was:<3>
at org.eclipse.ui.tests.themes.ThemeAPITest.checkEvents(ThemeAPITest.java:163)
at org.eclipse.ui.tests.themes.ThemeAPITest.testFontCascadeEvents(ThemeAPITest.java:427)
* junit.framework.AssertionFailedError: expected:<[1|Tahoma|20.0|1|GTK|1|> but was:<[1|Courier|30.0|2|GTK|1|>
at org.eclipse.ui.tests.themes.ThemeTest.assertArrayEquals(ThemeTest.java:35)
at org.eclipse.ui.tests.themes.ThemeAPITest.testDefaultedFont_valfont(ThemeAPITest.java:351)
Comment 4 Mickael Istria CLA 2015-12-07 07:20:24 EST 
I have the impression that the test in error, despite it actually tries to test cascading of a font change to another using the changed font as default, actually checks that the cascaded change doesn't create a change event. I'm not whether this test actually makes sense.
Comment 5 Mickael Istria CLA 2015-12-07 10:17:04 EST 
The failing testFontCascadeFontEvents was introduced as part of bug 56484, which was about putting some mechanism to keep JFaceResources.fontRegistry and current themes's font registry in sync.
This current bug highlights the missing sync with the PreferenceStore, and also fixes some pitfalls in the cascading mechanism. So we can assume that the test that is currently failing is not testing cascading correctly. It's a candidate for removal.
The manual tests + additional test case provided with this change and related ones actually covers the cascading scenario better.
Comment 6 Eclipse Genie CLA 2015-12-07 10:22:29 EST 
New Gerrit change created: https://git.eclipse.org/r/62115
Comment 8 Eclipse Genie CLA 2015-12-08 04:36:28 EST 
New Gerrit change created: https://git.eclipse.org/r/62180
Comment 9 Dani Megert CLA 2015-12-10 10:28:17 EST 
(In reply to Eclipse Genie from comment #8)
> New Gerrit change created: https://git.eclipse.org/r/62180

This one was for bug 483882.