1612760 - Create warnAboutClosingTabs method for tabbrowser, use proper plural form for close tabs warnings and port |Bug 866880 - Implement Close Tabs to the Right| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, enhancement)

Description

[Ian Neal]

At the moment the close tabs warnings:

• are spread across 3 files which duplicates code
• don't make use of the proper plural form

This patch, as well as creating warnAboutClosingTabs method, also ports:

• Bug 350299 - window focus issue
• Bug 558461 - Don't warn about closing multiple tabs when closing only one tab
• Bug 947353 - Use proper plural form for tabs.closeWarningMultipleTabs
• Bug 866880 - Implement Close Tabs to the Right

The patch also adds some field names to tabmail so that common code can be used for both browser and mail tabs.

Comment 1

[Ian Neal]

Attachment: warnAboutClosingTabs patch

I've broadly kept the strings the same but should the title for closing multiple tabs be more than "Confirm close" e.g. "Confirm Closing Multiple Tabs" - this could make it less straight for localising.
Should the rest of Bug 866880 (Implement Close Tabs to the Right) be implemented i.e. do we want a context menu for "Close Tabs to the Right"? I don't think I've ever used it in Firefox, but some users might want it.

Comment 2

[Ian Neal]

Attachment: warnAboutClosingTabs patch v1.1

Now fully ports Bug 866880 - Implement Close Tabs to the Right, plus has option in File menu as well as the tab context menu.

Comment 3

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9124061 [details] [diff] [review]
warnAboutClosingTabs patch v1.1

Works great. A few NITs and NUTs

suite/browser/navigator.js

> +function BrowserCloseTabsToTheEnd()
> +{
> +  var browser = getBrowser();

Old functions are the same but use getBrowser() directly without a var in the new one?

suite/browser/tabbrowser.xml

> +            var shouldPrompt = Services.prefs.getBoolPref(pref);
> +            if (!shouldPrompt)

shouldPrompt is only used here. Use the Services.prefs. directly

> +              case this.closingTabsEnum.OTHER:

I thought there was a directive to make the linter happy but seems not to. Still add a /* fallthru */ comment?

> +            var ps = Services.prompt;

Use Services.prompt directly or if not move var ps down before:
> +            var buttonPressed =


> +            var tabsToEnd = [];
> +            let tabs = this.tabs;

let var mix intentional because of return value?

> +     for (var i = this.tabs.length - 1; i >= 0; --i) {

> +     for (let i = tabs.length - 1; i >= 0; --i) {

One added for loop is var i and one is let i. Think both can be let.

r/a+ with either fixed or answered.

Comment 4

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9124061 [details] [diff] [review]
warnAboutClosingTabs patch v1.1

Message to myself: Don't forget to set the values the next time.

Comment 5

[Frank-Rainer Grahl (:frg)]

Last one:Extend the parentheses tip from Bug 1612720 to this one.

suite/browser/tabbrowser.xml

            menuitem.disabled =

•               (tbattr.includes("tabbrowser-totheend") && isAtEnd) ||
                (tbattr.includes("tabbrowser-multiple") && !isMultiple) ||
                (tbattr.includes("tabbrowser-tab") && !isTab);
  

Comment 6

[Ian Neal]

Attachment: warnAboutClosingTabs patch v1.2

Changes as suggested apart from var browser = getBrowser(); one as browser is also used as an argument within the function call.
Carrying forward r/a+

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9f3138103f30
Create warnAboutClosingTabs method for tabbrowser, use proper plural form for close tabs warnings and port |Bug 866880 - Implement Close Tabs to the Right| to SeaMonkey. r=frg