Closed Bug 954451 Opened 7 years ago Closed 7 years ago

Quit warning is hidden when conversation window lacks focus

Categories

(Instantbird :: Other, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1016 at 2011-09-03 14:44:00 UTC ***

When the conversation window is minimised, the unread messages warning is not displayed on-screen when trying to quit or restart IB. It is there, of course (when you restore the conversation window it pops up as well) but ideally the dialog box would automatically restore the conversation window when it is triggered.

(Related issues have been discussed on IRC afaik, but I could not find a related bug)
OS: Linux → All
Summary: "Unread messages" warning is hidden when conversation window minimised → Quit warning is hidden when conversation window lacks focus
Target Milestone: --- → 1.2
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1016 as attmnt 1132 at 2012-01-24 22:36:00 UTC ***

Yet another papercut. 

See also http://lxr.instantbird.org/instantbird/source/instantbird/content/tabbrowser.xml#480
Attachment #8352877 - Flags: review?(florian)
Assignee: nobody → aleth
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8352877 [details] [diff] [review]
Patch

*** Original change on bio 1016 attmnt 1132 at 2012-01-24 22:55:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352877 - Flags: review?(florian) → review?(clokep)
Comment on attachment 8352877 [details] [diff] [review]
Patch

*** Original change on bio 1016 attmnt 1132 at 2012-01-24 23:20:16 UTC ***

Looks good! Thanks for taking care of this. :)
Attachment #8352877 - Flags: review?(clokep) → review+
Comment on attachment 8352877 [details] [diff] [review]
Patch

*** Original change on bio 1016 attmnt 1132 at 2012-01-25 10:32:35 UTC ***

I think convWindow can be null if there's no conversation window (it's possible to have unread messages in conversations on hold, but no conversation window).
If I'm right on this, your patch breaks the quit prompt in this case.

The prompts service knows how to handle a null parent window (http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/public/nsIPromptService.idl#208). I think in this case it just falls back to whatever's the front most window at the time.

I think a better fix would be:
let parentWin = Services.wm.getMostRecentWindow("Messenger:convs") ||;
  Services.wm.getMostRecentWindow("Messenger:blist");
if (parentWin)
  parentWin.focus();
if (prompts.confirmEx(parentWin, ...
Attachment #8352877 - Flags: review-
*** Original post on bio 1016 at 2012-01-25 10:33:38 UTC ***

(In reply to comment #3)

> let parentWin = Services.wm.getMostRecentWindow("Messenger:convs") ||;
                                                                       ^
Pretend this semi colon isn't here :)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1016 as attmnt 1135 at 2012-01-25 12:30:00 UTC ***

Oh, good catch! I just assumed it was non-null since it was passed as an argument.

I've tested it also works when all windows are closed and only the tray icon is present.
Attachment #8352880 - Flags: review?(florian)
Comment on attachment 8352877 [details] [diff] [review]
Patch

*** Original change on bio 1016 attmnt 1132 at 2012-01-25 12:30:40 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352877 - Attachment is obsolete: true
*** Original post on bio 1016 at 2012-01-25 12:58:15 UTC ***

Comment on attachment 8352880 [details] [diff] [review] (bio-attmnt 1135)
Patch

>--- ibCore.jsm	2012-01-24 23:31:55.000000000 +0100
>+++ ./ibCore.jsm	2012-01-25 13:26:01.000000000 +0100
>+    let parentWindow = Services.wm.getMostRecentWindow("Messenger:convs") ||
>+      Services.wm.getMostRecentWindow("Messenger:blist");

Nit: can we make these line up?
>+    let parentWindow = Services.wm.getMostRecentWindow("Messenger:convs") ||
>+                       Services.wm.getMostRecentWindow("Messenger:blist");
Attached patch PatchSplinter Review
*** Original post on bio 1016 as attmnt 1136 at 2012-01-25 19:06:00 UTC ***

Spaces added.
Attachment #8352881 - Flags: review?(clokep)
Comment on attachment 8352880 [details] [diff] [review]
Patch

*** Original change on bio 1016 attmnt 1135 at 2012-01-25 19:06:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352880 - Attachment is obsolete: true
Attachment #8352880 - Flags: review?(florian)
Comment on attachment 8352881 [details] [diff] [review]
Patch

*** Original change on bio 1016 attmnt 1136 at 2012-01-25 19:11:20 UTC ***

Looks good, hopefully flo doesn't find anything wrong this time. :)
Attachment #8352881 - Flags: review?(clokep) → review+
*** Original post on bio 1016 at 2012-01-25 19:15:37 UTC ***

(In reply to comment #3)
> The prompts service knows how to handle a null parent window
> (http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/public/nsIPromptService.idl#208).
> I think in this case it just falls back to whatever's the front most window at
> the time.

For future reference, this did not I believe work quite as intended, as IB would simply quit (instead of showing a prompt when the window was maximized again). But I can't think of any circumstances where it could cause trouble after this patch.
*** Original post on bio 1016 at 2012-01-26 00:42:32 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/2c7e775bec92

Thanks a lot!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.