Closed Bug 643271 Opened 14 years ago Closed 14 years ago

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

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

(blocking-seamonkey2.1 -)

RESOLVED FIXED
seamonkey2.1b3
Tracking Status
blocking-seamonkey2.1 --- -

People

(Reporter: kairo, Assigned: InvisibleSmiley)

References

Details

Attachments

(2 files, 3 obsolete files)

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: --- → ?
Not blocking as no reason was given why it should.
blocking-seamonkey2.1: ? → -
Attached patch patch (obsolete) (deleted) — Splinter Review
(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 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.
Attached patch patch v2 (obsolete) (deleted) — Splinter Review
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)
(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?
(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"/>
Attached patch Possible patch (obsolete) (deleted) — Splinter Review
Attachment #523295 - Flags: feedback?(jh)
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)
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+
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+
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
Attachment #523126 - Attachment is obsolete: true
Attachment #523295 - Attachment is obsolete: true
Attachment #523126 - Flags: review?(neil)
Attachment #523326 - Flags: review?(neil)
Attachment #523326 - Flags: review?(neil) → review+
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]
Leaving open for part 2.
Attachment #523315 - Flags: review?(misak.bugzilla) → review+
Attachment #523315 - Attachment description: Fix tab notifications → Fix tab notifications [Checkin: comment 15]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: