Closed Bug 321490 Opened 19 years ago Closed 19 years ago

Make SeaMonkey compatible again with moveTabTo implementation of the Firefox

Categories

(SeaMonkey :: UI Design, defect)

Product:
SeaMonkey
Component:
UI Design
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: to.news2, Assigned: jag+mozilla)

References

(
URL
)

Details

(4 keywords)

Attachments

(1 file, 4 obsolete files)

Use mTabs.item() and get tab from insertBefore
1.58 KB, patch
csthomas
: review+
neil
: superreview+
csthomas
: approval-seamonkey1.0+
iannbugzilla
: approval-seamonkey1.1a+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.8) Gecko/20051219 SeaMonkey/1.0b
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.8) Gecko/20051219 SeaMonkey/1.0b

After install the release from seamonkey 1.0b the extension 'undoclosetab' no longer work!

The author from 'undoclosetab' says to this behavior:

"Latest SeaMonkey does have a <tabbrowser>.moveTabTo which was implemented in a way which makes it incompatible with the implementation of the Firefox one on which it was based. The Firefox moveTabTo was based on the moveTabTo by my miniT 
which was designed to be compatible with the one provided by TBE. This function has been API compatible since at least 2002-10-13, and many Extensions use this function if it exists (including undoclosetab), which now would cause breakages. If you want undoclosetab to work in further version, file a bug and ask them to change the name of the function or to be compatible with the Firefox implementation"

Please makes the differences between Firefox and Seamonkey(Mozilla) not bigger!
We have enough extension only for Firefox. Makes work for extension developer not harder...

Reproducible: Always

Steps to Reproduce:
1. open two or three tab
2. close a tab and click right mouse undoclose tab
3. a blank tab open


Actual Results:  
The system is confused now and i can no undo any tab.
Assignee: general → guifeatures
Component: General → XP Apps: GUI Features
QA Contact: general
Summary: Make Seamonkey compatible again with moveTabTo implementation of the Firefox → Make SeaMonkey compatible again with moveTabTo implementation of the Firefox
I believe we broke compat because their API was nonintuitive.
OK, so what's the best approach here? Rename moveTabTo and provide a stub function that calls through to the real function?
Version: unspecified → Trunk
Flags: blocking-seamonkey1.0?
Assignee: guifeatures → cst
Severity: major → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-seamonkey1.0? → blocking-seamonkey1.0+
Attached patch patch? (obsolete) — — Splinter Review 
I can't test this.
Attachment #208228 - Flags: review?(mconnor)
Love the JS loose typing. Take a look at the type of the first parameter and take it from there.
(In reply to comment #4)
> Love the JS loose typing. Take a look at the type of the first parameter and
> take it from there.
> 

I don't understand - did I mess something up?  The Firefox version takes a tab and an index.  Our version takes two indices.
Sorry for being cryptic.

I think you can use |instanceof| or |"localName" in| with the first argument to determine whether a number or a tab was passed in, allowing you to not need two separate method names.
(In reply to comment #6)
> Sorry for being cryptic.
> 
> I think you can use |instanceof| or |"localName" in| with the first argument to
> determine whether a number or a tab was passed in, allowing you to not need two
> separate method names.
> 

I thought about that.  This method has the advantage of keeping our method (and the wrapper) really simple; that method has the advantage of only having one function.  Whichever the reviewers prefer is fine with me.
      <method name="moveTabTo">
        <parameter name="aSrcIndex"/>
        <parameter name="aDestIndex"/>
        <body>
          <![CDATA[
            // for compatibility with extensions
            if (typeof(aSrcIndex) == "object")
              aSrcIndex = this.getTabIndex(aSrcIndex);

            this.mTabFilters.splice(aDestIndex, 0, this.mTabFilters.splice(aSrcIndex, 1)[0]);
            this.mTabListeners.splice(aDestIndex, 0, this.mTabListeners.splice(aSrcIndex, 1)[0]);

            this.mCurrentTab.selected = false;
            if (aDestIndex == this.mTabs.length - 1)
               this.mTabContainer.appendChild(this.mTabs[aSrcIndex]);
            else {
              var beforeIndex = aDestIndex < aSrcIndex                                    ? aDestIndex : aDestIndex + 1;
              this.mTabContainer.insertBefore(this.mTabs[aSrcIndex],
                                              this.mTabs[beforeIndex]);
            }
            this.mCurrentTab.selected = true;

            return this.mTabs[aDestIndex];
          ]]>
        </body>
      </method>

Not that big a deal I'd say. I added and moved down the |beforeIndex| so we can easily return the moved tab, though something like this would've worked too:

            // for compatibility with extensions
            var tab;
            if (typeof(aSrcIndex) == "object") {
              tab = aSrcIndex;
              aSrcIndex = this.getTabIndex(tab);
            } else
              tab = this.mTabs[aSrcIndex];

            /* old moveTabTo body */

            return tab;

Hmmm, wrapping is a bit odd there, but you get the idea.
I don't like one-line else (except with one-line then, of course). If you go that route, reverse the test so that the single line in the then portion.
Comment on attachment 208228 [details] [diff] [review]
patch?

mconnor said he doesn't like this over IRC, and jag said he'd attach his version.
Attachment #208228 - Attachment is obsolete: true
Attachment #208228 - Flags: review?(mconnor)
Attachment #208228 - Flags: review?(jag)
Comment on attachment 208228 [details] [diff] [review]
patch?

>+            for (var i=0; i<this.mTabs.length; i++) {
>+              if (this.mTabs[i] == aTab)
>+                break;
Didn't jag write a getTabIndex function?

>+            aIndex = (aIndex < i) ? aIndex : aIndex + 1;
if (aIndex >= i)
  aIndex ++;

>+      <method name="moveTabTo2">
Silly name. Use moveTabToIndex?
This also puts the |beforeIndex| into a separate var so we have the aDestIndex to return the moved tab from.
Attachment #208715 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #208715 - Flags: review?
Attachment #208715 - Flags: review? → review?(cst)
Comment on attachment 208715 [details] [diff] [review]
Check if the type of the first parameter is "number", assume it's a <tab> otherwise.

I don't like the beforeIndex scheme, although I can't decide between a) always returning the first argument b) setting up tab and tab index variables c) using the fact that this.mTabContainer.insertBefore(foo, this.mTabs.item(aDestIndex)) works when aDestIndex == this.mTabs.length plus it returns the moved tab.

