Closed Bug 934191 Opened 7 years ago Closed 7 years ago

The title of the "Quit SeaMonkey" dialog is misleading when closing just the Browser

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
minor

Tracking

(seamonkey2.24 wontfix, seamonkey2.25 fixed)

VERIFIED FIXED
seamonkey2.25
Tracking Status
seamonkey2.24 --- wontfix
seamonkey2.25 --- fixed

People

(Reporter: tonymec, Assigned: tonymec)

Details

Attachments

(2 files, 2 obsolete files)

Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 SeaMonkey/2.24a1 ID:20130919003001 c-c:1ea1fc3586db m-c:803189f35921

The above describes the SeaMonkey version I'm using now, but AFAIK this problem has "always" existed; This is the "latest" (!) Linux64 trunk nightly at the moment.

When closing a multitab Browser window (and with the right preferences set) a dialog comes up, titled "Quit SeaMonkey" and with three buttons: "Quit", "Cancel", "Save and "Quit". It asks me if I'm sure I want to quit, and whether or not I want to save my tabs for restoring them "the next time it (SeaMonkey) starts".

This is OK if the browser close happens as a result of Ctrl+Q or equivalent, which does indeed quit SeaMonkey; but it is misleading if it is as a result of Ctrl+Shift+W or equivalent on the only (or last) browser window, if non-browser windows (e.g. MailNews and/or ChatZilla) are also up: in that case we are not "quitting SeaMonkey", only "closing the browser" (while leaving MailNews, ChatZilla, etc., running if they were). The title "Close Browser" also covers the "Quit SeaMonkey" case so it is a simple case of replacing one string by another.

