Tab group migration page names anonymous groups "Window 1/2/3/..." instead of "Group 1/2/3..." and does not sort named groups first

VERIFIED FIXED in Firefox 46

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: brindusat, Assigned: Gijs)

Tracking

45 Branch
Firefox 46
x86_64
Windows 7

Details

MozReview Requests

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

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8696671 [details]
Window_TabGroupName.png

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.
(Reporter)

Updated

2 years ago
Blocks: 1221050
(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

2 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)
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)
Created attachment 8696925 [details]
TheGroupsOrder1.png

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: 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
Created attachment 8698171 [details]
MozReview Request: Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r?mconley

Bug 1231114 - fix labels of unnamed groups as well as group ordering on the tab migration page, r?mconley
Attachment #8698171 - Flags: review?(mconley)
Attachment #8698171 - Flags: review?(mconley) → review+
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".
(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 8

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/f8840e2aec5a

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f8840e2aec5a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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 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

2 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
status-firefox46: fixed → verified
(Reporter)

Comment 13

2 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

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb00a862858b
status-firefox45: affected → fixed
(Reporter)

Comment 15

2 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
status-firefox45: fixed → verified
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.