arni2033 Reporter
	Description • 7 years 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

	:Gijs (he/him)
	Comment 1 • 7 years ago

Jared, I thought we always listed the currently selected theme, if any. Do you know why that isn't working here?

	arni2033 Reporter
	Comment 3 • 7 years 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.

	:Gijs (he/him)
	Comment 4 • 7 years 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.

	:Gijs (he/him)
	Comment 5 • 7 years 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.

	arni2033 Reporter
	Comment 6 • 7 years ago

@ Gijs:
You're totally right, I'm sorry for not reading the patch properly and interfering with development

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 8 • 7 years 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.

	Pulsebot
	Comment 9 • 7 years 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

	Carsten Book [:Tomcat]
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/08a1e5ba05cf