Closed
Bug 1231114
Opened 9 years ago
Closed 9 years ago
Tab group migration page names anonymous groups "Window 1/2/3/..." instead of "Group 1/2/3..." and does not sort named groups first
Categories
(Firefox Graveyard :: Panorama, defect)
Tracking
(firefox45 verified, firefox46 verified)
VERIFIED
FIXED
Firefox 46
People
(Reporter: btot, Assigned: Gijs)
References
Details
Attachments
(3 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
mconley
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0 Nightly 44 build from 11.29.2015 migrated to Nightly 45 buildId 20151203030226 Steps to reproduce: 1. Install Firefox 44 2. Create several Panorama tab groups: two with specified names, and two with no name. 3. Update to Firefox 45 Expected results: The "Tab Groups are no more. Sorry." page is opened and the tab groups are displayed with correct names. Actual results: The "Tab Groups are no more. Sorry." page is opened, but the groups with no name are displayed as Window 1 and Window 2.
Assignee | ||
Comment 1•9 years ago
|
||
(In reply to Brindusa Tot from comment #0) > Expected results: > the tab groups are displayed with correct names. > Actual results: > the groups with no name are displayed as Window 1 and Window 2. That *is* the correct name, if they had no name to begin with. What name were you expecting?
Flags: needinfo?(brindusa.tot)
Reporter | ||
Comment 2•9 years ago
|
||
Sorry for not being clear. As specified in bug 1221050 comment 16, the tab groups that don't have name should be named "Tab Group 1", "Tab Group 2" and so on. Please see in the screenshot attached that in the bookmarks, the tab groups without names are named as "Group 1" and in the "Tab Groups are no more. Sorry." page are named Windows 1(attachment: https://bugzilla.mozilla.org/attachment.cgi?id=8696671)
Flags: needinfo?(brindusa.tot)
Assignee | ||
Comment 3•9 years ago
|
||
So the bookmark labels are correct and not the subject of this bug, but the restoration page is reusing about:sessionrestore's labels here, and I don't think it's worth extra engineering work in order to call these extra things "Group" rather than "Window" if they do get restored as windows. Getting confirmation from Philipp.
Flags: needinfo?(philipp)
Comment 4•9 years ago
|
||
The order of the groups from the "Tab Groups are no more. Sorry." page is also confusing. For instance, I created 11 groups and I only named 6 of them. When they appear in the restoration page they are all mixed up (in the Bookmarks folder the first ones are named and are followed by the un-named ones).
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(philipp)
Summary: Tab Groups do not have correct names on the informing page: "Tab Groups are no more" → Tab group migration page names anonymous groups "Window 1/2/3/..." instead of "Group 1/2/3..." and does not sort named groups first
Assignee | ||
Comment 5•9 years ago
|
||
Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r?mconley
Attachment #8698171 -
Flags: review?(mconley)
Updated•9 years ago
|
Attachment #8698171 -
Flags: review?(mconley) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8698171 [details] MozReview Request: Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r?mconley https://reviewboard.mozilla.org/r/27849/#review25081 This looks okay to me (modulo nits). Thanks Gijs! ::: browser/modules/TabGroupsMigrator.jsm:121 (Diff revision 1) > + groupData.tabGroupsMigrationTitle = Trailing ws ::: browser/modules/test/xpcshell/test_TabGroupsMigrator.js:173 (Diff revision 1) > - Assert.equal(group2.tabGroupsMigrationTitle, "", "We don't assign titles to untitled groups"); > + Assert.equal(group2.tabGroupsMigrationTitle, "Group 1", "We assign a numeric title to untitled groups"); So is the idea that even though the internal groupID of this group was "2", we assign it the title of "Group 1" because it was the first unnamed group we saw? If so, let's call this out explicitly in a comment above this assertion, since it's a bit strange to make sure that group2 has title "Group 1".
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) > ::: browser/modules/test/xpcshell/test_TabGroupsMigrator.js:173 > (Diff revision 1) > > - Assert.equal(group2.tabGroupsMigrationTitle, "", "We don't assign titles to untitled groups"); > > + Assert.equal(group2.tabGroupsMigrationTitle, "Group 1", "We assign a numeric title to untitled groups"); > > So is the idea that even though the internal groupID of this group was "2", > we assign it the title of "Group 1" because it was the first unnamed group > we saw? If so, let's call this out explicitly in a comment above this > assertion, since it's a bit strange to make sure that group2 has title > "Group 1". Good catch. Will do.
Comment 9•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f8840e2aec5a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8698171 [details] MozReview Request: Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r?mconley > Approval Request Comment > [Feature/regressing bug #]: tab groups migration code > [User impact if declined]: sorting and naming issues in the tab groups > migration page > [Describe test coverage new/current, TreeHerder]: has tests > [Risks and why]: pretty low risk because of the tests, and also, aurora only > just branched > [String/UUID change made/needed]: no - I'm reusing existing strings so this > is safe for aurora.
Attachment #8698171 -
Flags: approval-mozilla-aurora?
Comment 11•9 years ago
|
||
Comment on attachment 8698171 [details] MozReview Request: Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r?mconley Has tests, feature removed
Attachment #8698171 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter | ||
Comment 12•9 years ago
|
||
Verified as fixed on Firefox Nightly 46(upgrade from Nightly 44 to Nightly 46) on following OSes: Windows 7 x64: buildID: 20151220030223, Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 Ubuntu 14.04 x64: buildID: 20151221004011, Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 Mac 10.10: buildID 20151221030239, Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:46.0) Gecko/20100101 Firefox/46.0
Reporter | ||
Comment 13•9 years ago
|
||
(In reply to Brindusa Tot from comment #12) > Verified as fixed on Firefox Nightly 46(upgrade from Nightly 44 to Nightly > 46) on following OSes: > Windows 7 x64: buildID: 20151220030223, Mozilla/5.0 (Windows NT 6.1; WOW64; > rv:46.0) Gecko/20100101 Firefox/46.0 By mistake added the Aurora Agent > Ubuntu 14.04 x64: buildID: 20151221004011, Mozilla/5.0 (X11; Linux x86_64; > rv:45.0) Gecko/20100101 Firefox/45.0 Actually verified on Nightly 46: BuildID: 20151220030223, Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0 > Mac 10.10: buildID 20151221030239, Mozilla/5.0 (Macintosh; Intel Mac OS X > 10.10; rv:46.0) Gecko/20100101 Firefox/46.0
Comment 14•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb00a862858b
Reporter | ||
Comment 15•9 years ago
|
||
Verified as fixed on Aurora builds, on following Oses: Windows 7 x64: 45.0a2 Build ID 20151229004007 - Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0 Ubuntu 14.04 x32: 45.0a2 Build ID20151228004010 - Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0 Mac 10.10: 45.0a2 Build ID 20151229004007 - User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0
Status: RESOLVED → VERIFIED
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•