Closed Bug 468808 Opened 11 years ago Closed 9 years ago

Tabs can't be reordered

Categories

(Thunderbird :: Mail Window Front End, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a3

People

(Reporter: binco, Assigned: schmid-thomas)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: ux-control, Whiteboard: [UXprio])

Attachments

(1 file, 14 obsolete files)

54.41 KB, patch
asuth
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1b3pre) Gecko/20081204 Thunderbird/3.0b1

In 3.0b1 it is not possible to change the order of the Tabs linke it is in Firefox.

Reproducible: Always

Steps to Reproduce:
1.Open a Folder in a new Tab
2.try to move the new Tab to the first position
Severity: normal → enhancement
Component: General → Mail Window Front End
QA Contact: general → front-end
Version: unspecified → Trunk
Possible duplicate of bug 469964.
Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Duplicate of this bug: 469964
Blocks: tabsmeta
Duplicate of this bug: 519439
Whiteboard: [UXprio]
Attached patch Implements tab reordering (obsolete) — Splinter Review
This patch implements reordering tabs in thunderbird. Apply patch to chrome\messenger\content\messenger.tabmail.xml 

Tabs within the same window are simply swaped. If a tab is dragged to a different window, session restore is used to migrate it.
Thomas, you might want to do a `hg diff` from the comm-central repository instead. More information here: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_diff_and_patch_files.3f

Also, please feel free to request for a review by setting the review flag to ? and selecting a reviewer by entering the email address.
Just to add to what Gary said, you'll need to ask :clarkbw for a ui-review, and you'll probably want to pick someone from https://wiki.mozilla.org/Modules:Thunderbird#Tab_UI to do the code review.

Later,
Blake.
Attached patch Tab reordering as hg diff (obsolete) — Splinter Review
Attachment #500773 - Attachment is obsolete: true
Attachment #500774 - Attachment is obsolete: true
Attachment #500828 - Flags: review?(bugmail)
Attachment #500828 - Flags: ui-review?(clarkbw)
Comment on attachment 500828 [details] [diff] [review]
Tab reordering as hg diff

This looks very promising!  Thank you for taking this on!

A few notes:

- We require unit tests.  As this is UI stuff, we would use mozmill tests rather
  than xpcshell tests.  Our page on this is:
  https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing

  I don't think we have any existing drag/drop tests, but
  EventUtils.js has synthesizeDragStart and synthesizeDrop methods that should
  hopefully work.

  If you need help with this, please post on the tb-planning list or the
  mozilla.dev.apps.thunderbird mailing list/newsgroup.


- There are almost no situations where alert()s should be committed to the tree.
  (I understand that you likely left those in for debugging.)  You may also
  want to consider using dump("message\n") for temporary debugging that may
  be less annoying.  (You will need to be running a debug build or have enabled
  the "browser.dom.window.dump.enabled" preference.  See
  https://developer.mozilla.org/en/setting_up_extension_development_environment
  for other good stuff.

  For exceptional error cases where you want to log something to the error console
  to assist in QA, you would want to use Application.console.log("message") a la
  https://developer.mozilla.org/en/Toolkit_API/extIConsole


- Coding style-wise, please stick to 80 columns and put spaces around operators,
  especially in for loops.  Example:  for (let i = 0; i < 10; i++).


- You have some dead code in moveTabTo.


- You need to guard against moving the first tab.  We have a canClose flag which
  is set to false for the tab, which is what you should probably check.  (Or we
  can rename it to canCloseOrMove or something like that.)  The rationale for this
  is that we need to have a 3-pane visible at all times because if the user
  manages to get rid of the 3-pane, it can become impossible to get it back.  So,
  we just make the first tab permanently a 3-pane and leave it at that.


- If any of the code is ported from the firefox code, please explicitly call it
  out in comments so that it's obvious where the code came from and so that we
  know if there's someplace we might need to port bugfixes to.  I am assuming
  that this is the case for some of the dragging logic where there aren't as many
  comments.  If this is not the case, some more comments please :)


- I think you can lose a lot of the ellipsis in comments without hurting
  readability, and in many cases I think it will improve things.  This is
  especially true for a comment line immediately following another comment line.


Again, thank you for pursuing this.  I know a lot of users will be very excited
to be able to drag their tabs around!
Attachment #500828 - Flags: review?(bugmail) → feedback+
As this is my first bug, what are the next steps?