Fixing the 3-space indent is good though ;-)
Attachment #208715 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

        Attachment #209046 -
        Flags: superreview?(neil.parkwaycc.co.uk)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Get tab from aSrcIndex and use mTabs[] || null (obsolete)
        — 
          — Splinter Review
      

    


  
Also, aDestIndex += ... vs. if (...) ++aDestIndex in the previous patch.

I suspect the final patch will be a little bit from every patch.

        Attachment #209048 -
        Flags: superreview?(neil.parkwaycc.co.uk)

    
    

    
  
Comment on attachment 209048 [details] [diff] [review]
Get tab from aSrcIndex and use mTabs[] || null

>+            aDestIndex += aDestIndex < aSrcIndex ? 0 : 1;
Heh, why not aDestIndex += aDestIndex >= aSrcIndex; ;-)

>+            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex] || null);
JS strict warning when aDestIndex == this.mTabs.length

        Attachment #209048 -
        Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

    
    

    
  
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

>+            if (aDestIndex > aSrcIndex)
Hmm, I should have reviewed these in order... needs to be >=

        Attachment #209046 -
        Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

    
    

    
  
It shouldn't matter whether we insert a tab before itself or after itself (before its nextSibling), it'll end up in the same place, namely its own spot.

    
    

    
  
So does foo.insertBefore(bar, bar); work? It still looks wrong...

    
    

    
  
It works:

data:text/html,<div id="foo"><a href="#" id="bar" onclick="var foo = document.getElementById('foo'); var bar = document.getElementById('bar'); try { foo.insertBefore(bar, bar); } catch (e) { alert(e); }">bar</a></div>

Also, for `this.mTabs[aDestIndex] || null' the `|| null' part is unnecessary. I thought insertBefore didn't like |undefined| as its second parameter, but it seems to accept it just fine now.

If you wish I can use >=, just let me know whether you want tab returned from insertBefore or from mTabs[aSrcIndex].

    
    

    
  
(In reply to comment #21)
>If you wish I can use >=, just let me know whether you want tab returned from
>insertBefore or from mTabs[aSrcIndex].
mTabs[aSrcIndex]? I don't think I've seen that version ;-) The indecision is:
a) always return arguments[0]
b) figure out the source tab and index from arguments[0], return the tab
c) convert the source tab to an index, then back to a tab via insertBefore

    
    

    
  
