Closed Bug 578512 Opened 14 years ago Closed 9 years ago

Each TabCandy instance to show tabs from all windows

Categories

(Firefox Graveyard :: Panorama, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: aza, Unassigned)

References

Details

(Whiteboard: [aza])

Attachments

(1 file)

Tabcandy continues to be accessible via each window, however Tabcandy should show all tabs. This allows a number of beneficial features:

* Never lose a tab (search works through all open tabs and groups)
* Similarly, without this tabs become very hard-to-find when multiple windows are used (window --> group --> tab) is a lot to keep in your head.
* Windows become a view-port into your browsing session. Switching entire tab groups becomes trivial.

There are a number of issues that arise that we need to spec:

* Are two different Tabcandy instances kept in sync. When you drag in one, does the group move in the other?
* What happens if you open the same group in two windows at the same time?
* What happens if you try to open the same tab in two windows at the same time. A lot of this will be potentially solved by app tabs.

Full spec to come.
Blocks: 577322
Assignee: nobody → aza
Depends on: 580952
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Version: unspecified → Trunk
I dunno where the full spec ended up, but I just proposed something very similar in d-a-f: http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/00c8b6d46336c279#

It may nonetheless be a good idea.
Depends on: 583414
Target Milestone: --- → Firefox 4.0b5
I haven't uploaded the full specification yet, unfortunately. It is mostly finished in my local version, though. I will put it up shortly.
Requesting blocking, for the benefits enumerated in comment #0. Without this, the utility of the feature is significantly hampered.
blocking2.0: --- → ?
QA Contact: tabcandy → tabcandy
Blocks: 587874
Blocks: 587676
Blocks: 588526
Blocks: 588899
blocking2.0: ? → beta5+
I'm working on the (rather major) code refactor that makes this possible. Creating new tab and group JavaScript modules that communicate with the various Tab Candy frames (one frame for each window).
Blocks: 588217
Aza. 
I have an idea, which actually come from your introduction video of Tab Candy. 

Idea: 
If user has 1 window. Tab Groups appear like the current behavior. 
If user has X windows. Tab Groups will be grouped according to their Window. 

That means that Tab groups in Window 1 will be inside a Grouping labeled as Window 1. Tab Groups in window 2 will be inside a Grouping labeled as window 2.
Some thoughts from Beltzner on IRC last night:

[02:32]	<beltzner> in a nutshell: overview should show all tabs open in firefox, and if a group is being viewed by a window, that should be signalled somehow. selecting a tab in a group that's being viewed by a window switches to that window. selecting a tab that's in a group not viewed by any window replaces the current window's group with that group. ta-dah
[02:33]	<beltzner> (obv. a window can only display one group at a time)
I've checked the first steps in this direction into the tabcandy-central branch: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/7d2abfdbc033

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/e5582fa166d1

With the old architecture, each Tab Candy frame (one per window) kept track of just the tabs and groups for that window, tracking updates, saving to sessionstore, etc. 

With the new architecture, the tabview/groups.jsm and tabview/tabs.jsm keep track of all tabs and groups for the whole application, tracking updates, saving to sessionstore, and communicating with the various Tab Candy frames (as well as others in the system as needed) via a subscription interface. 

Still to be done: 

* Lots of little things broken (such as synchronized title updates, thumbnail updates, etc)
* Need to swap tabs from window to window as needed
* Unit tests

It's currently unusable, as you can't load a tab into any other window than the one it started in, but once that's fixed, it'll be usable but with bugs. 

I expect we'll get tab swapping in this week, plus a number of bug fixes, though the remaining fixes will continue into the following week.
Depends on: 590488
Hardware: x86 → All
More work has been done on the tabcandy-central branch. Here's the current TODO list, from http://etherpad.mozilla.com:9000/tabcandy (in priority order, with possible assignments):