(In reply to comment #10)
> - We require unit tests.  
[...]
>   If you need help with this, please post on the tb-planning list or the
>   mozilla.dev.apps.thunderbird mailing list/newsgroup.

ok I'll post to the mailing list, as I've no clue how to use mozmill.

> - There are almost no situations where alert()s should be committed to the
> tree.
>   (I understand that you likely left those in for debugging.)  You may also
>   want to consider using dump("message\n") for temporary debugging that may
>   be less annoying.  (You will need to be running a debug build or have enabled
>   the "browser.dom.window.dump.enabled" preference.  See
>   https://developer.mozilla.org/en/setting_up_extension_development_environment
>   for other good stuff.

Your right the alerts are leftovers from debugging. I've either removed them or 
replaced them with simple return statements.

> - Coding style-wise, please stick to 80 columns and put spaces around
> operators,
>   especially in for loops.  Example:  for (let i = 0; i < 10; i++).

I've tried to fix that. 

> - You have some dead code in moveTabTo.
ok, it's removed

> - You need to guard against moving the first tab.  We have a canClose flag
> which
>   is set to false for the tab, which is what you should probably check.  (Or we
>   can rename it to canCloseOrMove or something like that.)  The rationale for
> this
>   is that we need to have a 3-pane visible at all times because if the user
>   manages to get rid of the 3-pane, it can become impossible to get it back. 
> So,
>   we just make the first tab permanently a 3-pane and leave it at that.

is already implemented. See line 1920-1924 of last patch. 

> - If any of the code is ported from the firefox code, please explicitly call it
>   out in comments so that it's obvious where the code came from and so that we
>   know if there's someplace we might need to port bugfixes to.  I am assuming
>   that this is the case for some of the dragging logic where there aren't as
>   many comments.  If this is not the case, some more comments please :)

as most of the code in tabmail the hittests in dragover and dragend are based on firefox code.
Thus I've left comments referencing to firefox bugs in the code. But most of the code 
needed changes, as tabmail and tabbrowser seem to be similar but are slightly different.

> - I think you can lose a lot of the ellipsis in comments without hurting
>   readability, and in many cases I think it will improve things.  This is
>   especially true for a comment line immediately following another comment
> line.

well it's not very common but rather useful for structuring comments. When skimming 
a text/code, you can skip all lines starting with ellipsis. If a comment ends with 
ellipsis a new logic/program flow is stating.  


Some Questions:

- Is there a better way to obtain the tabmail element from a tab element? I am 
  currently using this.parentNode.parentNode.parentNode.parentNode

- Should detaching a tab to a new window be supported? If so should the new window 
  always contain a 3-pane or should the tab decide? (e.g. when detaching a message window) 

- What events should be fired when switching tabs?

- If you are moving tabs between windows, it uses session restore to migrate the 
  tab. This means it will be closed and reopened in the new window. This works
  only if the tab supports session restore. Any other ideas concering this issue?
Attached patch tabmail patch v2 (wip) (obsolete) — Splinter Review
Attachment #500828 - Attachment is obsolete: true
Attachment #500828 - Flags: ui-review?(clarkbw)
Attachment #500889 - Flags: ui-review?(clarkbw)
Attached patch tabmail patch v3 (wip) (obsolete) — Splinter Review
Previous patch was incomplete and thus buggy.

What changed:
  - added _tabDropIndecator to pair with firefox
  - added persistTab() method 
      simplifies code, might be useful for Bug 504122 - provide undo close tab)
  - fix code style 
      spaces and columns
      removed deadCode
      cleaned up comments
  - ensure that only "canClose" tabs can be dragged
Attachment #500889 - Attachment is obsolete: true
Attachment #500900 - Flags: ui-review?(clarkbw)
Attachment #500889 - Flags: ui-review?(clarkbw)
clarkbw, question for you in here.


