643271 - Late-coming post-checkin comments from Neil on bookmarks UI

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Robert Kaiser]

I have no intention of working on those, but if they sit in the closed bug, nobody will, so here's a new bug for them:

Bug 580660 comment #13 by Neil on attachment 463197 [details] [diff] [review]
v2.2: in-browser UI, updated for comments

>+  _updateCommandState: function BATH__updateCommandState(aTabClose) {
>+    var numTabs = gBrowser.browsers.length;
Nit: gBrowser.tabs.length

>+    // The TabClose event is fired before the tab is removed from the DOM
>+    if (aTabClose)
>+      numTabs--;
>+
>+    if (numTabs > 1)
>+      this._command.removeAttribute("disabled");
>+    else
>+      this._command.setAttribute("disabled", "true");
In theory you only need to check numTabs when closing a tab; after opening a
tab I would be very surprised if there was only one!

>     <!-- Bookmarks Menu -->
>+    <key id="addBookmarkKb" key="&addCurPageAsCmd.commandkey;" command="Browser:AddBookmark" modifiers="accel,alt"/>
Unfortunately Ctrl+Alt key combos are reserved on Windows. (Also, why was
Ctrl+Shift+D moved to bookmark all tabs?)

Comment 1

[Robert Kaiser]

Not blocking as no reason was given why it should.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

(In reply to comment #0)
> >     <!-- Bookmarks Menu -->
> >+    <key id="addBookmarkKb" key="&addCurPageAsCmd.commandkey;" command="Browser:AddBookmark" modifiers="accel,alt"/>
> Unfortunately Ctrl+Alt key combos are reserved on Windows. (Also, why was
> Ctrl+Shift+D moved to bookmark all tabs?)

I think is bad (I hate it when apps block reserved shortcuts!), so taking. I'll just remove the shortcut for Bookmark All Tabs, which had no shortcut before this change but stole the one from Bookmark This Page, as Neil noted. No l10n impact since addCurPageAsCmd.commandkey is used for both actions and I'll only remove one of the two users, so no need to block b3.

As KaiRo said, no need to block on this, but I think we should really fix this before final because otherwise people might get used to it and then it'll be hard to justify removing it. Currently it's easy, since it should not have happened in the first place.

Comment 3

[[e-mail]]

Comment on attachment 523059 [details] [diff] [review]
patch

>     // The TabClose event is fired before the tab is removed from the DOM
...
>+    numTabs--;
>     if (numTabs > 1)
Could just compare against 2 instead, with the comment possibly expanded?

>       this._command.removeAttribute("disabled");
Don't actually need to remove the attribute because it won't be set.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

So if we're rewriting half of the method, why not all of it?

Note: The == 1 case is needed for the init (new window with only one tab), and the parentheses are of course not really needed but I think it's cleaner.

Comment 5

[[e-mail]]

(In reply to comment #4)
> Note: The == 1 case is needed for the init (new window with only one tab)
Bah, init code strikes again. Can we just disable the command in XUL instead?

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #5)
> (In reply to comment #4)
> > Note: The == 1 case is needed for the init (new window with only one tab)
> Bah, init code strikes again. Can we just disable the command in XUL instead?

It already is:

    <!-- The command is disabled for the hidden window. Otherwise its enabled
         state is handled by gBookmarkAllTabsHandler. -->
    <command id="Browser:BookmarkAllTabs"
             label="&addCurTabsAsCmd.label;" accesskey="&addCurTabsAsCmd.accesskey;"
             oncommand="gBookmarkAllTabsHandler.doCommand();"
             disabled="true"/>

Comment 7

[[e-mail]]

Attachment: Possible patch

Comment 8

[[e-mail]]

Attachment: Fix tab notifications [Checkin: comment 15]

So, Jens pointed out that when we close the last-tab "in-place", the menuitem gets enabled with any of these patches. This is because we add a tab to replace the one that just closed, but the added tab dispatches a TabOpen event before we have really closed the tab, so there are now two tabs at this point, so we think we need to enable the "Bookmark this Group of Tabs" menuitem. The solution appears to be to dispatch the TabClose event after the TabOpen event.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 523315 [details] [diff] [review]
Fix tab notifications [Checkin: comment 15]

f=me for the result, which is all I checked (review needs to ensure this does not regress anything else).

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 523295 [details] [diff] [review]
Possible patch

f=me, seems to work as expected (together with "Fix tab notifications").
Note that this does not fully replace my patch; I'll need to merge both.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 523315 [details] [diff] [review]
Fix tab notifications [Checkin: comment 15]

FTR: "close the last-tab "in-place"" is closing the last tab with either:
a) tab middle click and middlemouse.contentLoadURL = false
b) close tab/window and browser.tabs.closeWindowWithLastTab = false
c) using the tab bar close button

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Attachment: merged patch (w/o tab notifications fix) [Checkin: comment 13]

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 523326 [details] [diff] [review]
merged patch (w/o tab notifications fix) [Checkin: comment 13]

http://hg.mozilla.org/comm-central/rev/a51304869f50

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Leaving open for part 2.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 523315 [details] [diff] [review]
Fix tab notifications [Checkin: comment 15]

http://hg.mozilla.org/comm-central/rev/80c1b0c04bd2