Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Panorama button should reflect the number of groups before Panorama has launched

VERIFIED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
7 years ago
a year ago

People

(Reporter: mitcho, Assigned: raymondlee)

Tracking

Trunk
Firefox 7

Details

Attachments

(1 attachment, 5 obsolete attachments)

The Panorama button currently only accurately reflects the number of groups after Panorama has been loaded. Fix it.
Blocks: 653099
No longer depends on: 588217
(Assignee)

Comment 1

6 years ago
Created attachment 534700 [details] [diff] [review]
v1
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #534700 - Flags: feedback?(tim.taubert)
Comment on attachment 534700 [details] [diff] [review]
v1

Review of attachment 534700 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: browser/base/content/browser-tabview.js
@@ +104,5 @@
> +        data = sessionstore.getWindowValue(window, this.GROUPS_IDENTIFIER);
> +        if (data) {
> +          let parsedData = JSON.parse(data);
> +          this.updateGroupNumberBroadcaster(parsedData.totalNumber);
> +        }

We should put .getWindowValue() and JSON.parse() in a try-catch block.

@@ +391,5 @@
> +  // Updates the group number broadcaster.
> +  updateGroupNumberBroadcaster: function TabView_updateGroupNumberBroadcaster(number) {
> +    let groupsNumber = document.getElementById("tabviewGroupsNumber");
> +    groupsNumber.setAttribute("groups", number);
> +    dump("set " + number + "\n")

Nit: leftover debug statement.
Attachment #534700 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 3

6 years ago
Created attachment 534818 [details] [diff] [review]
v2
Attachment #534700 - Attachment is obsolete: true
Attachment #534818 - Flags: review?(ian)
Comment on attachment 534818 [details] [diff] [review]
v2

Review of attachment 534818 [details] [diff] [review]:
-----------------------------------------------------------------

R+ with those comments addressed

::: browser/base/content/browser-tabview.js
@@ +106,5 @@
> +          if (data)
> +            existingData = JSON.parse(data);
> +        } catch (e) {
> +          // getWindowValue will fail if the property doesn't exist
> +        }

Doesn't look like we need this getWindowValue at all; please remove.

@@ +113,5 @@
> +        if (data) {
> +          let parsedData = JSON.parse(data);
> +          this.updateGroupNumberBroadcaster(parsedData.totalNumber);
> +        }
> +

The first time this happens on a new upgrade, parsedData.totalNumber will be undefined. Rather than passing undefined into updateGroupNumberBroadcaster, pass: 

  parsedData.totalNumber || 0
Attachment #534818 - Flags: review?(ian) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 535251 [details] [diff] [review]
Patch for checkin
Attachment #534818 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/32862966ebaf
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: Future → Firefox 7
Version: unspecified → Trunk
I just realized something... aren't we already saving the number of groups in the group data? Why save that number again in a second place for this patch?

Sorry for not thinking of that sooner...
(Assignee)

Comment 8

