Closed Bug 1327560 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: arni2033, Assigned: jaws)

References

Details

Attachments

(1 file)

>>> 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
No longer blocks: 1277113
Component: Untriaged → Toolbars and Customization
Jared, I thought we always listed the currently selected theme, if any. Do you know why that isn't working here?
Flags: needinfo?(jaws)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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 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+
(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.
@ Gijs: You're totally right, I'm sorry for not reading the patch properly and interfering with development
Flags: needinfo?(jaws)
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.
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
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.

Attachment

General

Created:
Updated:
Size: