Bug 726560 - Add support of Panorama group name (FF10+) for titlebar customization #19

xabolcs · 2012-02-29T19:05:29Z

Hi!

This is the pull request that contains the patch from Bug 726560.

Before reviewing the diff, please read Comment 4 and 5 in Bugzilla!

…customization

xabolcs · 2012-03-09T21:15:20Z

Rebased to top of reorg merge.

whimboo · 2012-03-12T14:48:19Z

extension/chrome/locale/en-US/variables.properties

@@ -52,5 +52,6 @@ variable.Processor.description=Compilation Processor
 variable.Compiler.description=Compiler
 variable.DefaultTitle.description=Default Application Title
 variable.TabTitle.description=Current Tab's Title
+variable.ActiveTabGroupName.description=Active TabView group name - may be empty in rare cases


Please reduce the variable to 'TabGroup' so we stay in sync with 'TabTitle' and others. Also please kill the last part of the description content and use 'Tab Group' instead of 'TabView', e.g. 'Current Tab Group'.

… startup

xabolcs · 2012-03-13T01:32:28Z

Assigned some of @whimboo's review comments.

Unaddressed:

What happens if no tab group exists anymore?

Nothing special. ${TabGroup} will be empty.

Would we carry around the last name forever? Or is there at least one group in any case?

If we thinking about it strictly, then yes, we would carry that forever. But as I said above, if
user changes TabGroup (open / close / rename a group) then ${TabGroup} will follow the last active group.

As I see, in FF11, there will be always a TabGroup. Could somebody provide it's Bugzilla number?

In FF4.2a1pre user is able to close all the groups and leave tabs alone. The tab bar would contain only one tab in that case, if user exits from the TabView.

When will those values be deleted from the session store file?

I don't know sessionStore. IMHO if we don't save the value at shutdown, it wouldn't be saved, which is an implicit delete.
Should I discard empty nightlyApp.tabGroupTitle values? In other words: should I test it's emptiness before save?

Can you please get a reference to this service once and store it globally.

I introduced some other getService() in these v2 commits.
So in which global would you ask to store the reference? In nightly or in (browser.js') nightlyApp? Or just in window? ;)
Anyway, could we defer this after the landing of #6? Importing Services.jsm would be enough then, wouldn't be?

Please consider that I would like to fold these commits before pull!
Please give me a day or two to make it clean after r+!

whimboo · 2012-03-13T10:29:25Z

I know it's a bit harder for you but I would love it if we could keep up with the conversation at the right places and don't combine all in a single reply. It's hard to follow-up on changes and questions.

Here some answers and questions:

Have you tested that we correctly handle the TabGroup name across windows? Each window has its own set of groups
Not sure of a bug which has changed the behavior of a default group. But that happened a while ago
Re: sessionstore. It should already have the name of the group included per default. Can't we simply retrieve it?
We could defer the Services issue for now, that's fine with me

whimboo · 2012-03-13T10:34:22Z

extension/chrome/content/browser.js

+    .getService(Ci.nsIObserverService).addObserver({
+      observe: function NightlyTT_Restore() {
+        Cc["@mozilla.org/observer-service;1"]
+        .getService(Ci.nsIObserverService).removeObserver(this, "sessionstore-windows-restored");


Please don't call getService twice here but cache it locally right before:

var obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService)
obs.addObserver(...);

Same applies to the SessionStore service below.

Have you tested that we correctly handle the TabGroup name across windows? Each window has its own set of groups

Not tested before, but a quick test shows something: only the registering window's TabGroup title is restored.
Because I don't iterate through all the windows. :)

But I don't sure about the need of removeObserver. And related: how to take care Private Browsing...?

Any observer we register we have to remove.

Any observer we register we have to remove.

Yes, this is obvious. But when to remove?
On startup of the observer code? On application shutdown? On entering PB mode .... :)

It depends on when the code gets called. If it's during the startup phase then we can do it on shutdown. Otherwise it has to be done before possible references get invalid.

Hi!

Please review the v3 commits, not the v2! :)
#19 (commits)

By the way, thank you for being "r?"!

Szabolcs

2012/3/19 Henrik Skupin
reply@reply.github.com:

It depends on when the code gets called. If it's during the startup phase then we can do it on shutdown. Otherwise it has to be done before possible references get invalid.

Reply to this email directly or view it on GitHub:
https://github.com/mozilla/nightlytt/pull/19/files#r573457

xabolcs · 2012-03-13T11:05:20Z

Thank You for the answers and the questions!

xabolcs · 2012-03-13T11:24:31Z

extension/chrome/content/browser.js

@@ -66,6 +95,37 @@ init: function()

  tabbrowser.updateTitlebar = nightly.updateTitlebar;
  tabbrowser.addEventListener("DOMTitleChanged", nightly.updateTitlebar, false);
+


Re: sessionstore. It should already have the name of the group included per default. Can't we simply retrieve it?

If I move these lines some line below, into the if ... else then getting and setting of tabGroupTitle would be in sync.
After that I wouldn't understand Your question.
Would You mind to write some details about Your question? Because I don't really know why are You asking that.

The question was about our own sessionstore entry for NTT. Per default the sessionstore.js file will contain the list of all the windows, tabs, and groups. So if we would query the session store service we probably could retrieve the last active group on startup, whereby we wouldn't have to store our own property.

Still not clear. :)

Are You talking about L#44, LAST_SESSION_GROUP_NAME_IDENTIFIER: "nightlytt-last-session-group-name", ?
I wouldn't like to collide TabView's property if we are before Bug 682996. In that case I plan simply use TabView's stuff, and do not save / restore ours.

I don't think that we should care about code before bug 682996 has been fixed. It was fixed for Firefox 10 which we still support on the ESR branch. Releases before we do not support anymore. 3.6.x is an exception but will also be dropped soon.

whimboo · 2012-03-14T10:57:25Z

I don't think this enhancement its worth delaying 3.2.2. Lets push it to 3.2.3.

xabolcs · 2012-03-19T08:57:44Z

Sorry @whimboo, but I couldn't keep this status update in the diffs.

Updated pull (with a debug "build"):

refactored TabGroup support to JS Code module, due it's size
not addressed "simply retrive"
checked against multiple windows
unable to address Private Browsing (with multiple windows)

Could somebody help me?
STR:

start Firefox (recommended with -console)
open a new window
name all your groups
start Private Browsing
browse a little
stop Private Browing
note that the other (background) windows won't get their TabGroup
initialize their TabView: click on "Group your tabs", "move to group"
examine log messages

I'm going to investigate the "simple retrive". (See comment: #19 (comment) )

xabolcs · 2012-03-19T09:17:22Z

unable to address Private Browsing (with multiple windows)

Reason seems to be: SessionStore likes only startup and shutdown.
Isn't reliable to store title's when entering PB. :(
Are there any other way to store and load titles in case of PB?

xabolcs · 2012-03-19T09:19:14Z

Updated pull (with a debug "build"):

In summary: TabGroup feature is complete except Private Browsing compatibility.

EDIT:
It's worth to test TabView's own implementation about this in FF 9!

whimboo · 2012-03-19T10:56:45Z

Re: private browsing, we could observe the transition events and react accordingly. Those are independent from the session restore notifications.

jwir3 · 2012-09-27T14:54:14Z

Is this going to land? It would be really nice to have!

xabolcs · 2012-09-27T18:30:57Z

jwir3: all the reviewer guys are busy, so not really. :(

jwir3 · 2012-09-27T19:00:22Z

xabolcs:

In the meantime, then, have you considered developing your own plugin for this and distributing it separately? If we can't get it into NTT, then there doesn't seem to be any reason not to push it from a 3rd party perspective.

xabolcs · 2012-09-27T19:55:45Z

jwir:
hmmm, You're right.
There are a Nighly Tester Tools Resurrection named thread on MozillaZine.
Some months ago I posted there an unofficial release of NTT 3.4pre.

So I will update that soon, and would pack another one which has got this feature too.

xabolcs · 2012-09-28T21:11:57Z

jwir3: see comment on MozillaZine! I made it for You! :)

tonymec · 2012-09-29T02:55:27Z

FWIW: on 26 Sep. three's been an update on https://github.com/mozilla/nightlytt

Nothing yet at AMO since 2 May.

whimboo · 2013-02-23T00:29:40Z

In https://bugzilla.mozilla.org/show_bug.cgi?id=836758 the panorama feature will be removed from Firefox. It will become an extension. So I don't believe that this pull is helpful anymore.

xabolcs · 2013-02-23T08:56:02Z

You are wrong, IMHO. Do you use tab groups?

I believe it could be very useful.
See @jwir3's comment in this pull, and the others in the original Bug 682996!
And please note, this pull (in it's current status - last commit is almost a year old) supports Fx4 ... Fx21!
Did you check NTT's statistics on AMO by Application?

Of course, you are right, this pull needs update if Bug 836758 lands.
That's why I filed issue #118.

xabolcs · 2013-02-23T18:36:00Z

Btw try build + addon + this pull works as expected.
No update needed. :)

So I really don't know why did you think that this pull isn't helpful anymore.

whimboo · 2013-02-25T10:56:06Z

Please run the current version of this pull without the panorama ad-on installed. Does it break NTT?

Also how many people will still use panorama when it has been removed from Firefox core? There is a real low number as given on bug 836758, and this would also apply to users of NTT. You might want to start a survey on mozillazine if you want to figure that out. If there are only a few people why should we maintain such a feature in NTT which also relies on another add-on? Please keep that in mind.

xabolcs · 2013-02-25T11:04:47Z

Please run the current version of this pull without the panorama ad-on installed. Does it break NTT?

It works like you run with Fx 3.6. See below:

// TabView isn't implemented
if (!("TabView" in win))
  return;

It will show "Undefined".

If there are only a few people why should we maintain such a feature in NTT which also relies on another add-on? Please keep that in mind.

I happily do the maintenance work.

whimboo · 2013-03-11T23:09:34Z

In this case I'm fine with it. But shouldn't we move out this code into its own sub module? Given that it's not supported by default it's distracting to see when we are working on the code around it.

xabolcs · 2013-06-20T20:39:35Z

Sad but true: this is still open :(
Unticking from v3.6.

whimboo · 2013-07-22T20:55:43Z

extension/chrome/content/browser.js

+  {
+    return nightlyApp.getTabGroupTitle(window);
+  } 
+  catch (e) { }


I don't see where ´getTabGroupTitle´ throws an exception. Can you please explain that? If we silently catch that, we might already want to do that in that method.

Is getTabGroupTitle defined in SeaMonkey? AFAIK, there are no tab groups there at all (yet). Or do we never come here in SeaMonkey (however braindead the user's actions)?

As you can see, nightlyApp.getTabGroupTitle() is only defined in browser.js === Firefox.
Therefore the current implementation needs the try ... catch. (Of course it could be improved, to avoid try ... catch)
BTW, only Firefox supports TabGroups (but the dev guys are going to extract it to an addon).

whimboo · 2013-07-22T21:13:53Z

With the updates made, can we get a new XPI so users (on mozillazine?) could test it?

xabolcs · 2013-07-25T08:32:12Z

With the updates made, can we get a new XPI so users (on mozillazine?) could test it?

Hi!
Thank you for the review!

Sadly, I don't have enough time on this, so I'm unable to address your comments soon. :(
Which implies this pull won't merged before v3.6 release date.
But what is delayed may come later. :)

xabolcs · 2013-10-13T15:57:52Z

extension/chrome/locale/en-US/variables.properties

@@ -52,5 +52,6 @@ variable.Processor.description=Compilation Processor
 variable.Compiler.description=Compiler
 variable.DefaultTitle.description=Default Application Title
 variable.TabTitle.description=Current Tab's Title
+variable.TabGroup.description=Current Tab Group


As you know, we have zh-CN locale too. Please merge from master!

Missing translation entity

Warning: Localizations must include a translated copy of each entity from each file in the reference locale. The required files may vary from target application to target application.

Missing Entities: variable.PlatformChangeset.description, variable.TabGroup.description
chrome/locale/zh-CN//chrome/locale/zh-CN/variables.properties

Please do the merge yourself and ensure that all is included. Thanks.

xabolcs · 2016-08-28T20:01:28Z

This needs a rework, definitely. As Tab Groups was removed from Firefox but continues as addons! 👍

I hope they expose some API to achive this feature:

xabolcs · 2016-08-28T20:22:56Z

extension/modules/tabGroupTitle.jsm

+
+  // TabView isn't implemented
+  if (!("TabView" in win))
+    return;


("TabView" in win) is false for Simplified Tab Groups

xabolcs · 2017-10-11T09:13:51Z

This PR is fully outdated now. Closing.

Group name in the titlebar is still a good feature IMHO. (#152 is related in some degree)

xabolcs mentioned this pull request Mar 9, 2012

Reorganize repository and documentation. Fix #22 #28

Merged

Bug 726560 - Add support of Panorama group name (FF10+) for titlebar …

a14ef5f

…customization

whimboo reviewed Mar 12, 2012
View reviewed changes

xabolcs added 4 commits March 12, 2012 23:31

Bug 726560 - v2: renaming activeTabGroupName

bcdc712

Bug 726560 - v2: leave tabGroup empty

57e2da6

Bug 726560 - v2: trying to get saved tab group title without delaying…

7dbf770

… startup

Bug 726560 - v2: commenting saveActiveGroupName

e52f841

whimboo reviewed Mar 13, 2012
View reviewed changes

xabolcs reviewed Mar 13, 2012
View reviewed changes

xabolcs mentioned this pull request Mar 13, 2012

Add 'conditional separator' variable to titlebar customization #34

Closed

xabolcs added 4 commits March 19, 2012 08:24

Bug 726560 - v3: introducing tabGroupTitle.jsm

91017e7

Bug 726560 - v3: addObserver PB exit

5f86f3d

Bug 726560 - v3: listening PB exit

2b7bfbf

Bug 726560 - v3: NTT logging

2853a69

xabolcs mentioned this pull request Oct 19, 2012

Re-license titlebar.css under MPL2 #98

Closed

xabolcs mentioned this pull request Oct 31, 2012

Prepare to release version 3.4. Helps to fix Issue #95. #97

Closed

whimboo reviewed Jul 22, 2013
View reviewed changes

xabolcs added a commit to xabolcs/nightlytt that referenced this pull request Aug 6, 2013

Merge pull mozilla#19 - Tab groups

b0b5072

xabolcs reviewed Oct 13, 2013
View reviewed changes

Merge master

3c7f334

xabolcs reviewed Aug 28, 2016
View reviewed changes

xabolcs force-pushed the master branch from 5377c91 to b4e77b9 Compare May 13, 2017 23:35

xabolcs closed this Oct 11, 2017

		@@ -66,6 +95,37 @@ init: function()

		tabbrowser.updateTitlebar = nightly.updateTitlebar;
		tabbrowser.addEventListener("DOMTitleChanged", nightly.updateTitlebar, false);

Bug 726560 - Add support of Panorama group name (FF10+) for titlebar customization #19

Bug 726560 - Add support of Panorama group name (FF10+) for titlebar customization #19

Conversation

xabolcs commented Feb 29, 2012

xabolcs commented Mar 9, 2012

Choose a reason for hiding this comment

xabolcs commented Mar 13, 2012

whimboo commented Mar 13, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabolcs commented Mar 13, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Mar 14, 2012

xabolcs commented Mar 19, 2012

xabolcs commented Mar 19, 2012

xabolcs commented Mar 19, 2012

whimboo commented Mar 19, 2012

jwir3 commented Sep 27, 2012

xabolcs commented Sep 27, 2012

jwir3 commented Sep 27, 2012

xabolcs commented Sep 27, 2012

xabolcs commented Sep 28, 2012

tonymec commented Sep 29, 2012

whimboo commented Feb 23, 2013

xabolcs commented Feb 23, 2013

xabolcs commented Feb 23, 2013

whimboo commented Feb 25, 2013

xabolcs commented Feb 25, 2013

whimboo commented Mar 11, 2013

xabolcs commented Jun 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Jul 22, 2013

xabolcs commented Jul 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabolcs commented Aug 28, 2016

Choose a reason for hiding this comment

xabolcs commented Oct 11, 2017