Closed Bug 953622 Opened 10 years ago Closed 10 years ago

Conditional prompt for quitting Instantbird

Categories

(Instantbird Graveyard :: Other, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: romain)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 175 at 2009-04-22 22:57:00 UTC ***

Ask user if he/she really likes to quit if there are still unread messages in a conversation (for example messages that arrived just before clicking 'quit').
Attached patch Patch V1 (obsolete) — Splinter Review
*** Original post on bio 175 as attmnt 144 at 2009-07-28 17:25:00 UTC ***

This patch counts the number of unread conversation(s) when a close event is received.

Conversations on chats are considered (in this script) as unread only if someone requested your attention (blue tab).
And on normal conversation, it is when the conversation is unread (red tab).

I also improved the way Instantbird is closing with the X, to have the same behavior as File->Quit (it was necessary for the improvement to work).
Assignee: nobody → romain
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8351888 [details] [diff] [review]
Patch V1

*** Original change on bio 175 attmnt 144 at 2009-07-28 18:55:04 UTC ***

>diff --git a/instantbird/base/content/instantbird/blist.js b/instantbird/base/content/instantbird/blist.js

>+  _onQuitRequest: function (aCancelQuit, aQuitType) {
>+    // The request has already been canceled somewhere else
>+    if ((aCancelQuit instanceof Ci.nsISupportsPRBool) && aCancelQuit.data)
>+      return;
>+
>+    let prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);
>+
>+    let showPrompt = prefs.getBoolPref("messenger.warnOnQuit");
If the result is false here, you want to return without doing further computations.

>+    let wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                       .getService(Components.interfaces.nsIWindowMediator);
>+    let convWindow = wm.getMostRecentWindow("Messenger:convs");
This won't work as soon as we support having more than one conversation window.
I'm ok with taking it for now though.

>+    let unreadConvsCount = 0;
>+    if (convWindow) {
>+      let tabs = convWindow.document.getElementById("tabs");
>+      let panels = convWindow.document.getElementById("panels");
>+      if (tabs) {
>+        for (let i = 0; i < tabs.itemCount; ++i) {
>+          let tab = tabs.getItemAtIndex(i);
>+          let panel = panels.children[i];
>+          // For chats: attention, for simple conversations: unread
>+          if ((panel.hasAttribute("chat") && panel.getAttribute("chat") == "true") {
Do you really need both of these tests?

>+            if (tab.hasAttribute("attention"))
>+              unreadConvsCount++;
>+          }
>+          else if (tab.hasAttribute("unread"))
>+            unreadConvsCount++;

The tests here look more complicated than they actually are. What about something like:
 if ((panel.getAttribute("chat") == "true" && tab.hasAttribute("attention")) ||
     tab.hasAttribute("unread"))

Coding style nit: ++unreadConvsCount instead of unreadConvsCount++

>+    let bundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                           .getService(Components.interfaces.nsIStringBundleService)
>+                           .createBundle("chrome://instantbird/locale/quitDialog.properties");
That last line looks a bit long. If it helps to stay under 80 columns, you can indent like this:
  let bundle =
    blahblahblah;

>+    Components.utils.import("resource://gre/modules/PluralForm.jsm");
Are you sure you want to import this every time the function is executed?

>+    let checkbox = {value:false};
Coding style nit: {value: false}

>+    promptMessage = PluralForm.get(unreadConvsCount, promptMessage);
>+    promptMessage = promptMessage.replace("#1", unreadConvsCount);
    promptMessage = PluralForm.get(unreadConvsCount, promptMessage)
                              .replace("#1", unreadConvsCount);

>+    if (checkbox.value)
>+      prefs.setBoolPref("messenger.warnOnQuit", false);
I was bit surprised to see that the checkbox is completely ignored when clicking cancel, but we already discussed it and you convinced me it's the right behavior.

> this.addEventListener("load", buddyList.load, false);
>+this.addEventListener("close", buddyList.close, false);
Can't you add this listener inside the load method?

Looks good otherwise.
Attachment #8351888 - Flags: review-
Attached patch Patch V2 (obsolete) — Splinter Review
*** Original post on bio 175 as attmnt 145 at 2009-07-28 21:54:00 UTC ***

Did as Florian described ahead and:
- Multiple conversation windows are handled
- Cancel is now the default focus button.
- "Quit" instead of "Close" or "Exit"
Attachment #8351889 - Flags: review?(florian)
Comment on attachment 8351888 [details] [diff] [review]
Patch V1

*** Original change on bio 175 attmnt 144 at 2009-07-28 21:54:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351888 - Attachment is obsolete: true
Comment on attachment 8351889 [details] [diff] [review]
Patch V2

*** Original change on bio 175 attmnt 145 at 2009-07-28 22:44:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351889 - Flags: review?(florian) → review-
*** Original post on bio 175 at 2009-07-28 22:44:36 UTC ***

Comment on attachment 8351889 [details] [diff] [review] (bio-attmnt 145)
Patch V2

>diff --git a/instantbird/base/content/instantbird/blist.js b/instantbird/base/content/instantbird/blist.js

>+  _onQuitRequest: function (aCancelQuit, aQuitType) {
>+    // The request has already been canceled somewhere else
>+    if ((aCancelQuit instanceof Ci.nsISupportsPRBool) && aCancelQuit.data)
>+      return;
>+
>+    let prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);
>+
>+    let showPrompt = prefs.getBoolPref("messenger.warnOnQuit");
>+    if (!showPrompt)
>+      return;
You don't need that variable.

>+    let wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                       .getService(Components.interfaces.nsIWindowMediator);
>+    // We would like the windows to be sorted by Z-Order
>+    // See Bugs 156333 and 450576 on mozilla about getZOrderDOMWindowEnumerator
>+    let enumerator = wm.getEnumerator("Messenger:convs");
wm is not very useful either. Sometimes it's good to add variables even used only once because the name helps to understand, but 'wm' doesn't help much and the reader will rather look at nsIWindowMediator at the end of the line.

>+    while(enumerator.hasMoreElements()) {
Add a space between while and (

>+      if (tabs) {
>+        let localCount = 0;
>+        for (let i = 0; i < tabs.itemCount; ++i) {
>+          let tab = tabs.getItemAtIndex(i);
>+          let panel = panels.children[i];
>+          // For chats: attention, for simple conversations: unread
>+          if ((panel.getAttribute("chat") == "true" && tab.hasAttribute("attention"))
>+              || tab.hasAttribute("unread")) {
When wrapping a line like that, please put the operator at the end of the line, not at the beginning.

>+            ++localCount;
>+          }
>+        }
>+        if (localCount > 0) {
>+          unreadConvsCount += localCount;
>+          attachedWindow = convWindow;
>+        }
You don't need the localCount variable:
  attachedWindow = convWindows is cheap, you can do it each time you find a conversation with unread messages.

>+    let bundle =
>+      Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                .getService(Components.interfaces.nsIStringBundleService)
>+                .createBundle("chrome://instantbird/locale/quitDialog.properties");
>+    let prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                            .getService(Components.interfaces.nsIPromptService);
>+    if (!("PluralForm" in window))
>+      Components.utils.import("resource://gre/modules/PluralForm.jsm");
>+
>+    let checkbox = {value: false};
>+
>+    let promptTitle    = bundle.GetStringFromName("dialogTitle");
>+    let promptMessage  = bundle.GetStringFromName("message");
>+    let promptCheckbox = bundle.GetStringFromName("checkbox");
>+    let quitButton     = bundle.GetStringFromName("quitButton");
>+
>+    promptMessage = PluralForm.get(unreadConvsCount, promptMessage)
>+                              .replace("#1", unreadConvsCount);
>+
>+    let flags = prompts.BUTTON_TITLE_IS_STRING * prompts.BUTTON_POS_0 +
>+                prompts.BUTTON_TITLE_CANCEL * prompts.BUTTON_POS_1 +
>+                prompts.BUTTON_POS_1_DEFAULT;
>+
>+    if (prompts.confirmEx(attachedWindow, promptTitle, promptMessage, flags,
>+                          quitButton, null, null, promptCheckbox, checkbox)) {
>+      // Cancel
>+      aCancelQuit.QueryInterface(Ci.nsISupportsPRBool);
>+      aCancelQuit.data = true;
You can put that on a single line:
 aCancelQuit.QueryInterface(Ci.nsISupportsPRBool).data = true;
And I'm not sure the "// Cancel" comment helps at all. It doesn't give more information than the line it immediately precedes.

General code style comments:
 - Try to define and set variables directly before the code that uses them. (For instance, the plural form module import should be just before using PluralForm.get, and let prompts = ... should be just before using confirmEx). Also sometimes when declaring a bunch of variables for a single code block, it's more readable to define first the variables that will hold the result, this way you know what the block of code is attempting to do by reading the name of the variable in the first line (this specific comment targets unreadConvsCount and attachedWindow that should IMHO be declared before wm and enumerator that are short lived and are implementation details of the block of code).
 - Use empty lines only to separate blocks that have a different goal, not steps of a single 'action'.
An empty line is usually appropriate after a return, after a loop or if clause that contains multiple lines, etc...

All of the above is left to interpretation of course, these are not strict rules, but a general idea of how it is done in others parts of our code (and of most Mozilla code).
Attached patch Patch V3 (obsolete) — Splinter Review
*** Original post on bio 175 as attmnt 146 at 2009-07-28 23:21:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351890 - Flags: review?(florian)
Comment on attachment 8351889 [details] [diff] [review]
Patch V2

*** Original change on bio 175 attmnt 145 at 2009-07-28 23:21:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351889 - Attachment is obsolete: true
Attached patch Patch v4Splinter Review
*** Original post on bio 175 as attmnt 147 at 2009-07-28 23:34:00 UTC ***

Still an improvement ... removed a useless variable and coding style change for an empty line.
Comment on attachment 8351890 [details] [diff] [review]
Patch V3

*** Original change on bio 175 attmnt 146 at 2009-07-28 23:34:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351890 - Attachment is obsolete: true
Attachment #8351890 - Flags: review?(florian)
Comment on attachment 8351891 [details] [diff] [review]
Patch v4

*** Original change on bio 175 attmnt 147 at 2009-07-28 23:38:35 UTC ***

Looks like this is good to go :). Thanks!

(Note that I haven't tested this myself yet, but I know you tested this extensively at least on Linux).
Attachment #8351891 - Flags: review+
*** Original post on bio 175 at 2009-07-29 00:25:38 UTC ***

Fixed: https://hg.instantbird.org/instantbird/rev/903d3b707ea3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
OS: Other → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 0.2b1
*** Original post on bio 175 at 2009-08-06 01:47:27 UTC ***

Pushed a follow up to fix a little mistake in attachment 8351891 [details] [diff] [review] (bio-attmnt 147):
https://hg.instantbird.org/instantbird/rev/5e8a86802b49
*** Original post on bio 175 at 2009-08-20 11:37:41 UTC ***

So far it notifies you when you've _already missed_ a message.
Extending this idea:

-> Maybe ask as well if you're about to miss a message that is likely to come soon (i.e. when in one of your chat someone is sending an 'Is typing' messages in the moment you want to quit)
Depends on: 953668
Blocks: 953668
No longer depends on: 953668
*** Original post on bio 175 at 2009-10-27 15:11:54 UTC ***

Another follow up to remove the useless "pluralRule" string that was added by mistake in the string bundle and is confusing for translators: https://hg.instantbird.org/instantbird/rev/3c43e8ca58da
You need to log in before you can comment on or make changes to this bug.