* When you go into a tab from a group that's not attached to the current window, bring all the tabs over to the current window [RAYMOND]
*  When closing a window, the currently open group should be closed, but  all other groups and tabs that are attached to the window but hidden  should be pushed over to another window so they don't get closed  [RAYMOND?]
* Make sure it doesn't break any of our unit tests (or those of other folks) [RAYMOND?]
* Closing tabs from the wrong window sometimes causes the whole window to go away when it shouldn't, or causes it to not go away when it should [RAYMOND?]
* You don't get all the tabs and groups in your Tab Candy if the first window to open is not in Tab Candy UI [IAN?]
* When a new window opens up, its tabs should automatically be put into a fresh group [MITCHO?]
* When a new tab opens in one window, its thumbnail isn't getting updated in other windows [RAYMOND?]
*  For "new tab", need to put in selected group in the window the user is  acting in (right now it's going in the selected group for the first  window in the window list) [MITCHO?]
* Get it reviewed [DIETRICH]
* Unit tests for the new code [RAYMOND?]
* Maintain child order within groups. [MITCHO?]
* Right now I have "first run" set to go off every time you open a new window and you don't have any groups. We need a better rule
* If different Tab Candy windows are different sizes, scale their contents appropriately [MITCHO]
* userSize isn't connected between *Item and TabView* [IAN?]
*  Should each tab candy frame have its own set of thumbnails, or should  they all subscribe to one thumbnail per tab (persumably rendered at the  highest res currently on screen) [MITCHO?]
* What to do about saving cached thumbnails? Save the highest res version? Save one for each window? Right now we pretty much have it disabled [RAYMOND?]

I'll be out of town Thursday through Monday (family camping trip), but Raymond is hard at work on the tab swapping, with zpao's help. 

Also, for your reference, some architectural notes: 

https://wiki.mozilla.org/Firefox/Projects/TabCandy/TabsAndGroups

At any rate, I think the transition is going well, but there are lots of details to attend to, the biggest issue being tab swapping.
We have tab swapping working! Raymond has made it possible to switch into a tab from any window. Grab the latest tabcandy-central to try it out. 

With this change, the "all tabs/all windows" work is finally testable. There are plenty of issues (see the EtherPad list), but it's time to start documenting them.
(In reply to comment #11)
> We have tab swapping working! Raymond has made it possible to switch into a tab
> from any window. Grab the latest tabcandy-central to try it out. 
> 
> With this change, the "all tabs/all windows" work is finally testable. There
> are plenty of issues (see the EtherPad list), but it's time to start
> documenting them.

Could you please tell me where can I get the latest tabcandy-central?
I would really appreciate it!
(In reply to comment #13)
> https://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/

Many thanks!
Alas, I'd thought this was some kind of new build I could install to try the "all tabs/all windows" work. Am I right? If so, please tell me where I can find it! If not, I'm sorry for my ignorance!
This still doesn't fix the fundamental issue of groups being hidden from the OS. Solution: Just make all tab groups normal OS windows and use the OS to do what is
suggested here: https://wiki.mozilla.org/User:Broccauley/Fixing_TabCandy
Per Ian's message to DAF, not going to make b5. Aiming for early b6 cycle.
blocking2.0: beta5+ → beta6+
(In reply to comment #15)
> This still doesn't fix the fundamental issue of groups being hidden from the
> OS. Solution: Just make all tab groups normal OS windows and use the OS to do
> what is
> suggested here: https://wiki.mozilla.org/User:Broccauley/Fixing_TabCandy

Thanks for the input. This change isn't actually attempting to solve that particular problem. We're not going to get tight OS integration with Panorama in this release, perhaps we can explore these ideas in a future release though.
In terms of timing, this will not be able to land in beta 5. Instead we are shooting to be all reviewed (or at least be ready for review) when the beta 6 tree opens up. We have a number of other features that we know we need to work on (e.g., undo closing a group) that we will also need to finished in the beta 6 cycle so we want to minimize the amount of actual coding on this bug during beta 6.
Blocks: 591911
(In reply to comment #12)
> (In reply to comment #11)
> > We have tab swapping working! Raymond has made it possible to switch into a tab
> > from any window. Grab the latest tabcandy-central to try it out. 
> > 
> > With this change, the "all tabs/all windows" work is finally testable. There
> > are plenty of issues (see the EtherPad list), but it's time to start
> > documenting them.
> 
> Could you please tell me where can I get the latest tabcandy-central?
> I would really appreciate it!

Sorry, but I'd really like to try this! If the last tabcandy-central is a new build I can try tab swapping, please tell me how/where do I get it.
I've already received the following link from Aza:

https://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/

but I didn't find any build there.
Sorry for my ignorance but I love Tab Candy and I'd really like to try this!
Hi Gabriela! The tab-swapping changes are mostly internal at this point, not something that shows up in the front-end.. but it will when it's ready, and at that point will land on mozilla-central. So the regular Firefox nightly builds are the right place to be if you want these changes soonest.
(In reply to Comment 21)

Hi Dietrich! Many thanks for your explanation!
I'm afraid it was a misunderstanding on my part then! I thought "Grab the latest tabcandy-central to try it out" meant I could install a build with this fix to try, as I do sometimes with try server builds.
I actually use Firefox nightly builds so I suppose I'll see the changes as soon as they land.
Attached patch WIP 1Splinter Review
Attaching our current Work In Progress, for Dietrich's reviewing pleasure.  Since we're not ready to land the patch, I figure the "feedback" flag is more appropriate than the "review" flag?
Attachment #470901 - Flags: feedback?(dietrich)
Comment on attachment 470901 [details] [diff] [review]
WIP 1

general comment: please give 8 lines of context in patches, it really helps in reviewing.

the related lines in my .hgrc are:

[diff]
git = 1
showfunc = 1
unified = 8

>+pref("browser.gesture.swipe.up", "Browser:ToggleTabView");
>+pref("browser.gesture.swipe.down", "Browser:ToggleTabView");

we need to pull the gesture changes out and have those reviewed in that bug. but i'm not sure that's reasonably possible given the tabcandy-central model at the moment...

i'm ignoring those bits for now, can figure it out later.

>+// TODO: should we scope this file so these imports don't bleed out to the rest of browser.js?
>+Cu.import("resource://gre/modules/tabview/groups.jsm");
>+Cu.import("resource://gre/modules/tabview/tabs.jsm");
>+Cu.import("resource://gre/modules/tabview/utils.jsm");
>+
> let TabView = {

instead, you could make each exported object a memoized lazy getter on TabView object, eg:

let TabView = {
  get TabViewGroups() {
    let tmp = {};
    Cu.import("resource://gre/modules/tabview/groups.jsm", tmp);
    this.__defineGetter__("TabViewGroups", function() tmp.TabViewGroups);
    return this.TabViewGroups;
  },
  ...
};

not only does this avoid global scope pollution, but also ensures that the module isn't actually imported until first access. in the current code, you import the module at browser window-load time, which is not necessary for users who don't use tabview.

more on js memoization and avoiding shadow vars: http://osteele.com/archives/2006/04/javascript-memoization

a moz util for easing this: https://wiki.mozilla.org/Performance/Addons/BestPractices#Lazily_Load_Services

>+  // ----------
>+  uninit: function TabView_uninit() {    
>+    if (this._window) {
>+      this._window.UI.uninit();
>+    }

is there ever really a valid scenario in which _window will not be defined? if that's the case, then you should either not do the check, so that the resulting exception bubbles up, or you should check and report the error. otherwise you're silently swallowing the evidence in an error state.

>-let GroupItem = function GroupItem(listOfEls, options) {
>+let GroupItem = function GroupItem(group, options) {
>   try {

i can't see the other end of this in the patch, but i recommend only wrapping things in try/catch that you expect to throw something specific, and that you specifically handle. wrapping the entirety of function bodies often leads to silent failures, which are extremely hard to diagnose and debug.

>@@ -164,22 +145,30 @@
>     .appendTo($container);
> 
>   // ___ Title

is this a tabcandy-specific documentation format, that's parsed and used somehow?

>+  var reinforceTitle = function() {
>+    var focused = document.activeElement == self.$title[0];
>+    if (!focused)
>+      self.$titleShield.show();
>+    if (!self.group.title() && !focused) {

generally more inline documentation would be great. for example, just a quick line above inline functions like this, describe what it does and why.

also, maybe a separate method for this inline func instead of making this method so chubby? the tabview code tends towards a big method-ology (teehee!), which makes me a little nervous, as it makes debugging and unit testing more difficult, IMHO.

>+  this.group.addSubscriber(this, "childAdded", function(sender, tab) {

>+  this.group.addSubscriber(this, "childRemoved", function(sender, tab) {

if you made the subscriber callbacks methods on the main object, and they'd be easier to find when debugging, and they'd not make this such a megafunction. eg:

this.group.addSubscriber(this, "childRemoved", function(sender, tab) self.onChildRemoved(sender, tab));

at the very least, the anonymous functions should be named, so that they're identifiable in debuggers and stack dumps.

this goes for all the group property subscriber stuff below this as well.

>+    let item = TabItems.tabItem(tab);
>+    if (item) {

is there any reason this is expected to validly fail here? looks like silently swallowing errors again (referred to as "SSE" from here on out).

>+  this.group.addSubscriber(this, "newBounds", function(sender, bounds) {

>+  this.group.addSubscriber(this, "newTitle", function(sender, title) {

>+  this.group.addSubscriber(this, "close", function(sender) {

have you thought about a more general addSubscriber(subscriber) which gets send all events from a given subscribable thing, and calls subscriber.handleSubscriptionEvent(eventName, eventProperties). or something like that. doesn't have to happen in this bug, just floating the idea.

>diff -r 081a707a76b8 browser/base/content/tabview/modules/groups.jsm
>--- a/browser/base/content/tabview/modules/groups.jsm  Wed Aug 25 09:10:00 2010 -0400
>+++ b/browser/base/content/tabview/modules/groups.jsm  Tue Aug 31 11:11:57 2010 -0700
>@@ -19,7 +19,6 @@
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>- * Edward Lee <edilee@mozilla.com>
>  * Ian Gilman <ian@iangilman.com>

intentional?

>+// ----------
>+function sessionstore() {
>+  if (!_sessionstore) {
>+    _sessionstore =
>+      Cc["@mozilla.org/browser/sessionstore;1"].
>+        getService(Ci.nsISessionStore);
>+  }
>+
>+  return _sessionstore;
>+}
>+
>+// ----------
>+function newID() {
>+  if (!uuidGenerator) {
>+    uuidGenerator = 
>+      Cc["@mozilla.org/uuid-generator;1"].
>+        getService(Ci.nsIUUIDGenerator);
>+  }
>+
>+  let uuid = uuidGenerator.generateUUID();
>+  return uuid.toString().replace(/\{|\}/g, ""); // strip out the curly brackets that toString adds
>+}

for sessionstore and uuidGenerator, should move to the memoized lazy getter approach i showed previously.

>+// ----------
>+// Saves the data for a single group, associated with a specific window.
>+function saveGroup(win, data) {
>+  var id = data.id;
>+  var existingData = readGroups(win);
>+  existingData[id] = data;
>+
>+  sessionstore().setWindowValue(win, GROUP_DATA_IDENTIFIER,
>+    JSON.stringify(existingData));
>+}

any reason all these data storage functions aren't methods off of GroupItems?

>+// Deletes the data for a single group from the given window.
>+function deleteGroupStorage(win, id) {
>+  let existingData = readGroups(win);
>+  delete existingData[id];
>+
>+  let jsonString = JSON.stringify(existingData);
>+  if (jsonString == "{}")
>+    sessionstore().deleteWindowValue(win, GROUP_DATA_IDENTIFIER);
>+  else
>+    sessionstore().setWindowValue(win, GROUP_DATA_IDENTIFIER, jsonString);
>+}

called deleteGroupStorage, but can also save data?

also, could just check the data before stringification to save some cycles.

>+TabViewGroup.prototype = Utils.extend(new Subscribable(), {
>+  // ----------
>+  window: function() {
>+    return this._window;
>+  },

stylistic question: why not use getters? eg:

let object = {
  get window() this._window,
  ...
}

the code would generally look less brackety, and property-like things would be able to be treated as such.

>+
>+  // ----------
>+  setWindow: function(win) {

and here you could implement a setter:

let object = {
  set window(win) this._window = win,
  ...
}

if you don't switch to getters, then you should make the api semantically consistent:

getWindow()
setWindow()

update: now that i'm farther down into this file, i see you have this as a convention:

thing()    // get thing
setThing() // set thing

it's up to you, but IMO that's a sub-optimal convention for the reasons above.

>+  // ----------
>+  // Takes in a TabViewTab. 
>+  removeChild: function(tab) {

removeTab?

>+  // ----------
>+  preload: function(win) {

>+  // ----------
>+  loadForWindow: function(win) {

some commentary for these, explaining who calls them, where and why, would be good.

>diff -r 081a707a76b8 browser/base/content/tabview/modules/tabs.jsm

>+// ----------
>+function sessionstore() {
>+  if (!_sessionstore) {
>+    _sessionstore =
>+      Cc["@mozilla.org/browser/sessionstore;1"].
>+        getService(Ci.nsISessionStore);
>+  }
>+
>+  return _sessionstore;
>+}

ditto about converting to memoized getter.

>+// ----------
>+function newTab(xulTab, data) {

why is this stuff not built into the TabViewTab ctor instead?


>+function throwIfBadTabData(data) {

>+function loadTabData(xulTab) {

>+function saveTab(xulTab, data) {

ditto, why are these functions instead of built into TabViewTab?

>+
>+// ###########
>+function TabViewTab(xulTab, data) {
>+  this.isTabViewTab = true;

is this ever not true?

>+  this._needsData = false;

should document this

>+  setXulTab: function(xulTab) {
>+    Utils.assertThrow(xulTab, "must have a xulTab");
>+    
>+    if (xulTab == this._xulTab)
>+      return;
>+
>+    saveTab(this._xulTab, null); // deletes the old tab information
>+    
>+    this._xulTab = xulTab; 
>+    
>+    this._sendToSubscribers("swapTabs");
>+    this.save();
>+  },

is there a redundant saving of data here?

>+  // ----------
>+  takeData: function(data) {

document why this is here?

>+  // ----------
>   // Function: update
>   // Takes in a xul:tab.
>   update: function(tab) {

this function could be named much more descriptively. updateXULTab maybe?

>+      let tabItem = TabItems.tabItem(tab);
>+      if (tabItem) {

SSE?

>+        // ___ icon
>+        let iconUrl = tab.image;
>+        if (iconUrl == null)
>+          iconUrl = "chrome://mozapps/skin/places/defaultFavicon.png";

instead of hard-coding this URL, add something like this to the file, or off Utils if it's used elsewhere:

const defaultFaviconURL = Cc["@mozilla.org/browser/favicon-service;1"].
                          getService(Ci.nsIFaviconService).defaultFavicon.spec;

>@@ -1044,6 +1017,12 @@
>     canvas.width = w;
>     canvas.height = h;
>   },
>+  
>+  // ----------
>+  // Function: setTab  
>+  setTab: function(tab) {
>+    this.tab = tab;
>+  },

better description plz.

>diff -r 081a707a76b8 browser/base/content/tabview/tabview.css

>\ No newline at end of file

fixy

>@@ -234,6 +217,72 @@
>   },
> 
>   uninit: function() {
>+    let otherWindow;
>+    let enumerator = Services.wm.getEnumerator("navigator:browser");
>+    while(enumerator.hasMoreElements()) {
>+      let win = enumerator.getNext();
>+      // ToDo: what do we do if no other window has the tabview frame loaded?
>+      if (win != gWindow && win.TabView._window) {
>+        otherWindow = win;
>+        break;
>+      }
>+    }

this should be broken out into a method

>+  
>+        // Zoom out!
>+        item.zoomOut(function() {
>+          item = TabItems.tabItem(currentTab); // in case it's been destroyed

yikes! how and why would that happen?

>+  
>+          self.setActiveTab(item);
>+  
>+          if (item && item.parent) {
>+            var activeGroupItem = GroupItems.getActiveGroupItem();
>+            if (activeGroupItem)
>+              activeGroupItem.setTopChild(item);
>+          }

doesn't seem like item should ever be null here, possible SSE?

>@@ -431,9 +498,14 @@
>               (groupItem == null && gBrowser.visibleTabs.length == 1)) {
>             // for the tab focus event to pick up.
>             self._closedLastVisibleTab = true;
>+
>             // remove the zoom prep.
>-            if (tab && tab.tabItem)
>-              tab.tabItem.setZoomPrep(false);
>+            if (tab) {
>+              let tabItem = TabItems.tabItem(tab);
>+              if (tabItem)
>+                tabItem.setZoomPrep(false);
>+            }

SSE in two places here?

>@@ -482,12 +554,14 @@
> 
>     let oldItem = null;
>     let newItem = null;

can you update this to oldTab/newTab? it matches with tab and currentTab then, and is less ambiguous.

a general comment: this code is really hard to follow because the naming conventions are not very descriptive.

for example, i'd change:

GroupItem
GroupItems
TabViewGroup
TabViewGroups

to something like:

GroupView
GroupViews
GroupModel
GroupModels

it doesn't have to be exactly this, but it'd be much easier to follow the code if the concept that one is a UI object and one is a data class was encoded into the naming scheme better. this comment also applies to the TabItems/TabViewTabs code. it'll avoid things like tab.tabViewTab that are far from self-documenting :)

there are also places where you use "item" and "items" where you mean "tab" and "tabs", which would be clearer.

wrt consistency, you have TabViewGroups.createGroup and TabItems.add (iirc). it'd be great to have these apis be consistent in the semantics for similar actions like "creating a new thing" or "adding thing to a group of things" (where things are tabs, groups, groups of groups, etc).

the naming changes are easy to make (global search and replace) and are very important to future maintainability, so must be fixed before final review IMO. while they might seem trivial, the braincycle cost of difficult-to-read code is very high.

another thought i had while reviewing this: is there a reason why the tabview page is not also mirrored across windows the way that tabs are? in the Panels API for Jetpack, Myk loads web content into the hidden window, and then when the panel that contains that content is loaded into a given visible window, he just swaps the already-loaded content document in, similarly to how you do that with tabs. this would be a huge improvement in resource consumption when the user has multiple windows open.

feedback+ on the approach of these changes. the biggest problem i had in reviewing this was readability. consistent and descriptive api semantics, along with more inline documentation, would improve this greatly.
Attachment #470901 - Flags: feedback?(dietrich) → feedback+
(In reply to comment #24)
> >+// TODO: should we scope this file so these imports don't bleed out to the rest of browser.js?
> >+Cu.import("resource://gre/modules/tabview/groups.jsm");
> >+Cu.import("resource://gre/modules/tabview/tabs.jsm");
> >+Cu.import("resource://gre/modules/tabview/utils.jsm");
> >+
> > let TabView = {
> 
> instead, you could make each exported object a memoized lazy getter on TabView
> object, eg:
> 
> let TabView = {
>   get TabViewGroups() {
>     let tmp = {};
>     Cu.import("resource://gre/modules/tabview/groups.jsm", tmp);
>     this.__defineGetter__("TabViewGroups", function() tmp.TabViewGroups);
>     return this.TabViewGroups;
>   },
>   ...
> };

XPCOMUtils.defineLazyGetter!

> >+// ----------
> >+function sessionstore() {
> >+  if (!_sessionstore) {
> >+    _sessionstore =
> >+      Cc["@mozilla.org/browser/sessionstore;1"].
> >+        getService(Ci.nsISessionStore);
> >+  }
> >+
> >+  return _sessionstore;
> >+}
> >+
> >+// ----------
> >+function newID() {
> >+  if (!uuidGenerator) {
> >+    uuidGenerator = 
> >+      Cc["@mozilla.org/uuid-generator;1"].
> >+        getService(Ci.nsIUUIDGenerator);
> >+  }
> >+
> >+  let uuid = uuidGenerator.generateUUID();
> >+  return uuid.toString().replace(/\{|\}/g, ""); // strip out the curly brackets that toString adds
> >+}
> 
> for sessionstore and uuidGenerator, should move to the memoized lazy getter
> approach i showed previously.

XPCOMUtils.defineLazyServiceGetter!
Depends on: 592874
We've been working diligently on this features, and we've made incredible progress for such a fundamental architectural change, but the fact is, it's just too late in the game for a change of this magnitude; we're coming up on feature freeze, and there are still major architectural and UX questions unresolved. Therefore we've decided to put this feature on hold until after Firefox 4, instead spending the remaining time bug fixes and UX polish. Fortunately, all of the "all windows" work was done on the tabcandy-central branch, so mozilla-central is still clean, and we can just hold the tabcandy-central work in storage until we're ready to reengage with the feature. 

I've given this bug a target milestone of "future" and removed the "blocking 2.0"... I hope this is the correct procedure. 

Thank you to everyone who has contributed to this feature... we'll see you again in the future!
blocking2.0: beta6+ → ---
Target Milestone: Firefox 4.0b5 → Future
No longer blocks: 588526
No longer blocks: 587874
Depends on: 594009
No longer blocks: 593157
(In reply to comment #24)
> Comment on attachment 470901 [details] [diff] [review]

Thanks for all the great feedback! I started making some of these changes before we put this bug on hold, so here are my comments in response, for when we get back to it.  

> general comment: please give 8 lines of context in patches, it really helps in
> reviewing.

Cool, I've updated my settings.  

> instead, you could make each exported object a memoized lazy getter on TabView
> object, eg:

Thanks!  I'm now using a hybrid of your code and Dolske's:

  get TabViewGroups() {
    let tmp = {};
    Cu.import("resource://gre/modules/tabview/groups.jsm", tmp);
    return this.TabViewGroups = tmp.TabViewGroups;
  },

> >+  // ----------
> >+  uninit: function TabView_uninit() {    
> >+    if (this._window) {
> >+      this._window.UI.uninit();
> >+    }
> 
> is there ever really a valid scenario in which _window will not be defined? if
> that's the case, then you should either not do the check, so that the resulting
> exception bubbles up, or you should check and report the error. otherwise
> you're silently swallowing the evidence in an error state.

Yes, this._window being null is a valid state; it means we've never actually launched the frame in this window.  I've now added a comment to clarify that.  

> i can't see the other end of this in the patch, but i recommend only wrapping
> things in try/catch that you expect to throw something specific, and that you
> specifically handle. wrapping the entirety of function bodies often leads to
> silent failures, which are extremely hard to diagnose and debug.

Wherever we do this the catch has a log statement for the error, so it's never silent.  At any rate, we're pulling these out as we go.  

> >   // ___ Title
> 
> is this a tabcandy-specific documentation format, that's parsed and used
> somehow?

No, just a stylistic convention of mine for documenting the sections of a routine. 

> >+  var reinforceTitle = function() {
> >+    var focused = document.activeElement == self.$title[0];
> >+    if (!focused)
> >+      self.$titleShield.show();
> >+    if (!self.group.title() && !focused) {
> 
> generally more inline documentation would be great. for example, just a quick
> line above inline functions like this, describe what it does and why.

Thanks; at this point I don't know what's obvious and what's not.  I've now documented this function.

> also, maybe a separate method for this inline func instead of making this
> method so chubby? the tabview code tends towards a big method-ology (teehee!),
> which makes me a little nervous, as it makes debugging and unit testing more
> difficult, IMHO.

Done.  

> >+  this.group.addSubscriber(this, "childAdded", function(sender, tab) {
> 
> >+  this.group.addSubscriber(this, "childRemoved", function(sender, tab) {
> 
> if you made the subscriber callbacks methods on the main object, and they'd be
> easier to find when debugging, and they'd not make this such a megafunction.

Done.
No longer blocks: 588630
No longer blocks: 587140
Blocks: 592545
Priority: -- → P2
I'm adding our notes from EtherPad for the project here, and have removed them from the EtherPad; we can grab them again from here when we're ready.


== All tabs from all windows == 

NEEDED TO LAND:

* Make sure we pass all the leak tests. [Ehsan?]
* Get it reviewed [DIETRICH]
*  Open some windows, click on the panaroma icon in one of the windows,  everything is fine.  Do the same in another window.  It keeps loading  (spinning mouse pointer) and then I got an unresponsive dialog with  message Script: resource://gre/modules/tabview/utils.jsm:623. [MITCHO?]
*  Open two windows, take them both into the panorama UI, make a group,  drop the tabs into the group, quit Minefield, restart; the tabs are now  bunched up in the lower right. [RAYMOND]
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/bac8e1a8a3a0
COMMENT: [IAN] could you have a look this bug please?  I still couldn't find a way to locate the root cause of it.
* Create new tab with + doesn't always open in the correct group. [IAN]
* Open two windows, go to Panorama in one and click on a tab from the other window. That group gets a new tab (wrong) and all tabs are unclickable. http://people.mozilla.org/~araskin/panbugs/CloseAndClick.mov [RAYMOND] http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/b1806f623ffa
* Tabs are not moved to the other window properly when their browser window is closing [RAYMOND]

POSSIBLY NEEDED TO LAND?:

* Open two windows, show the  panorama UI in one of the windows.  Go to the window without panorama  iframe loaded and close the window.  The tabItem doesn't get removed in  the window with the panorama UI.  We need a way for window without panorama iframe loaded to remove groupItems and tabItems.
COMMENT: Ian: we need to figure how we should implement that. Case: for example,
1. Open two windows
2. Go to panaroma view in window A and leave window B
3. Create some tabs in window B
4. Close window B but the tabitems and group presenting tabs in window B would stll be in the panaroma view in window A
Therefore, we need a way to remove them even window B hasn't loaded the panaroma iframe.   Furthermore, when moving the last tab in a window to another window, we need to show the panaroma UI first before moving the tab, otherwise, the window would get closed as it's not in the panaroma UI view and the last time is removed. We need to fix that.
* The user starts with a fresh profile and makes three windows, populating each with 5 tabs.  Then they hit tabcandy for the first time.  At that moment, TC should create three groups, each with 5 tabs,  and the InfoItem (soon to become just a video tab).


AFTER LANDING, BUT BEFORE B6:

* New tabs are added to the first group in the window, not the currently selected group.  See newTab() in tabs.jsm.  
*  Open two window, take them both into the panorama UI, quite Fx and  restart.  An empty group would be created. It doesn't always happen.
*   It's possible for going into a group in one window to cause another  window to close (if that other window only had the tabs  for the group   in question); this needs to be fixed.   COMMENT: If the last tab is moved to other window, the browser would be  closed. May be we can move tabs from other group or orphan tab to that  window to keep it open but it's very hacky.  Thoughts? [ANSWER] I believe we have a bug out on allowing a Firefox window to exist in Tab Candy mode sans any tabs (yes, bug 576427). Thus, in this case, we'd simply have the window zoom out to Tab Candy land instead of closing.  http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/9c0b258f3e12
* https://bugzilla.mozilla.org/show_bug.cgi?id=592897 - perplexing experience when closing windows
* Maintain child order within groups.
*  Bug 593441: Right now placement of the InfoItem on firstTime is poor. Fix. [MITCHO]
* SCALING: If different Tab Candy windows are different sizes, scale their contents appropriately [MITCHO] - making good progress and functional-ish
*   Scaling bug: First-time dragging causes group to stay behind. New profile, single window. http://people.mozilla.org/~araskin/panbugs/firstrun_edge.mov
*   Scaling bug: we can currently push edges of the window and make groups smaller.   E.g., have two windows with different sizes and aspect ratios (like one  small square, and one larger wider rect), drag a group from left to  right in the larger one, note that the whole window scales down once the  group gets within 100 pixels of the right edge of the window.  
*  On Mac, after closing all browser windows and the Firefox icon is  still  on the dock, open a browser window, go into the tabview UI and  you  would end up seeing a blank thumb which isn't clickable.
* https://bugzilla.mozilla.org/show_bug.cgi?id=592885 - unable to get out of panorama mode on all windows at the same time  [perhaps not an issue at all?]

AFTER B6:

* Unit tests for the new code 
* userSize isn't connected between *Item and TabView* [IAN?]
*   Should each tab candy frame have its own set of thumbnails, or should they all subscribe to one thumbnail per tab (persumably rendered at the   highest res currently on screen) 
*  What to do about saving cached thumbnails? Save the highest res  version? Save one for each window? Right now we pretty much have it  disabled
* Rewrite bits external to the Tab Candy frame (such as tab triage, or the "next group" key combo) so they use groups.jsm as appropriate
Assignee: aza → nobody
Whiteboard: [aza]
I'm archiving here some email discussion on the implications of window closing in the "all windows" world, for when we get back to it. Note that this is related to bug 576427, but it goes beyond, so I'm posting it here. The emails below are in chronological order.


>> On Sun, Sep 5, 2010 at 3:00 AM, Ian Gilman <ian@iangilman.com> wrote:

I've been thinking about it, and I'm actually coming around to Dao's point of view in terms of windows without tabs. I think sometimes windows should close without your explicitly asking them to.

For instance, you should never have more windows on the screen than you have Panorama groups. If you have three windows and three groups, and you close one of the groups, one of the windows should close.

I'm not sure, but I think orphaned tabs should be considered groups in this context; if you have no groups, but three orphaned tabs, you should still be able to have three windows. Close one of those tabs, and one of the windows closes.

Now imagine you have three orphans tabs and three windows, one for each tab. If you then grouped two of those tabs, one of your three windows should close, so you have one window for the remaining orphan, and the other window for the group of two.

Furthermore, I think empty groups shouldn't count for windows. Let's say you have two groups, each with two tabs, and two windows open, one for each group. If you close all of the tabs in one of the groups (or move all of the tabs to the other group), its window should close. I believe this is true even if the group is named, and therefore won't automatically be destroyed; the group will still exist, but its window should not remain open.

I believe these are logical, understandable rules that the user can comprehend; we're not talking about windows closing for seemingly arbitrary reasons. In a way, we're just extending the existing browser behavior into the Panorama world.

If we don't do this, we'll be breaking the notional bond we are trying to create between windows and groups. Also, it would be too easy for the user to amass countless empty windows, and then rather than Panorama easing the user's window management responsibilities, we are making them worse.

In terms of implementation, since we never have fewer windows than we have groups, we just need to make sure the groups are spread out among the windows. If the user does a swap that would cause a window to end up empty, we shove a group into that window. If there aren't enough groups to go around, then we let that window close.

In terms of your group swapping scenario, Aza, we'd be fine: start with group A. in window A. and group B. in window B.; take window A. to Panorama and open group B. from there; window B. reverts to Panorama, and behind the scenes group A. has been swapped over to window B. while group B. was being swapped to window A., therefore no windows close; you can then open group A. in window B. and you've made a full swap.


>> On Sep 5, 2010, at 10:12 AM, Dietrich Ayala wrote:

What bug is Dao saying this in? I might have missed some of the
conversation, but from reading Ian's email I'm a little worried that
there will be no end of edge-cases and ill-fitting scenarios while
trying to get groups and windows to play nice.

Maybe we should go all the way and make it a 1:1 mapping of groups to
windows. New group opens a window, close group closes a window. New
empty group is whatever the user has as a default for a new window
(home page, about:blank, whatevs).

Or maybe go all the way the other way, and make Panorama completely
modal: enter Panorama and all windows collapse into one. Exit Panorama
and the session explodes back into multiple windows. (Would need to
figure out what to do with orphan tabs...)

Orphan tabs are awkward in both scenarios above, so maybe not the
right answer either :(
Blocks: 606257
Blocks: 603789
Note: in order to port some all-windows back to the current m-c for bug 601534, I recently made two commits to tabcandy-central which are not reflected in the WIP above.

A quick rundown of the changes:
- Making the canonicalGeometry a more "modular" component, in preparation for porting code over to m-c for bug 601534:
- changed gUniversalGeometry to instead be UI.canonicalGeometry
- marking private methods in TabViewUniversalGeometry as such
- changing most external references to "universal" and "universalized" to be "canonical" and "canonicalized"
- s/_universalRect/_canonicalRect/ for consistency
- fixed a bug wherein an Item which was forcing us to scale down, when moved, does not trigger a reevaluation of the scaling factor.
I haven't see the latest bleeding edge changes for Panorama, but what I see in the beta7 is a Panorama implementation that is nowhere near prime time. I would call the current state "early alpha" and I totally fail to see how you can seriously consider releasing that in a final Fx 4.0. Once you push this beast to the public, it will be very hard to make fundamental changes to it.

As of today, you have two options: 

1) Use tabs and windows for organization, move tabs between windows or make them windows of their own if needed, and totally ignore the presence of Panorama. 

2) Use Firefox as a single window application (like a MDI app or as if it was in fullscreen mode), never have more than one window open, use tabs and group them into tab groups using Panorama.

Either way works nice, either way has benefits. I personally prefer way one, since I'm a Mac user and I like using OS features like Exposé (and tab groups don't show up in Exposé), but I can understand people that prefer the second way, because they prefer MDI over SDI. However, in its current released state (beta7), don't dare to mix those two paradigms, the result is unusable for your daily work.

A lot of ideas to address this issue have been posted here and elsewhere, but to be honest, none of them seems to be the holy grail. The problem is that tab groups add an abstraction layer, that fits nowhere into the world of windows and tabs as we know it. The most reasonable suggestion is still:

https://wiki.mozilla.org/User:Broccauley/Fixing_TabCandy

It has its weaknesses and some "not so nice" border cases, but it combines the two worlds of above at least in a rather logical way. Even if it has not the answer to all questions, at least the problems mentioned on this page are fundamental problems that still exist in the beta7.

The only other solution I can think of is to simply NOT mix the two paradigms, but make them mutual exclusive. You can have multiple windows with tabs or a single window with tabs and tabs groups, not both at the same time. When you enable Panorama, each currently open window becomes a tab group and all tabs of that window are added to that group. All windows, but the currently focused one, will close and you are now in single window mode; trying to open a new window will in fact open a new tab group with a new tab inside (blank or with the requested page). There is a way out of single user mode, causing a new window to be opened for each tab group and all tabs of this group being added to the window and you are back to multi window mode. Fx opens in the same mode it was closed, users can choose either mode, e.g. single window mode is great for netbooks, but I'd prefer multi window mode on Mac OS X.
(In reply to comment #32)
Indeed, the fix to this multiple window problem can be described much more concisely than that old verbose essay that I wrote by the following statement:

"Replace the extraneous concept of a 'tab group' with a standard OS window."

I have created a new Bug 611194 to summarizes this.


TGOS, I wouldn't agree with you that you are "mixing two paradigms" if you merely look at Panorama as now being just another *view* of your open windows and tabs (compare to Opera's "Windows" panel). Your alternative proposal of having two different modes though just seems excessively complicated.

I also agree that Panorama really shouldn't be shipped at all in its current state in FF4. Contrast this to the excellent new Ctrl-Tab feature which is only available if you enable browser.ctrlTab.previews in about:config! Surely the new Ctrl-Tab feature is entirely consistent with OS paradigms and should be enabled by default, and the complicated and non-consistent Panorama feature should be the one which you can enable only in about:config until its usability issues have been ironed out!
Blocks: 620624
Blocks: 624149
No longer depends on: 590488
Has the work on this stalled? The last change to tabcandy-central was 4 months ago. Or has it moved somewhere else?

This would have been such a fantastic feature; without it Tab Panorama is kind of disappointing. Really hoping to see this finished...
This was put on hold until after Fx4. We can now start figuring out how we want to accomplish it.
Time to resurect this bug! Let's do it right this time, though. Part of that means really building the infrastructure into the browser to support it (see bug 645073). 

I suspect the original work on tabcandy-central will only be useful for reference. Also, this write-up: 

https://wiki.mozilla.org/Firefox/Projects/TabCandy/TabsAndGroups

... may provide some inspiration, but it's probably mostly out of date (though maybe we can get around to updating that page itself with a new architecture). 

One of the things that hampered us last time was trying to wedge our new functionality into the browser without changing too much. This time let's think it through and make the real changes in the browser necessary to make it all work.

The big thing is to make the Panorama state data centralized, rather than tied to a specific window. Each window's Panorama should be a view into a common set of Panorama data, and changes that happen in one should be reflected in others. The actual spatial layout is important (to utilize the user's spatial memory), so even if windows are of different sizes, the same layout should be displayed, just scaled to fit.

Another area to deal with is the fact that it's currently not possible for a browser window to exist without at least one tab (see discussion in bug 576427, as well as comment 30 here). We need to decide if it's okay for there to be a tab-less window (just showing Panorama). 

For that matter, I believe it's currently not possible for a tab to exist without being attached to a window, but maybe we should allow this? Already the tabs in Panorama groups other than the one you're currently looking at seem to be detached from the window (though they're just hidden). Once those tabs can easily move from window to window (due to Panorama having access to all the same tabs), does it make more sense for those tabs just exist outside of any window until they become displayed?

Even further, might it be possible some day to show the same exact tab in two windows? The "all windows" UI certainly lends itself to that (just open two windows, take them both to Panorama, and click on the same tab in each), though it seems problematic technically, and not of much benefit from a use standpoint. I could imagine more easily, however, wanting to be in the same group on two windows, just not the same tab. Even that isn't possible with the current browser architecture.

Maybe all of that is too big to chew in one chunk, but at the very least we need to figure out where we're going, and the browser team as whole needs to be behind it.

I don't know if this bug is the best place for this discussion, but it seems like a fine place to start. If you're new here, you can ignore much of the comments above, as it's mostly old history.
See Also: → 645073
Target Milestone: Future → ---
>Even further, might it be possible some day to show the same exact tab in two
>windows? The "all windows" UI certainly lends itself to that (just open two
>windows, take them both to Panorama, and click on the same tab in each)

This is something that has also come up in discussion when app tabs are in every window.  For panorama, a reasonable alternative is that when you navigate window  A to a particular tab group, if that group is simultaneously open in window B, then window B switches to displaying tab view.
Assignee: nobody → tim.taubert
bugspam
Blocks: 588058
(In reply to comment #38)

> Another area to deal with is the fact that it's currently not possible for a
> browser window to exist without at least one tab (see discussion in bug 576427,
> as well as comment 30 here). We need to decide if it's okay for there to be a
> tab-less window (just showing Panorama). 

This sounds reasonable. You close the last tab in a window and the common Panorama appears. If this is the last tab in a session then there is a decision to be made whether to close the window or show an empty Panorama.

> For that matter, I believe it's currently not possible for a tab to exist
> without being attached to a window, but maybe we should allow this? Already the
> tabs in Panorama groups other than the one you're currently looking at seem to
> be detached from the window (though they're just hidden). Once those tabs can
> easily move from window to window (due to Panorama having access to all the
> same tabs), does it make more sense for those tabs just exist outside of any
> window until they become displayed?
> 
> Even further, might it be possible some day to show the same exact tab in two
> windows? The "all windows" UI certainly lends itself to that (just open two
> windows, take them both to Panorama, and click on the same tab in each), though
> it seems problematic technically, and not of much benefit from a use
> standpoint. I could imagine more easily, however, wanting to be in the same
> group on two windows, just not the same tab. Even that isn't possible with the
> current browser architecture.
> 
> Maybe all of that is too big to chew in one chunk, but at the very least we
> need to figure out where we're going, and the browser team as whole needs to be
> behind it.
> 
> I don't know if this bug is the best place for this discussion, but it seems
> like a fine place to start. If you're new here, you can ignore much of the
> comments above, as it's mostly old history.
No longer blocks: 603789
No longer blocks: 588217
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
I just stumbled across this problem today. I had a window with a single tab in which I had open on my second monitor. Having decided to come back to it later, I thought I'd file it away in Panorama rather than being forced to bookmark it and forget about it. However upon clicking the Tab Candy button, I had no groups whatsoever. Hopefully we'll be able to globalise Tab Groups soon.
Blocks: 765422
Blocks: 792794
Assignee: ttaubert → nobody
Excuse me, but is this really a bug?

I think the solution isn't to 'mass move' all the tabs into a single panorama window, its probably better to stack panorama window sessions so that every window has its own panorama view, and a master view can also be pulled up which allows various windows to be focused etc.
(In reply to smaudet from comment #52)
> Excuse me, but is this really a bug?
> 
> I think the solution isn't to 'mass move' all the tabs into a single
> panorama window, its probably better to stack panorama window sessions so
> that every window has its own panorama view, and a master view can also be
> pulled up which allows various windows to be focused etc.

Looking for the tabcandy code now, seeing if I can find what the implementation would be...
As a panorama user, I vote that at the very least losing the panorama groups by closing windows in the "wrong" order is definitely very undesirable.

In my mind, a window is a view into a panorama group. You could argue that it's not really the case, but bear with me. A window being a view onto a panorama group is how I *want* it to behave; that's the most convenient model.

The way I use them, each panorama group stores some related tabs, like work on a particular project. With this use case, it makes very little sense that a second window is unable to switch to a group created in another window. This use case totally _begs_ for the panorama groups to be shared across all windows.

I personally don't want to see a "master view" showing various windows and their panorama groups. No, to me the panorama groups are the ultimate top-level view, and each window is merely attached to one group. That would be grand.


P.S. With regard to "What happens if you open the same group in two windows at the same time?". I say a group can only be active in one window at a time. This solves all problems with keeping things in sync and meaningful. If you click a group that's already opened in another window, Firefox could simply bring that window to front and focus it.
Panorama is getting removed soon - Bug 836758 so all bugs regarding it (or at least the extensive ones) are probably going to get marked RESOLVED WONTFIX pretty soon.
Status: NEW → UNCONFIRMED
Ever confirmed: false
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: