Last Comment Bug 643271 - Late-coming post-checkin comments from Neil on bookmarks UI
: Late-coming post-checkin comments from Neil on bookmarks UI
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
Depends on: 580660 585601
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-20 06:54 PDT by Robert Kaiser
Modified: 2011-04-01 07:44 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-


Attachments
patch (3.67 KB, patch)
2011-03-30 11:10 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (3.90 KB, patch)
2011-03-30 14:49 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
Possible patch (1.41 KB, patch)
2011-03-31 07:17 PDT, neil@parkwaycc.co.uk
jh: feedback+
Details | Diff | Splinter Review
Fix tab notifications [Checkin: comment 15] (2.45 KB, patch)
2011-03-31 08:38 PDT, neil@parkwaycc.co.uk
misak.bugzilla: review+
jh: feedback+
Details | Diff | Splinter Review
merged patch (w/o tab notifications fix) [Checkin: comment 13] (4.03 KB, patch)
2011-03-31 09:03 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2011-03-20 06:54:05 PDT
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 2011-03-20 13:04:05 PDT
Not blocking as no reason was given why it should.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-03-30 11:10:22 PDT
Created attachment 523059 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-03-30 14:21:40 PDT
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) 2011-03-30 14:49:27 PDT
Created attachment 523126 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-03-30 16:38:57 PDT
(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) 2011-03-31 06:47:11 PDT
(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 neil@parkwaycc.co.uk 2011-03-31 07:17:51 PDT
Created attachment 523295 [details] [diff] [review]
Possible patch
Comment 8 neil@parkwaycc.co.uk 2011-03-31 08:38:05 PDT
Created attachment 523315 [details] [diff] [review]
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) 2011-03-31 08:45:31 PDT
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) 2011-03-31 08:47:08 PDT
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) 2011-03-31 08:50:14 PDT
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) 2011-03-31 09:03:45 PDT
Created attachment 523326 [details] [diff] [review]
merged patch (w/o tab notifications fix) [Checkin: comment 13]
Comment 13 Jens Hatlak (:InvisibleSmiley) 2011-03-31 16:20:17 PDT
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) 2011-03-31 16:20:52 PDT
Leaving open for part 2.
Comment 15 Jens Hatlak (:InvisibleSmiley) 2011-04-01 07:43:42 PDT
Comment on attachment 523315 [details] [diff] [review]
Fix tab notifications [Checkin: comment 15]

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

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