Bug 546140 - Remove ability to set MRU via CSS
Summary: Remove ability to set MRU via CSS
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.11   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.17 M3   Edit
Assignee: Lars Vogel CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 563540
  Show dependency tree
 
Reported: 2019-04-05 05:33 EDT by Lars Vogel CLA
Modified: 2020-07-31 05:34 EDT (History)
4 users (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 Lars Vogel CLA 2019-04-05 05:33:40 EDT 
Dark theme specifies 

swt-mru-visible: true;

IIRC Andrey added MRU as preference which has priority over CSS. So I think we should remove it from the CSS.
Comment 1 Lars Vogel CLA 2019-04-05 05:36:41 EDT 
Actuall all themes specifies this. I create a change to remove it and hope that Andrey has time to respond. As I'm ignoring the existing of the MRU setting, I'm a bad judge if this change will affect MRU haters and lovers.
Comment 2 Eclipse Genie CLA 2019-04-05 05:37:21 EDT 
New Gerrit change created: https://git.eclipse.org/r/140097
Comment 3 Dani Megert CLA 2019-04-05 09:53:54 EDT 
(In reply to Lars Vogel from comment #1)
> Actuall all themes specifies this. I create a change to remove it and hope
> that Andrey has time to respond. As I'm ignoring the existing of the MRU
> setting, I'm a bad judge if this change will affect MRU haters and lovers.
How can you provide a change of which you do not know the impact?
Comment 4 Lars Vogel CLA 2019-04-05 10:46:58 EDT 
(In reply to Dani Megert from comment #3)
> (In reply to Lars Vogel from comment #1)
> > Actuall all themes specifies this. I create a change to remove it and hope
> > that Andrey has time to respond. As I'm ignoring the existing of the MRU
> > setting, I'm a bad judge if this change will affect MRU haters and lovers.
> How can you provide a change of which you do not know the impact?

By asking the expert (Andrey) for his opinion.
Comment 5 Andrey Loskutov CLA 2019-04-05 10:52:25 EDT 
(In reply to Lars Vogel from comment #4)
> 
> By asking the expert (Andrey) for his opinion.

I'm not expert in themes, I always have them disabled.
Comment 6 Lars Vogel CLA 2019-04-05 10:53:43 EDT 
(In reply to Andrey Loskutov from comment #5)
> (In reply to Lars Vogel from comment #4)
> > 
> > By asking the expert (Andrey) for his opinion.
> 
> I'm not expert in themes, I always have them disabled.

Didn't you implement that the preference overrides the CSS setting?
Comment 7 Dani Megert CLA 2019-04-05 10:54:21 EDT 
(In reply to Andrey Loskutov from comment #5)
> (In reply to Lars Vogel from comment #4)
> > 
> > By asking the expert (Andrey) for his opinion.
> 
> I'm not expert in themes, I always have them disabled.
Ha!
Comment 8 Lars Vogel CLA 2019-04-05 11:01:01 EDT 
(In reply to Dani Megert from comment #7)
> (In reply to Andrey Loskutov from comment #5)
> > (In reply to Lars Vogel from comment #4)
> > > 
> > > By asking the expert (Andrey) for his opinion.
> > 
> > I'm not expert in themes, I always have them disabled.
> Ha!

Ha ha? Let's stay constructive
Comment 9 Andrey Loskutov CLA 2019-04-05 11:05:50 EDT 
(In reply to Lars Vogel from comment #6)
> (In reply to Andrey Loskutov from comment #5)
> > (In reply to Lars Vogel from comment #4)
> > > 
> > > By asking the expert (Andrey) for his opinion.
> > 
> > I'm not expert in themes, I always have them disabled.
> 
> Didn't you implement that the preference overrides the CSS setting?

Right, I did this long time ago, see commit https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=ead0d552a9cadb8b02bab895e18c51a8c78a2235 for bug 388476. So by default those aren't relevant anymore, except user changes the default.
Comment 10 Lars Vogel CLA 2019-04-05 11:13:13 EDT 
(In reply to Andrey Loskutov from comment #9)
> (In reply to Lars Vogel from comment #6)
> > (In reply to Andrey Loskutov from comment #5)
> > > (In reply to Lars Vogel from comment #4)
> > > > 
> > > > By asking the expert (Andrey) for his opinion.
> > > 
> > > I'm not expert in themes, I always have them disabled.
> > 
> > Didn't you implement that the preference overrides the CSS setting?
> 
> Right, I did this long time ago, see commit
> https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/
> ?id=ead0d552a9cadb8b02bab895e18c51a8c78a2235 for bug 388476. So by default
> those aren't relevant anymore, except user changes the default.

Thanks Andrey, surprised that is already that long ago, feels like short term to me. 

I will have a closer look next week to see if we can safefy remove the CSS for the default. IIRC you argued that CSS is misuse for this case to which I agree. We could actually also style the preference via CSS if required.
Comment 11 Lars Vogel CLA 2019-07-05 11:13:31 EDT 
No time for detailed testing for this trivial change.
Comment 12 Lars Vogel CLA 2019-11-06 12:04:17 EST 
CSS engine is not API, so we should simplify remove the CSS handler. See CSSPropertyMruVisibleSWTHandler and its usage.
Comment 13 Lars Vogel CLA 2019-11-06 12:04:22 EST 
CSS engine is not API, so we should simplify remove the CSS handler. See CSSPropertyMruVisibleSWTHandler and its usage.
Comment 14 Eclipse Genie CLA 2020-07-20 07:21:44 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/166517
Comment 15 Eclipse Genie CLA 2020-07-20 08:14:39 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/166522