(In reply to comment #11)
> As this is my first bug, what are the next steps?

clarkbw signs off on the user experience (UX) aspects, we add unit tests, I review.

Especially since this is your first patch, I can help out with unit tests, but things can go much faster if you write them or take them as far as you can.


> (In reply to comment #10)
> ok I'll post to the mailing list, as I've no clue how to use mozmill.

You can also find live help in the #maildev IRC channel:
https://developer.mozilla.org/en/Extensions/Thunderbird


> well it's not very common but rather useful for structuring comments. When
> skimming 
> a text/code, you can skip all lines starting with ellipsis. If a comment ends
> with 
> ellipsis a new logic/program flow is stating.  

We're pretty flexible on comments, so if that works for you, it works for me.

 
> - Is there a better way to obtain the tabmail element from a tab element? I am 
>   currently using this.parentNode.parentNode.parentNode.parentNode

There is only one tabmail instance per window instance, so document.getElementById("tabmail") will always be the right tabmail for the given document.

 
> - Should detaching a tab to a new window be supported? If so should the new
> window 
>   always contain a 3-pane or should the tab decide? (e.g. when detaching a
> message window) 

This is basically a clarkbw question (in terms of new window support at all).  Based on how we currently do things, we are going to need to let the 3-pane window


> - What events should be fired when switching tabs?

We currently don't generate any special DOM events like firefox.  Our approach generates notifications via the tab monitor logic.  (Extensions can probably also listen for events from the tabbox widgets we inherit from too.)  As long as we continue to generate the current set of events with equivalent semantics, all should be well.


> - If you are moving tabs between windows, it uses session restore to migrate
> the 
>   tab. This means it will be closed and reopened in the new window. This works
>   only if the tab supports session restore. Any other ideas concering this
> issue?

I don't think there's any safe, magic way to automatically accomplish this.  We could certainly allow tabs to implement a "transplantIntoWindow" method or something like that where they are given the opportunity to move their DOM sub-tree and do whatever else to update their state to avoid a lot of redundant processing or information loss.  But since we definitely need the persist/restore logic in there as a fallback case, it makes the most sense to defer those changes as an enhancement for another bug for when we have a serious performance need for that optimization.
(In reply to comment #14)
> > - Should detaching a tab to a new window be supported? If so should the new
> > window 
> >   always contain a 3-pane or should the tab decide? (e.g. when detaching a
> > message window) 
> 
> This is basically a clarkbw question (in terms of new window support at all). 
> Based on how we currently do things, we are going to need to let the 3-pane
> window

whoops, trailed off there...

Every 3-pane window needs a 3-pane as its leftmost tab.  So if we drag a message tab into a new window and we are going to support the new windows, the right thing to me would seem to be to:
a) Create the new window and have it have a 3-pane as the first/leftmost tab.
b) Have the second tab be the message tab, and make that the active/displayed tab.
Assignee: nobody → schmid-thomas
Status: NEW → ASSIGNED
Attached patch tabmail patch v4 (wip) (obsolete) — Splinter Review
All drag'n'drop features are now implemented. This fixes as a side effect some other bugs.

What changed:

* Detaching Tabs works now, needed changes to msgMail3PaneWindow.js
  Fixes bug 521260

* Undo Closing tabs (history is currently hard coded to 10 tabs)
  Fixes bug 504122

  Should the history of closed tabs be restored by session restore, 
  as it is in Firefox? 

* session restore should work as it should. Previous patch broke it
Attachment #500900 - Attachment is obsolete: true
Attachment #501041 - Flags: ui-review?(clarkbw)
Attachment #501041 - Flags: feedback?(bugmail)
Attachment #500900 - Flags: ui-review?(clarkbw)
I just noticed that the last patch (501041) contains a tiny bug. 

  "return true;" in line 602 of "msgMail3PaneWindow.js" should be removed.
Comment on attachment 501041 [details] [diff] [review]
tabmail patch v4 (wip)

Exactly as asuth explained in comment 15

Creating new windows from tabs would be what most users expect coming from Firefox so it's likely to be requested as a feature.
Attachment #501041 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 501041 [details] [diff] [review]
tabmail patch v4 (wip)

Per previously provided feedback, the general strategy is sound.  Tests are needed before additional review steps can be taken; I'll mark it in the whiteboard as I have no spare cycles right now to provide them.
Attachment #501041 - Flags: feedback?(bugmail) → feedback+
Whiteboard: [UXprio] → [UXprio][needs tests]
Attached file Mozmill test for Bug (obsolete) —
This took me way longer than I ever expected. Especially since drag'n'drop implementation in MozMill very buggy and relies heavily on miracles. I would not give too much on this test, but anyhow it seems to be working.
 
The script should drag the second tab onto the fourth tab. Thus the test needs least 4 open tabs to succeed. Until now I did not find a reliable way to open message tabs with MozMill. 

Test for Dragging a tab to an other window and detaching a tab might follow. But the latter might be too complex to be synthesized.
Wow, it's awesome that you got mozmill to generate drag-and-drop events.  I presume that indeed was not fun.  Thank you very much for embarking on this journey of at-least-a-little-bit-of-frustration and making it this far already!

test-folder-display-helpers has helper logic related to opening tabs:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js

To see how it's used, check out the folder-display tests:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/

The helper logic includes the ability to assert about the state of what is displayed on the tab.  In order to check that the tab meta-info was correctly displaced as well, you will want to use these methods to make sure that the tab is in fact correctly displaying the right data.  This includes immediately, if applicable, as well as after switching to another tab and back, so make sure that the data structures were updated as well as the "current" stuff.

Since drag-and-drop between windows is presumably very similar to drag-and-drop within a window, it's probably fine to directly prod the internals to convince them that we are dragging a tab from another window if it turns out to be hard to accomplish that the most realistic way.


It looks like your mozmill test is intended to be run in the extension in a live Thunderbird instance.  We actually run our tests from a command-line driver that creates a new profile for the test and what not.  More details here (and from looking at the existing examples, as linked above):
https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing

And again, please feel free to ask questions here on the bug or in the mailing list or IRC or whatnot.
This Mozmill script drags a tab into the content area. The tab should detach an open up in a new window.

As with the previous test, it is build for the mozmill extension and requires atleast two open tabs. 

Next steps are:
 * a test for dragging tabs between two windows.
 * using the test-folder-display-helpers to open and verify tabs
 * converting to mozmill commandline scripts
this one was way easier than expected. Changing the first test's drop target was sufficient.
Whiteboard: [UXprio][needs tests] → [UXprio][needs reviews of the tests]
Whiteboard: [UXprio][needs reviews of the tests] → [UXprio][needs review once tests are complete]
Attachment #506269 - Flags: review?(bugmail)
Attachment #506983 - Flags: review?(bugmail)
Attachment #506269 - Flags: review?(bugmail)
Comment on attachment 506983 [details]
Mozmill test for moving Tab between windows

Ludo, I'm on this, you don't need to set flags.
Attachment #506983 - Flags: review?(bugmail)
Attached file tests as mozmill commandline scripts (obsolete) —
The three previous test converted into a mozmill command line script.

It could be included into "test-displaying-messages-in-folder-tabs". As drag'n'drop relies on some magic which might cause side effects, I suggest to keep it as a separate test script. What would be a good filename? 

Furthermore I did not find any code to ensure the tab content is loaded correctly. Do you have any suggestions or ideas concerning this issue?

The next step is cleaning up the patch, so that it's ready for review. I'll try to do that within the next days.
Attachment #504577 - Attachment is obsolete: true
Attachment #506269 - Attachment is obsolete: true
Attachment #506983 - Attachment is obsolete: true
Attachment #508165 - Flags: feedback?(bugmail)
It would be useful if you could make a generic drag-and-drop function in mail/test/mozmill/shared-modules, since there are a few other drag-and-drop operations that need tests (e.g. drag-and-drop messages). Bug 515776 resolves this, but we'd have to upgrade Mozmill to use that.
(In reply to comment #26)
> It would be useful if you could make a generic drag-and-drop function in
> mail/test/mozmill/shared-modules, since there are a few other drag-and-drop
> operations that need tests (e.g. drag-and-drop messages). Bug 515776 resolves
> this, but we'd have to upgrade Mozmill to use that.

well, implementing a generic drag'n'drop function is difficult, it would need to triggers so many different events. It took me several hours and some magic to figure out how get drag'n'drop for this bug's tests running. So it seems to me as if Bug 515776 does not trigger dragEnd events. And thus it most likely will not work for this bug's test.
Attached patch tabmail patch v5 (obsolete) — Splinter Review
What changes
  * strings are now localized
  * tab's new index did not honor old index
  * support drag images
Attachment #501041 - Attachment is obsolete: true
Attachment #508184 - Flags: review?(bugmail)
While testing around I recognized, that moving tabs from gloda/facet search is currently not possible. These tabs do not support session restore thus it's not possible to move them around.
I suppose Bug 467927 addresses this issue. Should I add into depends on field?
(In reply to comment #27)
> well, implementing a generic drag'n'drop function is difficult, it would need
> to triggers so many different events. It took me several hours and some magic
> to figure out how get drag'n'drop for this bug's tests running. So it seems to
> me as if Bug 515776 does not trigger dragEnd events. And thus it most likely
> will not work for this bug's test.

If you just put a basic drag-and-drop function in shared-modules that does what you need, then other people (probably me) could tweak it as needed or add new functions to do other operations. Don't feel like you need to make a "perfect" drag-and-drop function. :)
Attached patch tabmail patch v6 (obsolete) — Splinter Review
Tab not supporting sessionrestore can now be reorder within the same window/tabbar but not detached nor moved to a different window.
Attachment #508184 - Attachment is obsolete: true
Attachment #508302 - Flags: review?(bugmail)
Attachment #508184 - Flags: review?(bugmail)
Whiteboard: [UXprio][needs review once tests are complete] → [UXprio][needs review]
Comment on attachment 508165 [details]
tests as mozmill commandline scripts

(In reply to comment #25)
> Created attachment 508165 [details]
> tests as mozmill commandline scripts
> 
> The three previous test converted into a mozmill command line script.

Looking good!  Note that it's best to provide the tests as a patch rather than a file since it makes it easier to import or directly throw at the try server, etc.

> It could be included into "test-displaying-messages-in-folder-tabs". As
> drag'n'drop relies on some magic which might cause side effects, I suggest to
> keep it as a separate test script. What would be a good filename? 

The way our mozmill test runner works is that each directory in mail/test/mozmill gets run in its own Thunderbird session; all the files in that directory get run in the same session.  Unless tests are dealing with the same concept, they should usually go in their own file.

In this case, it sounds like maybe this test should go in its own directory so if it fails it does not upset any other tests?

> Furthermore I did not find any code to ensure the tab content is loaded
> correctly. Do you have any suggestions or ideas concerning this issue?

assert_selected_and_displayed (or assert_displayed) covers "folder" and "message" tabs:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1803

There is no such support for the gloda faceted search HTMLish UI but since none of the gloda things can be moved, that doesn't really matter.



Other comments:

- Instead of doing your own getElementById traversal or:
    'new elementslib.Lookup(aWindow.document,'/id("messengerWindow")/id("tabmail-container")/id("tabmail")').getNode()'
   you can do:
    'mc.e("domid")'

  This is using the helper methods put on the window controllers by test-window-helpers:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-window-helpers.js#607

  In the event you need an elementslib.Elem, you can just use 'mc.eid("domid")'
  instead.


- You need a test for the undo close tab functionality you folded into the
  patch.  There was a patch on bug 504122, but it seems to assume that lightning
  is available (given that it is using the "tasks" tab type), but that is never
  the case on our tinderboxen.


- Please factor out some of the drag-and-drop synthesis stuff, even if only to
  a function in the same file.  It would aid readability if not the benefit of
  actually reuse and simplify any bug fixin' later on.
Attachment #508165 - Flags: feedback?(bugmail) → feedback+
Comment on attachment 508302 [details] [diff] [review]
tabmail patch v6

I reviewed this on my reviewboard instance.  The pretty review is here:
http://reviews.visophyte.org/r/508302/

If you use emacs or vim, see the following blog post and comments on how to have your editor help you out jumping to edit points:
http://www.visophyte.org/blog/?p=447

And here is the review exported for bugzilla:

This is shaping up to be awesome!  Thank you very much for your persistence on
this and kudos on the comprehensiveness!  Please note that I have some syntax
nits in there and I feel bad about not pointing them out in earlier revisions;
as my penance I'll fix them myself if you don't get to them (all).


on file: mail/base/content/msgMail3PaneWindow.js line 605
>   // it's now safe to load extra Tabs.
>   setTimeout(loadExtraTabs, 0);

good call on simplifying things by moving this in here!


on file: mail/base/content/msgMail3PaneWindow.js line 638
>   // we got no action, so suppose its "legacy" code
>   if (!tab.action)

Please check for (!("action" in tab)) instead, so that strict mode will not
complain about us accessing "action" if it does not exist.  (In other words,
please do not depend on the fact that JS will return undefined for attributes
that do not already exist on the object, as that makes strict mode report
warnings and we do like those warnings.)


on file: mail/base/content/msgMail3PaneWindow.js line 639
>   if (!tab.action)
>   {

nit: Except for existing files that are uniformly next-line, squiggly braces
should be same-line for JS code in mail/.  This file is still somewhat of a
mess consistency-wise, but the trend is same-line.


on file: mail/base/content/msgMail3PaneWindow.js line 650
>   // This is used if a tab is deatached to a new window. 

typo: should be detached


on file: mail/base/content/msgMail3PaneWindow.js line 651
>   if (tab.action == "restore")

Perhaps it would be better to use "migrate" or "spawn", or something else in
order to avoid confusion from the actual state restoration case?


on file: mail/base/content/msgMail3PaneWindow.js line 653
>     for (let i = 0; i < tab.tabs.length; i++)
>       tabmail.restoreTab(tab.tabs[i]);

restoreTab can fail... I'm not sure we have a great response to this, but
perhaps add a comment noting that although restoreTab can end up not doing
anything, there is nothing we can really do about it?


on file: mail/base/content/tabmail.xml line 659
>       <method name="undoCloseTab">

Please document this method (briefly) up in the giant ugly XML comment at the
top of the file.


on file: mail/base/content/tabmail.xml line 663
>           if(!json)

nit: space between if and paren.


on file: mail/base/content/tabmail.xml line 676
>       <method name="closeTab">
>         <parameter name="aOptTabIndexNodeOrInfo"/>
>         <body><![CDATA[

There is some new complexity around undo close that you'll need to tackle. 
Specifically, not too long ago Jim updated the message tab logic to close when
the message in the tab gets deleted.  It obviously does not make a lot of
sense to add tabs to the undo list that we cannot possibly restore.

And actually, it looks like all the cases in messageDisplay.js where it closes
itself should not be undone see:
http://mxr.mozilla.org/comm-central/source/mail/base/content/messageDisplay.js#535

I would suggest adding an extra optional argument called aDoNotAllowUndo or
something and passing true in those three cases (with the unspecified value of
undefined being treated as false).

You would want to then add a check to the unit test for that logic to make
sure it does not add an entry to the undo list.  The test file in question is:
mail/test/mozmill/folder-display/test-close-window-on-delete.js


on file: mail/base/content/tabmail.xml line 731
>       <method name="warnAboutClosingTabs">
>         <body>

I presume this snuck in from another patch you are working on?  Please
separate this bit back out.


on file: mail/base/content/tabmail.xml line 831
>       <method name="replaceTabWithWindow">

Please document (briefly).


on file: mail/base/content/tabmail.xml line 848
>             // Converting to JSON and back again creates clean javascript 
>             // object with absolutely no references to our current window.
>             let tab = JSON.parse(JSON.stringify(tab));            

Is there some code in the system that needs this?  I like the preventative
measure as it would break particularly ill-behaved code, just hope you are
just being proactively defensive here.


on file: mail/base/content/tabmail.xml line 860
>       <method name="moveTabTo">

Please document.


on file: mail/base/content/tabmail.xml line 878
>           //remove the entries form tabInfo, tabMode and tabStrip

nit: space after // here and in a few places below.


on file: mail/base/content/tabmail.xml line 899
>           for (let i = 0; i < this.tabInfo; i++)
>           {

I presume you meant this.tabInfo.length.

Also, squiggly brace on same line!


on file: mail/base/content/tabmail.xml line 914
>           //update this.tabModes          
>           tab.mode.tabs.splice(lastIdx, 0, tab);

You go through a lot of logic to accomplish this; please add verification
logic to your mozmill tests to verify that this logic is doing the right
thing.


on file: mail/base/content/tabmail.xml line 919
>           // TODO Dispatch Event like firefox?
>           /*var evt = document.createEvent("UIEvents");
>           evt.initUIEvent("TabMove", true, false, window, oldPosition);
>           aTab.dispatchEvent(evt);*/

remove this dead code, please.


on file: mail/base/content/tabmail.xml line 926
>        <method name="persistTab">

Please document.  (I realize persistTabs was not documented, but I think the
idea was that it was more implementation internal for session-restore only and
now we're getting a lot more callers.)


on file: mail/base/content/tabmail.xml line 929
>            /* Returns null in case persits fails */

typo: persist


on file: mail/base/content/tabmail.xml line 1000
>       <method name="restoreTab">

please document.


on file: mail/base/content/tabmail.xml line 1038
>           for each (let [iTab, tabState] in Iterator(tabs))
>           {

Here you changed one of the squigglies from same-line to next-line; I'm on to
you! ;)


on file: mail/base/content/tabmail.xml line 1302
>             // by default "close other tabs" is disabeled...

s/disabeled/disabled/

(Mainly concerned for meta-/ emacs completion getting fooled.)


on file: mail/base/content/tabmail.xml line 1304
>               .getAnonymousElementByAttribute(this,"anonid", "closeOtherTabs")

nit: whitespace after comma (and below too)


on file: mail/base/content/tabmail.xml line 1308
>             for (let i = 0; i < this.tabInfo.length; i++)
>             {

nit: same-line squiggly reminder.


on file: mail/base/content/tabmail.xml line 1328
>               .setAttribute("disabled",tab.canClose ? "false" : "true")

Should we be checking for the tab mode/tab type having a persistTab function
too?  Since we will fail on faceted search tabs, it seems better to avoid
providing the option in the first place.


on file: mail/base/content/tabmail.xml line 1993
>         let tabmail = this.parentNode.parentNode.parentNode;

Please just use document.getElementById("tabmail") per my response to your
question about this.


on file: mail/locales/en-US/chrome/messenger/messenger.dtd line 9
> <!ENTITY closeTabCmd.accesskey "C">
> <!ENTITY closeOtherTabsCmd.label "Close Other Tabs">
> <!ENTITY closeOtherTabsCmd.accesskey "o">
> <!ENTITY undoCloseTabCmd.label "Undo Close Tab">
> <!ENTITY undoCloseTabCmd.accesskey "U">
> <!ENTITY moveToNewWindow.label "Move to New Window">
> <!ENTITY moveToNewWindow.accesskey "W">

For localization reasons (so the localizers see something has changed), I
think you may need to change the entity name if you change the values of the
access keys?

This will also require ui-review sign-off unless you've discussed the phrasing
with clarkbw on IRC or elsewhere.  (The phrasing was "Reopen Last Tab" for bug
504122 it looks like.)


on file: mail/themes/qute/mail/tabmail-aero.css line 211
> .tab-drop-indicator {
>    list-style-image: url("chrome://messenger/skin/icons/tabDragIndicator.png");
>    position: relative;
>    z-index: 2;
> }

Will this require changes in other themes too?
Attachment #508302 - Flags: review?(bugmail) → review-
(In reply to comment #34)
Thank you for reviewing.

> Comment on attachment 508302 [details] [diff] [review]
> on file: mail/base/content/msgMail3PaneWindow.js line 651
> >   if (tab.action == "restore")
> 
> Perhaps it would be better to use "migrate" or "spawn", or something else in
> order to avoid confusion from the actual state restoration case?

well, as migrating tabs and session restore use the same method, I would stick to "restore". So it's obvious it's closing and restoring a tab from instead of a smooth and seamless migration/transition. 

> on file: mail/base/content/msgMail3PaneWindow.js line 653
> >     for (let i = 0; i < tab.tabs.length; i++)
> >       tabmail.restoreTab(tab.tabs[i]);
> 
> restoreTab can fail... I'm not sure we have a great response to this, but
> perhaps add a comment noting that although restoreTab can end up not doing
> anything, there is nothing we can really do about it?

I added a comment to the method, that restoring tabs will fail silently. As the old tab is already closed, there is nothing we can do at this point. But I think it's rather unlikely to happen. It can only happen when moving tabs between windows and the tab's restore tab method is faulty. In case the tab has no restore method, it can't be moved to a new window.

> on file: mail/base/content/tabmail.xml line 676
> >       <method name="closeTab">
> >         <parameter name="aOptTabIndexNodeOrInfo"/>
> >         <body><![CDATA[
> 
> There is some new complexity around undo close that you'll need to tackle. 
> Specifically, not too long ago Jim updated the message tab logic to close when
> the message in the tab gets deleted.  It obviously does not make a lot of
> sense to add tabs to the undo list that we cannot possibly restore.
> 
> And actually, it looks like all the cases in messageDisplay.js where it closes
> itself should not be undone see:
> http://mxr.mozilla.org/comm-central/source/mail/base/content/messageDisplay.js#535
> 
> I would suggest adding an extra optional argument called aDoNotAllowUndo or
> something and passing true in those three cases (with the unspecified value of
> undefined being treated as false).

that's a good point, when thinking about it, I recognized there is even more code, where undo should not possible. For example when detaching a tab, or moving a tab to a new window... ;-)

This also showed me that tab history needs to be more sophisticate. Imagine you have three tabs A,B and C. You close the second one B, it will be store with restore index set to two. Now the first tab A is deleted without any undelete history. Calling undo will open the tab B after the third tab C. As tab C has now index 1 and B was stored as 2. Until now I did not find an easy solution for this. Any suggestions.

> You would want to then add a check to the unit test for that logic to make
> sure it does not add an entry to the undo list.  The test file in question is:
> mail/test/mozmill/folder-display/test-close-window-on-delete.js

I guess as tab history is more complex than expected, we'll need a test anyhow.

> on file: mail/base/content/tabmail.xml line 848
> >             // Converting to JSON and back again creates clean javascript 
> >             // object with absolutely no references to our current window.
> >             let tab = JSON.parse(JSON.stringify(tab));            
> 
> Is there some code in the system that needs this?  I like the preventative
> measure as it would break particularly ill-behaved code, just hope you are
> just being proactively defensive here.

it's just proactive defense. As we first close the tab, and then hope we can restore it in a new window, I wanted make this mechanism as solid as possible. At least this ensures no tab is lost because of bad JSON.

> on file: mail/base/content/tabmail.xml line 899
> >           for (let i = 0; i < this.tabInfo; i++)
> >           {
> 
> I presume you meant this.tabInfo.length.

yepp, good catch. 


> on file: mail/base/content/tabmail.xml line 1328
> >               .setAttribute("disabled",tab.canClose ? "false" : "true")
> 
> Should we be checking for the tab mode/tab type having a persistTab function
> too?  Since we will fail on faceted search tabs, it seems better to avoid
> providing the option in the first place.

good suggestion. I'll fix that.

> on file: mail/locales/en-US/chrome/messenger/messenger.dtd line 9
> > <!ENTITY closeTabCmd.accesskey "C">
> > <!ENTITY closeOtherTabsCmd.label "Close Other Tabs">
> > <!ENTITY closeOtherTabsCmd.accesskey "o">
> > <!ENTITY undoCloseTabCmd.label "Undo Close Tab">
> > <!ENTITY undoCloseTabCmd.accesskey "U">
> > <!ENTITY moveToNewWindow.label "Move to New Window">
> > <!ENTITY moveToNewWindow.accesskey "W">
> 
> For localization reasons (so the localizers see something has changed), I
> think you may need to change the entity name if you change the values of the
> access keys?
> 
> This will also require ui-review sign-off unless you've discussed the phrasing
> with clarkbw on IRC or elsewhere.  (The phrasing was "Reopen Last Tab" for bug
> 504122 it looks like.)

To be honest I just copied the entities from Firefox. I did not realize that I was changing access keys. My intention was using the same names as firefox, so that's its somehow consistent within Mozilla products. So contacting clarkbw is a good idea.
 
> on file: mail/themes/qute/mail/tabmail-aero.css line 211
> > .tab-drop-indicator {
> >    list-style-image: url("chrome://messenger/skin/icons/tabDragIndicator.png");
> >    position: relative;
> >    z-index: 2;
> > }
> 
> Will this require changes in other themes too?

good point, actually I don't know. But should be really easy to figure out. In case you see a drop indicator as on Firefox, no changes are needed to other themes. Can you test it on XP || MAC || Linux?  I can't test it as all my machines are running windows 7.

I'll post an updated patch as soon as I resolved the history stuff. Could take some days.
(In reply to comment #35)
> Can you test it on XP || MAC || Linux?  I can't test it as all my
> machines are running windows 7.

Yes, I have all platforms available.  FYI, I think you can trick Thunderbird into showing its XP skin on Windows 7 by telling Windows to run it in compatibility mode.
Your trick works perfectly. Drop indicators are shown in xp compatibility mode, so I suppose the no theme work is needed. I'll verify if the theme change is really necessary on areo.
Attached patch tabmail patch v7 (obsolete) — Splinter Review
Updated according to review comments

Concerning tab history issue, I looked around how other software handles, all used the simple logic, to remember the tab position and restore it there. Just as it is implemented. I guess there is no better way to handle this.

Updated test will follow.
Attachment #508302 - Attachment is obsolete: true
Attachment #509627 - Flags: ui-review?(clarkbw)
Attached patch tabmail patch v8 (obsolete) — Splinter Review
What changed:
  drag and drop stuff is now factored out into helper functions. 
  test file moved into "/tabmail" directory
  new test for undoClose tab
  content of tabs is now verified
  checks tabModes.tabs is updated correctly
  simplified logic in moveTabTo
  ...
  
Next Steps:
  Waiting for UI ui-review, due to changes in "messenger.dtd".
Attachment #508165 - Attachment is obsolete: true
Attachment #509627 - Attachment is obsolete: true
Attachment #510118 - Flags: ui-review?(clarkbw)
Attachment #510118 - Flags: review?
Attachment #509627 - Flags: ui-review?(clarkbw)
Attachment #510118 - Flags: review? → review?(bugmail)
Looking fantastic!  I've pushed this to try:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=eeb0a29fafb8

I also included a patch on top of a (very minor de-bitrotted) version of the v8 patch that should be doing the right thing in terms of styling on all platforms.  The v8 patch had the following glitches:

- On linux at least, there was some apparent cropping of the drop arrow due to z-ordering or cropping or something.

- On mac the indicator actually runs the full height of the tab plus some more, and the whole thing was ending up vertically above the tap.

My patch relocates the tab-drop-indicator node to be in the tabs binding, like in Firefox, and updates our CSS to be like in firefox.  Additionally, instead of clobbering the margin top/bottom in the XBL (which was causing the mac displacement issue), we just do that in the theme styling.  See the try push for the exact patch for now.

Once the try builds happen I'll have clarkbw do his UI thing and I'll finish up my review pass and we should be good to go barring test failures.
The try build I pushed had some mozmill unhappiness that I'll need to look into tomorrow, but in short there may be a regression in close-on-delete logic.  I also glitched the windows style fixes (that was the one platform I did not locally build this cycle), but that should now be resolved.
Comment on attachment 510118 [details] [diff] [review]
tabmail patch v8

tried this out on both linux and mac and it looks good to me.
Attachment #510118 - Flags: ui-review?(clarkbw) → ui-review+
(In reply to comment #42)
> Comment on attachment 510118 [details] [diff] [review]
> tabmail patch v8
> 
> tried this out on both linux and mac and it looks good to me.

That is the try server build of course.
Sorry about the delay, there is a lot of debt/breakage with our mozmill setup that I'm dealing with while trying to figure out what tests are actually being regressed (versus which are flaky) and how to fix them.
re:strings

(In reply to comment #34)
> on file: mail/locales/en-US/chrome/messenger/messenger.dtd line 9
> > <!ENTITY closeTabCmd.accesskey "C">
> > <!ENTITY closeOtherTabsCmd.label "Close Other Tabs">
> > <!ENTITY closeOtherTabsCmd.accesskey "o">
> > <!ENTITY undoCloseTabCmd.label "Undo Close Tab">
> > <!ENTITY undoCloseTabCmd.accesskey "U">

This one is fine.

> > <!ENTITY moveToNewWindow.label "Move to New Window">
> > <!ENTITY moveToNewWindow.accesskey "W">

This should be "Open in a New Window", might as well match Firefox.
(In reply to comment #45)
> > > <!ENTITY moveToNewWindow.label "Move to New Window">
> > > <!ENTITY moveToNewWindow.accesskey "W">
> 
> This should be "Open in a New Window", might as well match Firefox.

Firefox actually uses "Move To New Window" 
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#34
Just so everyone's on the same page, "Open in a New Window" is 3.6 and "Move To New Window" is 4.0.
(In reply to comment #47)
> Just so everyone's on the same page, "Open in a New Window" is 3.6 and "Move To
> New Window" is 4.0.

I did not realize they changed it. For the sake of completeness it was changed in Bug 593646. 

Clarkbw which one should be used? The Firefox 4.0 or 3.6 naming?
Personally, I'd go with the Firefox 4.0 version, since Thunderbird 3.3 will be released after Firefox 4.0, and we should be consistent with our future contemporary.
(In reply to comment #48)
> Clarkbw which one should be used? The Firefox 4.0 or 3.6 naming?

Go with the FF 4.0 version, Bryan will be happy with that.
Attached patch tabmail patch v9Splinter Review
What changed:

Creating the drag image was buggy/broken

In case the user drags an incompatible dragtype over a tab it will activate this tab instead of ignoring the drag over event.
Attachment #510118 - Attachment is obsolete: true
Attachment #512917 - Flags: review?(bugmail)
Attachment #510118 - Flags: review?(bugmail)
Are there any updates concerning the test regressions? 

What are the next steps to get the patch to the trunk?
(In reply to comment #52)
> Are there any updates concerning the test regressions? 

I have been actively working on improving the 'mozmill failures on tinderbox' situation across the board with logging that gets exposed via a web interface.  This has taken longer than my best-case estimate, but progress has been quite good and I hope to have things tied up by Wednesday.

I have been saving this failure as a driving use-case to keep things realistic.  I am sorry it has been slowing down getting your patch in, though :(


> What are the next steps to get the patch to the trunk?

Sorry if I did not make it clear before; except for the test failures, the patch is basically good to go.  I expect the test failure to be something reasonably trivial and I will probably just fix it for you unless you prefer to do so yourself.

(If the patch was still actively in the throes of development I would likely not be using its test failure as my thunderbird-mozmill-testing-snafu-fixing use case for fear of slowing down your development roll.  I fear I am still doing so a bit in a different way.  I hope my attempts to improve the development testing experience end up helping more than hurting...)

If you have other patches you are working on that are dependent on this one, please do keep working on them (just layer them on top of this one.)  If those patches are ready for feedback or review, please just make sure to reference their dependence on the patch on this bug when you ask for feedback/review.
(In reply to comment #53)
> (In reply to comment #52)
> > Are there any updates concerning the test regressions? 
> 
> I have been actively working on improving the 'mozmill failures on tinderbox'
> situation across the board with logging that gets exposed via a web interface. 
> This has taken longer than my best-case estimate, but progress has been quite
> good and I hope to have things tied up by Wednesday.

The update/new news is that I have the logging gussied up and exposed via a web interface.  Unfortunately, actually killing our linux perma-oranges is proving to be quite problematic due to platform opacity, general thunderbird snafu-ness, and the fact that the failures don't happen as one-offs making the debugging option not particularly straightforward.
I've got the horrible permaoranges in folder-display licked, with the notable exception that part of our problem appears to have been the mouse event sequence we were generating for our horrible, hacky transient selection stuff when you right click on a thread pane row.  It turns out when you naively fix the brokenness of what we were doing, even more things break.  Luckily, I have battle scars from delving into the right-click transient selection logic so it should be pretty short work to fix that tomorrow.
I got mozmill super-green on linux locally.  Running with the patch with the fancy logging made it pretty obvious what the problem was; not all of the closeTab() calls in messageDisplay.js were updated to forbid undo.  When the attempt was made to persist the tabs, they exploded throwing an exception because msgHdr was null.  This broke tab closing for those cases.  Once the boolean flag is added, the mozmill run is green.  I will make the outstanding required changes for UX/localization (the access key changes need the entities rolled) and land this very late tonight/early tomorrow.
And it's in!  Great work on this patch, Thomas!

patch pushed to trunk with the noted closeTab undo change (and an extra try to avoid unexpected failures in that vein) and minor style fixes:
http://hg.mozilla.org/comm-central/rev/491fbcf7a1b4

styling fixes also pushed to trunk:
http://hg.mozilla.org/comm-central/rev/33614283e675

I just sanity checked the styling and operation of the functionality on all 3 platforms and all 4 themes.  (win and mac were from try builds, with windows non-aero styling checked via compatibility mode; linux build was a local build.)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [UXprio][needs review] → [UXprio]
Target Milestone: --- → Thunderbird 3.3a3
Attachment #512917 - Flags: review?(bugmail) → review+
(In reply to comment #57)
> And it's in!  Great work on this patch, Thomas!
> 
> patch pushed to trunk with the noted closeTab undo change (and an extra try to
> avoid unexpected failures in that vein) and minor style fixes:
> http://hg.mozilla.org/comm-central/rev/491fbcf7a1b4
> 
> styling fixes also pushed to trunk:
> http://hg.mozilla.org/comm-central/rev/33614283e675
> 
> I just sanity checked the styling and operation of the functionality on all 3
> platforms and all 4 themes.  (win and mac were from try builds, with windows
> non-aero styling checked via compatibility mode; linux build was a local
> build.)

thank you very much for your support to get my first bugfix to the trunk :-)

As this patch fixes Bugs 521260, 516247 and some how 504122 as side effect they could be closed. So I just added a comment to these bugs referring to this bug.
Blocks: 521260
Blocks: 504122
Flags: in-testsuite+
Depends on: 467927
Blocks: 663216
You need to log in before you can comment on or make changes to this bug.