(In reply to comment #22)
> mTabs[aSrcIndex]? I don't think I've seen that version ;-)

Oops :-) aDestIndex, of course.

Ok, I see now. And since all of these need to figure out the source index when a source tab is passed in (for removing the filter and the listener), and vice versa (for |insertBefore()|), the indecision seems to be:

a) always return arguments[0]
b) return the source tab from (converted) arguments[0]
c) return the tab via insertBefore()

(a) vs. (b,c) is the more interesting question since it affects the API. For (b) vs. (c) I'd go with (c) since insertBefore already returns the tab anyway and the code is a bit shorter.

I think I'd prefer to always return the moved tab. It's merely a convenience when a source tab is passed in, but actually provides something new (and potentially useful, relatively cheaply) when a source index is passed in.

So, I'd go with (c).

    
    

    
  
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

OK, sr=me with >= as noted.

        Attachment #209046 -
        Flags: superreview- → superreview+

    
    

    
  
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

Not sure if you need my review again, but here it is ;)

        Attachment #209046 -
        Flags: review+

        Attachment #209046 -
        Flags: approval-seamonkey1.1?

        Attachment #209046 -
        Flags: approval-seamonkey1.0?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Option (b) with >= (obsolete)
        — 
          — Splinter Review
      

    


  
I was ready to check in when I realized that by grabbing |tab| from insertBefore we lose any additional properties set on the tab passed in. So, for full compatibility I'm back to saving and returning the passed in tab.

This is my final answer!

I hope.
Assignee: cst → jag

        Attachment #208715 -
        Attachment is obsolete: true

        Attachment #209046 -
        Attachment is obsolete: true

        Attachment #209048 -
        Attachment is obsolete: true
Status: NEW → ASSIGNED

        Attachment #209313 -
        Flags: superreview?(neil.parkwaycc.co.uk)

        Attachment #209313 -
        Flags: review?

        Attachment #209046 -
        Flags: approval-seamonkey1.1?

        Attachment #209046 -
        Flags: approval-seamonkey1.0?

    
  

        Attachment #209313 -
        Flags: review? → review?(cst)

    
    

    
  
Comment on attachment 209313 [details] [diff] [review]
Option (b) with >=

+            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex]);

Shouldn't this be |, this.mTabs.item(aDestIndex);|?

        Attachment #209313 -
        Flags: review?(cst) → review+

    
    

    
  
(In reply to comment #27)
> (From update of attachment 209313 [details] [diff] [review] [edit])
> +            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex]);
> 
> Shouldn't this be |, this.mTabs.item(aDestIndex);|?
> 

On second thought, it doesn't matter in this version, does it?

    
    

    
  
Sorry, I should've made that more clear. A quick test seemed to show (to my surprise) that .item(i) was needed 'coz it returns null, where [i] returns undefined. When I ran some more tests on Friday it turned out insertBefore(tab, undefined) works as well. See also comment 21.

    
    

    
  
Hmm, a quick test suggests that insertBefore() leaves the properties intact, which actually makes sense, you'd want the native object to always be represented by the same js object.

    
    

    
  
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

Ok, I've convinced myself that this is safe to use.

        Attachment #209046 -
        Attachment is obsolete: false

        Attachment #209046 -
        Flags: approval-seamonkey1.1?

        Attachment #209046 -
        Flags: approval-seamonkey1.0?

    
  

        Attachment #209313 -
        Attachment is obsolete: true

        Attachment #209313 -
        Flags: superreview?(neil.parkwaycc.co.uk)

    
    

    
  
Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.139; previous revision: 1.138
done
Assignee: jag → guifeatures
Status: ASSIGNED → NEW

    
  
Status: NEW → ASSIGNED

    
  
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
  
Status: REOPENED → ASSIGNED

    
  
Assignee: guifeatures → jag
Status: ASSIGNED → NEW

    
  
Status: NEW → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 209046 [details] [diff] [review]
Use mTabs.item() and get tab from insertBefore

a=me for SM1.1
a=me for SM1.0, one more needed

        Attachment #209046 -
        Flags: approval-seamonkey1.1? → approval-seamonkey1.1+

        Attachment #209046 -
        Flags: approval-seamonkey1.0? → approval-seamonkey1.0+

    
    

    
  
(In reply to comment #27)
>(From update of attachment 209313 [details] [diff] [review])
>>+            this.mTabContainer.insertBefore(tab, this.mTabs[aDestIndex]);
>Shouldn't this be |, this.mTabs.item(aDestIndex);|?
Yes it should, see comment 17, please fix.

    
    

    
  
Bah, strict warnings. Ok, I'll fix that.

    
    

    
  
[] -> .item() checked in.

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.140; previous revision: 1.139
done

Also checked in on 1_8 and 1_8_0:

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.126.2.11; previous revision: 1.126.2.10
done

Checking in tabbrowser.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.126.2.4.2.7; previous revision: 1.126.2.4.2.6
done

Keywords: fixed1.8.0.1, 
          
            fixed1.8.1
Whiteboard: fixed-seamonkey1.0

    
    

    
  
thank you for fixes this bug :-)
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.1) Gecko/20060125 SeaMonkey/1.0

a little behaivor disagreement, i have still found.

In mozilla version before seamonkey release the tab i undo, return on selve position if i close. In seamonkey it won't work, here is always the first tab.

On page http://www.extensionsmirror.nl/index.php?showtopic=226
i read this "Tabs will be focused and moved to previous position if gBrowser.moveTabTo is available (included in Firefox 1.0+ and most miniT variants)." Is this gBrowser.moveTabTo not implemented?

I also install the tablib-for-seamonkey.xpi for SeaMonkey from http://mozilla.dorando.at/readme.html without success.

    
    

    
  
(In reply to comment #37)
> I also install the tablib-for-seamonkey.xpi for SeaMonkey from
> http://mozilla.dorando.at/readme.html without success.
> 
tablib-for-seamonkey.xpi    19-Feb-2004 08:29     2k  

If it's really from 2004, it is not likely to work - we've made large changes since then.


    
    

    
  
(In reply to comment #38)
> tablib-for-seamonkey.xpi    19-Feb-2004 08:29     2k  
> 
> If it's really from 2004, it is not likely to work - we've made large changes
> since then.

o.k. but why work the extension undoclosetab not with function open on previous position, with SeaMonkey 1.0 at same firefox?

    
    

    
  
i have just now see, Netscape 8.1 actually have the function undoclosetab, see "When a tab is closed, focus to:" picture here http://scr3.golem.de/?d=0601/Netscape_8_1&a=42955&s=15

bye
Torsten
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
(In reply to comment #39)
> (In reply to comment #38)
> > tablib-for-seamonkey.xpi    19-Feb-2004 08:29     2k  
> > 
> > If it's really from 2004, it is not likely to work - we've made large changes
> > since then.
> 
> o.k. but why work the extension undoclosetab not with function open on previous
> position, with SeaMonkey 1.0 at same firefox?
> 

Could you join our IRC chat in irc.mozilla.org #seamonkey and talk to one of our German-speaking developers?  If this bug is not properly fixed, we need to know immediately, but I can't understand your comments.

    
    

    
  
oh, is my english so bad?

is it possible, build in the function tabs moved to previous position with 'browser.moveTabTo' after close and reopen?

    
    

    
  
Marking closed again.

I looked at the code for the undoclosetab extension, and the problem isn't with our implementation of |moveTabTo()|, it's with the extension itself:

eval("gBrowser.removeTab ="+gBrowser.removeTab.toString().replace(
'var oldTab = aTab;',
'this.closedTabs.push([aTab._tPos, oldBrowser.sessionHistory]); \  if(this.closedTabs.length > this.mPrefs.getIntPref("undoclosetab.cache")) this.closedTabs.shift(); \
 undoclosetab.cmd.removeAttribute("disabled"); \
 var oldTab = aTab;'));

This code expects aTab to have a _tPos property, which we don't have. This causes all the problems you see.

Could you contact the extension developer and ask them to do something like this before they remove the tab:

var tabPos = -1; // or maybe gBrowser.browsers.length?
if("_tPos" in aTab)
  tabPos = aTab._tPos;
else if ("getTabIndex" in gBrowser)
  tabPos = gBrowser.getTabIndex(aTab);

getTabIndex() was added when moveTabTo() was added, and you only need the index when you actually want to move the tab back to its old spot, so this code should work for both Firefox and SeaMonkey.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED

    
    

    
  
s/closed/fixed/ ;-)
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0

    
    

    
  
(In reply to comment #43)
> I looked at the code for the undoclosetab extension, and the problem isn't with
> our implementation of |moveTabTo()|, it's with the extension itself:

thank you very much for your fact finding, i contact the extension author!

    
  
Component: XP Apps: GUI Features → UI Design

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: