Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions extension/chrome/content/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ get tabTitle() {
return tabbrowser.mCurrentBrowser.contentTitle;
},

get tabGroupTitle() {
try
{
return nightlyApp.getTabGroupTitle(window);
}
catch (e) { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},

init: function()
{
var brandbundle = document.getElementById("bundle_brand");
Expand All @@ -33,6 +41,11 @@ init: function()

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

try { // import tabGroupTitle functionality for titlebar customization
Components.utils.import("resource://nightly/tabGroupTitle.jsm", nightlyApp);
}
catch(e) { Components.utils.reportError(e); }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, that error report is needed. IMHO it's enough to fail silently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to put this into try/catch? The file will always be available and this import should never fail. If it does it would be fatal, and the failure should really bubble up.

},

openURL: function(url)
Expand Down
1 change: 1 addition & 0 deletions extension/chrome/content/nightly.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ variables: {
get compiler() this.appInfo.XPCOMABI.split("-")[1],
get defaulttitle() { return nightlyApp.defaultTitle; },
get tabtitle() { return nightlyApp.tabTitle; },
get tabgroup() { return nightlyApp.tabGroupTitle; },
profile: null,
toolkit: "cairo",
flags: ""
Expand Down
1 change: 1 addition & 0 deletions extension/chrome/content/titlebar/customize.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ init: function(aEvent)

paneTitle.addVariable("DefaultTitle");
paneTitle.addVariable("TabTitle");
paneTitle.addVariable("TabGroup");
paneTitle.addVariable("AppID");
paneTitle.addVariable("Vendor");
paneTitle.addVariable("Name");
Expand Down
1 change: 1 addition & 0 deletions extension/chrome/locale/en-US/variables.properties
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

variable.Profile.description=Current Profile
variable.Toolkit.description=Graphics Toolkit
75 changes: 75 additions & 0 deletions extension/modules/tabGroupTitle.jsm
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
var EXPORTED_SYMBOLS = ["getTabGroupTitle"];

const Cc = Components.classes;
const Ci = Components.interfaces;

const GROUP_DATA_IDENTIFIER = "tabview-group";

let sstore = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);


/**
* Simply retrieves the active tabgroup's title from SessionStore
*/
function getActiveGroupName(win) {
let data = "", groupTitle = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please put those on separate lines.

try {
data = sstore.getWindowValue(win, win.TabView.GROUPS_IDENTIFIER);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you make use of ´GROUP_DATA_IDENTIFIER´ here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know! :)

But it looks like this ("tabview-group") will work for Tab Groups and Simple Tab Groups too! :)

if (data) {
let parsedData = {};
parsedData = JSON.parse(data);
let activeGroupId = parsedData.activeGroupId;
data = sstore.getWindowValue(win, GROUP_DATA_IDENTIFIER);
parsedData = JSON.parse(data);
groupTitle = parsedData[activeGroupId].title;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to add a comment what this code is doing, and why you have to parse twice? That would help a lot. Thanks.

}
} catch (e) { }

return groupTitle;
}

/**
* Calculates Tab Group's title for nightlyApp
* @param {nsIDOMWindow} win A window which contains nightly.
*
* In Gecko 1.x title is set to "Undefined" as in other Apps
* Before FF10 title is managed by TabView
* After that we manages the title: using SessionStore and other borrowed code to generate
*/
function getTabGroupTitle(win) {
let nightlyApp = win.nightlyApp;
let TabView = win.TabView;

// TabView isn't implemented
if (!("TabView" in win))
return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// If we are before Bug 682996,
// use TabView's own implementation except it is null
if (typeof(TabView.getActiveGroupName) === "function")
return TabView.getActiveGroupName() || "";

// TabView isn't initialized
if (!TabView._window) {
return getActiveGroupName(win);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no legal way to modify active group and / or active group's title then caching the result of getActiveGroupName() would be nice.
In other words: if the (TabView._window == null) implies that group items are immutable then caching the result of getActiveGroupName() is allowed.

}

// We get the active group this way, instead of querying
// GroupItems.getActiveGroupItem() because the tabSelect event
// will not have happened by the time the browser tries to
// update the title.
let groupItem = null;
let activeTab = win.gBrowser.selectedTab;
let activeTabItem = activeTab._tabViewTabItem;

if (activeTab.pinned) {
// It's an app tab, so it won't have a .tabItem. However, its .parent
// will already be set as the active group.
groupItem = TabView._window.GroupItems.getActiveGroupItem();
} else if (activeTabItem) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would not recommend to use the hanging style. Lets move the ´else´ and ´catch´ statements into their own line.

groupItem = activeTabItem.parent;
}

// groupItem may still be null, if the active tab is an orphan.
return groupItem ? groupItem.getTitle() : "";
}