ENABLED developer edition theme is invisible in customize mode when 5 lightweight themes (lwts) were applied

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: arni2033, Assigned: jaws)

Tracking

Trunk
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open customize
2. Open https://addons.mozilla.org/en-US/firefox/themes/ , add 5 first lwts from that site
3. Open about:addons -> Appearance, apply "Developer Edition" theme
4. Switch to tab with customize opened in Step 1, click button "Themes"
5. Close customize, open customize. Click button "Themes"

AR:  Step 4, Step 5 - themes panel doesn't show enabled dev.edition theme.
ER:  Themes panel should display dev.edition theme as enabled in steps 4 and 5 or at least in Step 5.

Probably this was missed in bug 1148996, because AR is not a normal behavior of a lightweight theme.
Before bug 1094821 dev.theme button was always visible. Pushlog (between 2015-03-26 and 2015-03-27):
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=37d3dcbf23a9&tochange=e046475a75cb
(Reporter)

Updated

a year ago
No longer blocks: 1277113
(Reporter)

Updated

a year ago
Component: Untriaged → Toolbars and Customization

Comment 1

a year ago
Jared, I thought we always listed the currently selected theme, if any. Do you know why that isn't working here?
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
(Reporter)

Comment 3

a year ago
Note. What the patch attachment 8823438 [details] says:
> Always include the current lwtheme in the list of recent lightweight themes

contradicts to what GIJS said in bug 1260596 comment 3:
> We only show the last 5 themes that have been in use, mixed with the default themes,
> and the devedition theme is treated as just another lightweight theme

It seems that that logic doesn't necessarily should go away in order to fix this bug.
Flags: needinfo?(jaws)

Comment 4

a year ago
mozreview-review
Comment on attachment 8823438 [details]
Bug 1327560 - Always place the current lwtheme at the beginning of the list of recent lightweight themes to prevent it from getting truncated out.

https://reviewboard.mozilla.org/r/101958/#review102338

::: browser/components/customizableui/CustomizeMode.jsm:1398
(Diff revision 1)
>        }
>  
>        let themes = [aDefaultTheme];
>        let lwts = LightweightThemeManager.usedThemes;
> +      let currentLwt = LightweightThemeManager.currentTheme;
> +      lwts.sort((a, b) => {

Hm, this or you could do something like:

```js
let currentLwtIndex = lwts.indexOf(currentLwt);
if (currentLwtIndex > -1) {
  lwts = lwts.splice(currentLwtIndex, 1).concat(lwts);
  // Or:
  lwts.unshift(... lwts.splice(currentLwtIndex, 1));
}
```

which accomplishes the same, but avoids the O(n log n) sort. Probably doesn't matter much here, but it's also shorter, and IMO slightly more obvious about what it does (because I can never remember which way around sort directions and the 1/-1 return values of comparators correspond). Up to you.
Attachment #8823438 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 5

a year ago
(In reply to arni2033 [Please stop 'improving' Firefox] from comment #3)
> Note. What the patch attachment 8823438 [details] says:
> > Always include the current lwtheme in the list of recent lightweight themes
> 
> contradicts to what GIJS said in bug 1260596 comment 3:
> > We only show the last 5 themes that have been in use, mixed with the default themes,
> > and the devedition theme is treated as just another lightweight theme
> 
> It seems that that logic doesn't necessarily should go away in order to fix
> this bug.

I don't see how it does contradict - in bug 1260596, the point was the devedition theme wasn't currently (or even recently) in use, and so it doesn't get shown - instead you've used all the other themes so they "push out" the devedition theme.

Here, in step 3 of the STR, you're switching to it. So it's the current theme. So it should appear in the list. From a quick look, it seems internal aspects of how LightweightThemeManager treats the array of used themes vs. the array of builtin themes explains why there's an unexpected issue here with the builtin (devedition) lwtheme, and not with 'normal' lwthemes (I think?) - but I haven't looked at it in detail.
(Reporter)

Comment 6

a year ago
@ Gijs:
You're totally right, I'm sorry for not reading the patch properly and interfering with development
Flags: needinfo?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8823438 [details]
Bug 1327560 - Always place the current lwtheme at the beginning of the list of recent lightweight themes to prevent it from getting truncated out.

https://reviewboard.mozilla.org/r/101958/#review102338

> Hm, this or you could do something like:
> 
> ```js
> let currentLwtIndex = lwts.indexOf(currentLwt);
> if (currentLwtIndex > -1) {
>   lwts = lwts.splice(currentLwtIndex, 1).concat(lwts);
>   // Or:
>   lwts.unshift(... lwts.splice(currentLwtIndex, 1));
> }
> ```
> 
> which accomplishes the same, but avoids the O(n log n) sort. Probably doesn't matter much here, but it's also shorter, and IMO slightly more obvious about what it does (because I can never remember which way around sort directions and the 1/-1 return values of comparators correspond). Up to you.

Fair enough. I went with the first of yours and added a comment to explain why this is being done.

Comment 9

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08a1e5ba05cf
Always place the current lwtheme at the beginning of the list of recent lightweight themes to prevent it from getting truncated out. r=Gijs

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/08a1e5ba05cf
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.