6 years ago
(In reply to comment #7)
> I just realized something... aren't we already saving the number of groups
> in the group data? Why save that number again in a second place for this
> patch?
> 
> Sorry for not thinking of that sooner...

We can actually use Storage.readGroupItemData(win) to get all group items, and then count the number of groups.  However, user might have many different groups (huge data set) so i don't think it worth to parse all the group items data from string to JSON for just getting the number of groups. Since the user might not even enter into Panorama for the whole browsing section.

What do you think?
(In reply to comment #8)
> We can actually use Storage.readGroupItemData(win) to get all group items,
> and then count the number of groups.  However, user might have many
> different groups (huge data set) so i don't think it worth to parse all the
> group items data from string to JSON for just getting the number of groups.
> Since the user might not even enter into Panorama for the whole browsing
> section.

Yeah, that's what I thought about when reading the patch. IMHO it might be worth the additional property to not have to parse all groupItems on startup.
bugspam
Blocks: 660175
No longer blocks: 653099
(In reply to comment #9)
> (In reply to comment #8)
> > We can actually use Storage.readGroupItemData(win) to get all group items,
> > and then count the number of groups.  However, user might have many
> > different groups (huge data set) so i don't think it worth to parse all the
> > group items data from string to JSON for just getting the number of groups.
> > Since the user might not even enter into Panorama for the whole browsing
> > section.
> 
> Yeah, that's what I thought about when reading the patch. IMHO it might be
> worth the additional property to not have to parse all groupItems on startup.

Cool, good point. Just making sure it was considered.

Comment 12

6 years ago
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110601 Firefox/7.0a1

I am still able to see this issue on Mac OS X 10.6 and Win7 - the number of opened groups (one group on a clean profile) is correctly displayed only after Panorama is accessed for the first time.

Also logged Bug 661501 - No panorama icon in List all tabs drop down menu
(Assignee)

Comment 13

6 years ago
(In reply to comment #12)
> Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110601 Firefox/7.0a1
> 
> I am still able to see this issue on Mac OS X 10.6 and Win7 - the number of
> opened groups (one group on a clean profile) is correctly displayed only
> after Panorama is accessed for the first time.
>

It seems to make senese that we display zero groups before user enters Panorama for the first time.  When user enters Panorama for the first time, a group is then created.

@tim/ian: any comments?
 
> Also logged Bug 661501 - No panorama icon in List all tabs drop down menu
I think there's some confusion between the two different kinds of "entering Panorama for the first time": there's the first time in a session (when Panorama has already been set up for that particular window in a previous session), and there's the first time in a window (where Panorama has never been set up for that window). I believe this bug is about the former "first", while comment 12 is about the latter "first". 

I agree with Raymond that it's reasonable in a window that's never had Panorama set up to indicate that by displaying 0 groups in the Panorama button. On the other hand, I think ultimately we want to get to a state where, at least as far as the UI is concerned, it feels like Panorama is "always just there", not something you have to initialize; in that case, having one group lit when Panorama hasn't been initialized for a window makes sense. 

So I guess it depends on whether we feel like it's still worth indicating to the user that they've never run Panorama on a specific window.
(In reply to comment #14)
> I agree with Raymond that it's reasonable in a window that's never had
> Panorama set up to indicate that by displaying 0 groups in the Panorama
> button. On the other hand, I think ultimately we want to get to a state
> where, at least as far as the UI is concerned, it feels like Panorama is
> "always just there", not something you have to initialize; in that case,
> having one group lit when Panorama hasn't been initialized for a window
> makes sense.

+1 :)

> So I guess it depends on whether we feel like it's still worth indicating to
> the user that they've never run Panorama on a specific window.

I don't think we should make the user think too much about Panorama and espcecially its lazy-loading behavior. So switching the default value to "1" seems totally fine to me.
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

6 years ago
Created attachment 537177 [details] [diff] [review]
v3

This patch would set the number of group to 1 if user hasn't activated Panorama before.
Attachment #535251 - Attachment is obsolete: true
Attachment #537177 - Flags: feedback?(tim.taubert)
Comment on attachment 537177 [details] [diff] [review]
v3

Review of attachment 537177 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Can you please include this in browser_tabview_bug628061.js?
Attachment #537177 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 18

6 years ago
Created attachment 537210 [details] [diff] [review]
v4
Attachment #537177 - Attachment is obsolete: true
Attachment #537210 - Flags: review?(ian)
Comment on attachment 537210 [details] [diff] [review]
v4

Review of attachment 537210 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #537210 - Flags: review?(ian) → review+

Comment 20

6 years ago
If the default is 1, then is there any instance in which the "zero" state icon would be displayed? If not then we might as well chuck the default icon.

On that note I would vote that it default to a zero state.  Perhaps there's at least merit in the icon being consistent with marketing.
(Assignee)

Comment 21

6 years ago
(In reply to comment #20)
> If the default is 1, then is there any instance in which the "zero" state
> icon would be displayed? If not then we might as well chuck the default icon.
> 

Yes, when there are some orphan tabs. However, bug 654721 should remove the "zero" state.

> On that note I would vote that it default to a zero state.  Perhaps there's
> at least merit in the icon being consistent with marketing.
(Assignee)

Comment 22

6 years ago
Created attachment 537730 [details] [diff] [review]
Patch for checkin
Attachment #537210 - Attachment is obsolete: true
(Assignee)

Comment 23

6 years ago
Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=f7cff306f927
http://hg.mozilla.org/mozilla-central/rev/716fe9b93db5
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 25

6 years ago
Verified issue on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110609 Firefox/7.0a1 and also on Win 7 and Win XP. 

Issue is no longer present - setting status to Verified Fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.