Bugzilla – Full Text Bug Listing
| Summary: | Define proper API for hover/info color constants | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Markus Keller <markus.kell.r> |
| Component: | UI | Assignee: | Leo Ufimtsev <lufimtse> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P2 | CC: | akurtakov, daniel_megert, eclipse.sprigogin, gautier.desaintmartinlacaze, jonah, Lars.Vogel, lufimtse, markus.kell.r, noopur_gupta, peter |
| Version: | 4.8 | Flags: | daniel_megert:
review-
|
| Target Milestone: | 4.8 M3 | ||
| Hardware: | PC | ||
| OS: | All | ||
| URL: | https://etherpad.openstack.org/p/EclipseBug508819 | ||
| See Also: |
https://git.eclipse.org/r/96122 https://bugs.eclipse.org/bugs/show_bug.cgi?id=516420 https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=288d9edade4aa92b619e472c34edcd7173d15453 https://git.eclipse.org/r/96117 |
||
| Whiteboard: | |||
| Bug Depends on: | 505738, 508498, 516533 | ||
| Bug Blocks: | 501742, 507072, 508793, 513978, 515755, 516364, 516515 | ||
|
Description
Markus Keller
JFacePreferences looks like the right place to declare the constants. Thank you for the bug submission. (In reply to Markus Keller from comment #0) > +++ This bug was initially created as a clone of Bug #505738 +++ > > Fix the APIs introduced with bug 505738. > > IDs that are declared in a plugin.xml and that are supposed to be used by > Java code need to be declared as Java API constants (e.g. in an @noimplement > interface). > > Java code should always refer to that constant and should not embed magic > string literals. I see. I will work on that. > > org.eclipse.ui.workbench.HOVER_FOREGROUND/BACKGROUND are poor IDs for colors > that are called "info" colors everywhere else. Fix the ID. Yea, it was on my mind to change the name of those IDs. Thank you for pointing this out. > The <colorDefinition> contributions are missing <description> elements that > describe the usage of these colors. (In reply to Markus Keller from comment #1) > JFacePreferences looks like the right place to declare the constants. Thank you for taking the time and making the suggestion. I will investigate this class. At the moment I'm waiting for code un-freeze next week and for the following patch to be reviewed first: Bug 508498 – Add "information" color api *** Bug 508476 has been marked as a duplicate of this bug. *** Awaiting code de-freeze. Mass move. Please move back to M6, if necessary Investigating this now. Not sure if I'm suppose to rename the existing ID or mark current id as deprecated and create a new one. For now, since the usage of this ID is very limited (platform.ui + jdt), I'll rename this business and fix up the relevant usages (let me know if it should be done otherwise). New Gerrit change created: https://git.eclipse.org/r/96117 (In reply to Eclipse Genie from comment #8) > New Gerrit change created: https://git.eclipse.org/r/96117 I first need to decouple jdt from using HOVER_ to avoid breakage. New Gerrit change created: https://git.eclipse.org/r/96122 New Gerrit change created: https://git.eclipse.org/r/96123 Patch 1,2,3 written to address bug submission. Awaiting reviews. We might consider bumping api for jFace/platform ui as we're renaming api here. (part 2). But I'm not sure about api/version bumping guidelines. Will investigate a solution with backwards compatability. (In reply to Leo Ufimtsev from comment #14) > Will investigate a solution with backwards compatability. Implemented part 2 (platform.ui change), such that it has backwards compatibility. Awaiting patch review: https://git.eclipse.org/r/#/c/96122/ New Gerrit change created: https://git.eclipse.org/r/96608 (In reply to Eclipse Genie from comment #16) > New Gerrit change created: https://git.eclipse.org/r/96608 Duplicate. Ignore. I created a separate task to weed-out old API from JDT: Bug 516364 – Replace use of HOVER_ color with INFORMATION_ So for now, only the one patch is relevant to solving this bug: https://git.eclipse.org/r/96122 (In reply to Leo Ufimtsev from comment #18) > So for now, only the one patch is relevant to solving this bug: > https://git.eclipse.org/r/96122 NOTE: While testing I found that the annotation hover status line is still broken. I would rather prefer to get a fix for that. public class Test { private String s; } Change the Information background color. Hover over 's' ==> status line has wrong color. (In reply to Dani Megert from comment #19) > NOTE: While testing I found that the annotation hover status line is still > broken. I would rather prefer to get a fix for that. > > public class Test { > private String s; > } > > Change the Information background color. Hover over 's' ==> status line has > wrong color. ==> Bug 516420. Currently awaiting for the following to be merged: Bug 516533 – Bug: isEditable="false" colors/fonts can still be edited if referenced by other colors Which should resolve outstanding concerns about this patch. Will followup with this later. Status update: Dependents resolved. I rebased the fix and requested reviewers to review patch again. ~Awaiting review. Marking org.eclipse.ui.workbench.HOVER_BACKGROUND/FOREGROUND with isEditable="false" is a problem for clients that already refer to the constant. With the colorFactory, the value now gets reverted to the defaults.
=> Those users who felt a need to configure the color are now broken again, unless they're lucky to have a separate color definition they can adjust (e.g. Javadoc colors, which currently inherit the deprecated colors).
The right thing to do would be to declare e.g.:
<colorDefinition
categoryId="org.eclipse.ui.workbenchMisc"
id="org.eclipse.ui.workbench.HOVER_BACKGROUND"
isEditable="false"
defaultsTo="org.eclipse.ui.workbench.INFORMATION_BACKGROUND"
label="%Color.hoverBackgroundDeprecated">
<description> %Color.hoverBackgroundDeprecatedDesc</description>
</colorDefinition>
However, this neither seems to work in the Preferences UI:
- HOVER_BACKGROUND is wrongly shown in the UI
- Javadoc colors' "Go to / Edit Default" buttons don't work,
nor does it let HOVER_BACKGROUND inherit the color from INFORMATION_BACKGROUND. If those two problems can be solved, then that's the way to go.
Otherwise, use the https://git.eclipse.org/r/#/c/96122/9 I just pushed, which corrects the bundle versions and some Javadocs. Since the INFORMATION_* colors were only added in 4.7 and we now have a good alternative for clients, I would accept some loss of functionality here.
(In reply to Markus Keller from comment #23) > Marking org.eclipse.ui.workbench.HOVER_BACKGROUND/FOREGROUND with > isEditable="false" is a problem for clients that already refer to the > constant. With the colorFactory, the value now gets reverted to the defaults. > > => Those users who felt a need to configure the color are now broken again, > unless they're lucky to have a separate color definition they can adjust > (e.g. Javadoc colors, which currently inherit the deprecated colors). > > The right thing to do would be to declare e.g.: > > <colorDefinition > categoryId="org.eclipse.ui.workbenchMisc" > id="org.eclipse.ui.workbench.HOVER_BACKGROUND" > isEditable="false" > defaultsTo="org.eclipse.ui.workbench.INFORMATION_BACKGROUND" > label="%Color.hoverBackgroundDeprecated"> > <description> %Color.hoverBackgroundDeprecatedDesc</description> > </colorDefinition> > > However, this neither seems to work in the Preferences UI: > - HOVER_BACKGROUND is wrongly shown in the UI > - Javadoc colors' "Go to / Edit Default" buttons don't work, > nor does it let HOVER_BACKGROUND inherit the color from > INFORMATION_BACKGROUND. If those two problems can be solved, then that's the > way to go. > > Otherwise, use the https://git.eclipse.org/r/#/c/96122/9 I just pushed, > which corrects the bundle versions and some Javadocs. Since the > INFORMATION_* colors were only added in 4.7 and we now have a good > alternative for clients, I would accept some loss of functionality here. Thank you for taking the time to review this patch. I had as similar thought. I experimented with defaultsTo while doing the patch, but the problem that I ran into is that "HOVER_" is then shown as the child of "INFORMATION_", where as the goal is to remove visibility of HOVER_ altogether. I haven't found a way to prevent this other than defining a color of it's own. At the moment, at least in Eclipse, the only user of HOVER_* is JDT's javadoc, which has it's own color definition and will be switched to INFORMATION_ after patch submission. So hopefully shouldn't affect anyone. If it does, they can easily switch over to INFORMATION_. Thank you for updating bundle versions and javadocs. Should we go ahead with merge? Afaik current outstanding concerns are addressed. ~Awaiting review/merge of patch to move on and fix outstanding color issues. ~Awaiting patch review/merge. Currently pending till I finish off webkit2 port: Bug 510905 – [GTK3][Webkit2] Implement webkit2 support for browser function (Part 2: Java return a value from callback.) Notes to self: - Rebase patch (merge conflict), prep for new review. Thanks Leo for this change to handle Markus concerns. Looks very good to me and I feel ashamed that it took me so long to review this change. Sorry for the delay and thanks for this change. Gerrit change https://git.eclipse.org/r/96122 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=288d9edade4aa92b619e472c34edcd7173d15453 (In reply to Lars Vogel from comment #28) > Thanks Leo for this change to handle Markus concerns. Looks very good to me > and I feel ashamed that it took me so long to review this change. Sorry for > the delay and thanks for this change. Thanks for looking into this. Timing is good, as I first have to finish webkit2 port before I can finish the remaining color fix tasks. |