The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.1b3

Status

SeaMonkey
Bookmarks & History
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Robert Kaiser, Assigned: InvisibleSmiley)

Tracking

Trunk
seamonkey2.1b3
Dependency tree / graph

Firefox Tracking Flags

(blocking-seamonkey2.1 -)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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?)
blocking-seamonkey2.1: --- → ?
(Reporter)

Comment 1

6 years ago
Not blocking as no reason was given why it should.
blocking-seamonkey2.1: ? → -
(Assignee)

Comment 2

6 years ago
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.
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #523059 - Flags: review?(neil)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #523059 - Attachment is obsolete: true
Attachment #523059 - Flags: review?(neil)
Attachment #523126 - Flags: review?(neil)

Comment 5

6 years ago
(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?
(Assignee)

Comment 6

6 years ago
(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

6 years ago
Created attachment 523295 [details] [diff] [review]
Possible patch
Attachment #523295 - Flags: feedback?(jh)

Comment 8

6 years ago
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.
Attachment #523315 - Flags: review?(misak.bugzilla)
Attachment #523315 - Flags: feedback?(jh)
(Assignee)

Comment 9

6 years ago
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).
Attachment #523315 - Flags: feedback?(jh) → feedback+
(Assignee)

Comment 10

6 years ago
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.
Attachment #523295 - Flags: feedback?(jh) → feedback+
(Assignee)

Comment 11

6 years ago
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
(Assignee)

Comment 12

6 years ago
Created attachment 523326 [details] [diff] [review]
merged patch (w/o tab notifications fix) [Checkin: comment 13]
Attachment #523126 - Attachment is obsolete: true
Attachment #523295 - Attachment is obsolete: true
Attachment #523126 - Flags: review?(neil)
Attachment #523326 - Flags: review?(neil)

Updated

6 years ago
Attachment #523326 - Flags: review?(neil) → review+
(Assignee)

Comment 13

6 years ago
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
Attachment #523326 - Attachment description: merged patch (w/o tab notifications fix) → merged patch (w/o tab notifications fix) [Checkin: comment 13]
(Assignee)

Comment 14

6 years ago
Leaving open for part 2.

Updated

6 years ago
Attachment #523315 - Flags: review?(misak.bugzilla) → review+
(Assignee)

Comment 15

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

http://hg.mozilla.org/comm-central/rev/80c1b0c04bd2
Attachment #523315 - Attachment description: Fix tab notifications → Fix tab notifications [Checkin: comment 15]
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
You need to log in before you can comment on or make changes to this bug.