I believe that the same text is used in Firefox (with the app name appropriately replaced by "Firefox", "Nightly" or whatever), but it is of less import there because Firefox has no mailnews component, and if ChatZilla is installed, there is no "Browser" icon at bottom left of its window which, if it were there, would enable to restart the Browser: so I'm reporting this bug for SeaMonkey because it hurts SeaMonkey (and especially new SeaMonkey users) the most, and because I don't expect the Firefox devs to be amenable to changing this string in Firefox or Toolkit or Core or wherever the code is, especially considering their present attitude of "We are not responsible for any application without 'Firefox' in its [commercial] name; if we make changes which break them, let them fix them as best they may, we won't help them". (That's what I think I understood lately from some newsgroup posts by Firefox module owners or peers; I hope it isn't as bad as this, but it's the impression I have at the moment).


Neil: I think this is the kind of bug that I could fix, but I'm not sure if the policy (when replacing one text by another in some string everywhere it happens) is to replace the text "in place" in the EN-US locale file, or contrariwise if it is required to invent a new string name (&entityname; or whatever) in this case. If the former, how do we warn translators that they might have to change the translation for this string?

I noticed this problem because, as long as there are no Linux nightlies being built, bug 916631 is not fixed for me, so I find myself using the workaround in bug 916631 comment #39 instead, which means restarting the browser while leaving MailNews open. I believe that the string change (from "Quit seaMonkey" to "Close Browser" or similar) wouldn"t harm, and that closing the Browser but not MailNews is something that one might occasionally want to do -albeit less often than I find myself doing it these days- even in a build with bug 916631 fixed.
The quit dialog function _onQuitRequest in nsSuiteGlue.js also handles restarts and (as you noticed) closing the last browser window if it has more than one tab open. It is passed null, "restart" or "lastwindow" as the aQuitType parameter appropriately. However it then ignores everything other than "restart", treating them all as "quit". Rather than changing all of the "quit" strings to refer to closing the browser, I would prefer for the "lastwindow" to be its own special case. This would then give you full freedom over the wording of the title, message and buttons, e.g.

lastwindowDialogTitle=Close Browser Window
closeTitle=Cl&ose
savetabsTitle=&Save and Close
messageLastWindow=Do you want %S to save your tabs for the next time you open the browser?

Note: these are just suggestions from off the top of my head; please don't ask me to review them.
ah, indeed, if the code creating the dialog is told that we aren't quitting but ignores it, then I agree that it's a bug, and that it ought to take note of it instead.
Assignee: nobody → antoine.mechelynck
Status: NEW → ASSIGNED
Attached patch patch v0 (obsolete) — Splinter Review
As you can see I'm a partisan of heavy commenting.

Neil: I didn't follow your suggestions to the letter. I looked for some other reviewer but there aren't many for Browser UI. Feel free to ask someone else for an additional review if, for instance, you think you're too involved to be objective.

What I'd most like to get an opinion about are the added comments in both files, and the wording. Of course, all other changes should be looked at too.
Attachment #826481 - Flags: review?(neil)
Oh, and the dialog will need to be wider to accomodate a longer button label. I hope its with is "auto" and not some fixed value.
Attached patch patch v0.1 (obsolete) — Splinter Review
if needs () around the whole condition. Comment #3 still applies.
Attachment #826481 - Attachment is obsolete: true
Attachment #826481 - Flags: review?(neil)
Attachment #826490 - Flags: review?(neil)
Comment on attachment 826481 [details] [diff] [review]
patch v0

>+    if (aQuitType != "restart") && (aQuitType != "lastwindow")
(aQuitType != "restart" && aQuitType != "lastwindow") would have worked.
(In reply to neil@parkwaycc.co.uk from comment #6)
> Comment on attachment 826481 [details] [diff] [review]
> patch v0
> 
> >+    if (aQuitType != "restart") && (aQuitType != "lastwindow")
> (aQuitType != "restart" && aQuitType != "lastwindow") would have worked.

Well, so they would. I'm quite green about JS and had to check the language specs about operator precedence to be sure of that. Do you want me to remove me to remove the inner parens on the same line in the patch v0.1 ?
(In reply to Tony Mechelynck [:tonymec] from comment #7)

> Well, so they would. I'm quite green about JS and had to check the language
> specs about operator precedence to be sure of that. Do you want me to remove
> me to remove the inner parens on the same line in the patch v0.1 ?
Yes please. Note this is the SeaMonkey style preference. With Firefox the preference is (as far as I can tell) "depends on the reviewer".
Don't bother attaching a new patch just yet, there may be other comments.
Comment on attachment 826490 [details] [diff] [review]
patch v0.1

>+    if ((aQuitType != "restart") && (aQuitType != "lastwindow"))
[Don't forget]

>+lastwindowTitle=C&lose browser
Needs a capital B, which should probably be the access key too. r=me with those fixed.
Attachment #826490 - Flags: review?(neil) → review+
Carrying forward r+ = Neil
Attachment #826490 - Attachment is obsolete: true
Attachment #829937 - Flags: review+
https://hg.mozilla.org/comm-central/rev/97eeebff7f37
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.25
Adding verifyme keyword because still no new builds on Linux. Anyone on whose platform there are 2.25a1 builds of SeaMonkey more recent than comment #12 may verify this bug by means of the following STR:

Steps to Reproduce:
1. Make sure that SeaMonkey (preferably en-US version) is running as both browser and mailer, with only one browser window, and two or more browser tabs.
2. Make sure that browser.tabs.warnOnClose and browser.warnOnQuit are set to true (which is not the default) in about:config.
3. Close the browser but not the mailer, e.g. by "File → Close Window" or Ctrl+Shift+W in the browser window.
Expected result:
-- The dialog which appears should display the new strings defined in quitDialog.properties: those whose names include "lastwindow" and whose text does not include "Quit", as shown with green background in the bottom-right part of the patch when viewed in "Diff" mode: https://bugzilla.mozilla.org/attachment.cgi?id=829937&action=diff
Keywords: verifyme
At last, Linux is being built again since 3 January (though not today, but, let's hope, tomorrow), so I could take this snapshot myself.

Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 SeaMonkey/2.26a1 ID:20140106003001 c-c:08b8cae1f27e m-c:14ac61461f2a

Ryan VanderMeulen, in his capacity as checkin engineer, set RESOLVED FIXED when he pushed my patch. I am the bug reporter and a member of the SeaMonkey QA team, but also the patch author: at times I'm losing count of my hats. So -- let's set VERIFIED (the present attachment comment has no widget for that) and if anyone following the steps in comment #13 in a SeaMonkey build later than comment #12 does not get the new strings, let him (or her, as the case may be) be sure to tell me, after making sure that no extension is interfering (I have one which would, so I took the snapshot in safe mode, and then I wrote this out of safe mode, so Nightly Tester Tools would put the Build ID and both changeset IDs after the User-Agent string above).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.