954302 - Contacts window forgets screen position when closed

Status: RESOLVED FIXED

(Instantbird Graveyard :: Contacts window, defect)

Description

[aleth [:aleth]]

*** Original post on bio 869 at 2011-06-29 14:02:00 UTC ***

When closing the Contacts window (but not quitting the application), its screen position is forgotten. (On reopening the Contacts window by clicking on the tray icon, the former position is not restored.)

Comment 1

[Benedikt Pfeifer]

*** Original post on bio 869 at 2011-07-01 08:16:56 UTC ***

This problem doesn't occur on Windows by the way (I know that the bug is filed for Linux).

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 869 at 2011-07-01 15:01:51 UTC ***

I think it's a known Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=542839

Comment 3

[aleth [:aleth]]

*** Original post on bio 869 at 2011-08-26 16:19:31 UTC ***

What's odd is that it doesn't *always* forget the position - it happens intermittently.

It doesn't care whether the window was closed using the close button or by single-clicking the tray icon.

Another (rarer) event that can happen is that the window is briefly restored (on single click on the tray) for a couple ms, but then immediately hides again. Another single click brings it back, but invariably with forgotten position.

Comment 4

[aleth [:aleth]]

*** Original post on bio 869 at 2011-08-26 16:21:04 UTC ***

Might be - but I can't remember encountering anything like it with Firefox. (Then again, restoring window positions happens much rarer in FF...)
 
(In reply to comment #2)
> I think it's a known Mozilla bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=542839

Comment 5

[aleth [:aleth]]

*** Original post on bio 869 at 2011-10-12 23:46:13 UTC ***

Is there a conceivable way to get around this mozilla bug? It's rather annoying.

Comment 6

[Florian Quèze [:florian]]

*** Original post on bio 869 at 2011-10-12 23:54:57 UTC ***

(In reply to comment #5)
> Is there a conceivable way to get around this mozilla bug? It's rather
> annoying.

Well, fixing the actual bug would be much better.
Maybe an add-on could call window.moveTo in an onload handled to move the window back to its saved position. I'm not sure that would work.

Comment 7

[Instantbot]

*** Original post on bio 869 by Xavior Penguin <xavior.penguin AT gmail.com> at 2011-11-30 02:11:02 UTC ***

Just for sake of verifying, I can confirm this is happening also in Linux.
Running xubuntu 11.10 with Instantbird 1.1 Stable

Comment 8

[Florian Quèze [:florian]]

*** Original post on bio 869 at 2011-11-30 09:51:52 UTC ***

(In reply to comment #7)
> Just for sake of verifying, I can confirm this is happening also in Linux.

This happens *only* on Linux ;).

Comment 9

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 869 as attmnt 1140 at 2012-01-26 12:03:00 UTC ***

This patch is an IB workaround for the mozilla bug and is therefore not the ideal solution. 

However:

- This is by far the most annoying UI bug on Linux.

- I can't see the longstanding mozilla bug being fixed anytime soon. I can't currently set up a suitable dev environment to attempt a patch, and even if there was a patch it would probably take a long time to get landed, since the relevant code (https://mxr.mozilla.org/mozilla-central/source/widget/gtk2/nsWindow.cpp) affects so many different platforms and situations. Moreover, the comment in that code "We only move a general managed toplevel window if someone has actually placed the window somewhere.  If no placement has taken place, we just let the window manager Do The Right Thing." suggests to me it is quite possible the current behaviour was desired and problems only arise in edge cases, which may be hard to identify and resolve consistently.

Comment 10

[Patrick Cloke [:clokep]]

*** Original post on bio 869 at 2012-01-26 12:58:34 UTC ***

Wouldn't you also want to store the size, or is that properly handled on Linux?

Why the extra focus call that is added?

Do we have a link to the Mozilla bug about this?

Comment 11

[aleth [:aleth]]

*** Original post on bio 869 at 2012-01-26 17:00:27 UTC ***

(In reply to comment #10)
> Wouldn't you also want to store the size, or is that properly handled on Linux?

It seems to be properly handled.
 
> Why the extra focus call that is added?

It is necessary when the contact window is minimized and then restored via click on the tray icon. 

Come to think of it, this part may also be required on other OS, I don't know.

> Do we have a link to the Mozilla bug about this?

In comment #2.

Comment 12

[aleth [:aleth]]

*** Original post on bio 869 at 2012-01-28 14:12:41 UTC ***

An old mozilla bug that indicates the current behaviour was intentional at least in 2001 (cf comment #12)
https://bugzilla.mozilla.org/show_bug.cgi?id=31086

Some related (inconclusive) discussions, copied across from IRC for future reference
https://bugs.launchpad.net/ubuntu/+source/metacity/+bug/124315
https://groups.google.com/group/mozilla.dev.extensions/browse_thread/thread/a52c37c007249338#

Comment 13

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 869 as attmnt 1141 at 2012-01-29 21:40:00 UTC ***

Changed ordering to fix this warning

Warning: reference to undefined property aEvent.button
Source File: chrome://instantbird/content/mintrayr.js
Line: 104

Comment 14

[aleth [:aleth]]

Comment on attachment 8352885 [details] [diff] [review]
Patch

*** Original change on bio 869 attmnt 1140 at 2012-01-29 21:40:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8352886 [details] [diff] [review]
Patch

*** Original change on bio 869 attmnt 1141 at 2012-01-31 03:58:05 UTC ***

>@@ -122,8 +146,15 @@
>       return;
>     }
>
>-    if (this._icon.isMinimized)
>+    if (this._icon.isMinimized) {
>       this._icon.restore();
>+#ifndef XP_MACOSX
>+#ifndef XP_WIN
>+      window.moveTo(this.storeX, this.storeY);
>+      window.focus();
>+#endif
>+#endif
>+    }
>     else
>       this._icon.minimize();
>   },
For the most part this patch looks great. I dislike this part and wonder if they should call this.restore() and this.minimize() instead. I'm not sure if that'll work properly or not, but I think it'd be more readable. :)

(Also _icon doesn't seem to be defined on the object, maybe it should be?)

I'd like to see it try calling restore/minimize directly, if that doesn't work then we'll see if we can make this a bit neater still.

Comment 16

[aleth [:aleth]]

*** Original post on bio 869 at 2012-01-31 10:24:04 UTC ***

(In reply to comment #14)
> (Also _icon doesn't seem to be defined on the object, maybe it should be?)

It does not cause any warnings, and there are various places in the code where its presence or absence is checked for ("if (!this._icon)"...), so I don't see any problem there.

Comment 17

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 869 as attmnt 1146 at 2012-01-31 12:25:00 UTC ***

This works for me and is cleaner. It also restores the window even when it has been minimized by the user without using the tray icon to do so. (Previously, this required two clicks on the tray icon, as the tray icon didn't realize the contact window wasn't visible.)

However, to take account of your review comments, it now necessarily changes the mintrayr code for OS other than Linux. (And for some of the original code, it is not clear to me why it is the way it is.)

Comment 18

[aleth [:aleth]]

Comment on attachment 8352886 [details] [diff] [review]
Patch

*** Original change on bio 869 attmnt 1141 at 2012-01-31 12:25:20 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[Patrick Cloke [:clokep]]

*** Original post on bio 869 at 2012-01-31 12:39:59 UTC ***

(In reply to comment #16)
> Created attachment 8352891 [details] [diff] [review] (bio-attmnt 1146) [details]
> Patch
This looks a lot nicer! I meant to ask you to change the ifdefs as flo suggested in a different bug (#ifdef XP_UNIX, #ifndef XP_MACOSX...I think). Let me try it in Windows first though and make sure there's no issues with the changes. Thanks for working on this!

Comment 20

[aleth [:aleth]]

*** Original post on bio 869 at 2012-01-31 12:46:51 UTC ***

As (I think?) I commented on the other bug, I think we should decide on a standard ifdef combination for the non-existent XP_LINUX and then use it consistently everywhere it applies - or define XP_LINUX.

Comment 21

[Florian Quèze [:florian]]

*** Original post on bio 869 at 2012-01-31 13:18:26 UTC ***

(In reply to comment #18)
> As (I think?) I commented on the other bug, I think we should decide on a
> standard ifdef combination for the non-existent XP_LINUX and then use it
> consistently everywhere it applies

#ifdef XP_UNIX
#ifndef XP_MACOSX

I think this is already used (mostly) consistently throughout both our and Mozilla code.

Comment 22

[Patrick Cloke [:clokep]]

Attachment: Patch with build bits and spacing fixes

*** Original post on bio 869 as attmnt 1161 at 2012-02-07 01:16:00 UTC ***

I updated aleth's patch to include the necessary build bits + fixed a couple of spacing nits & added a comment from aleth.

Comment 23

[Patrick Cloke [:clokep]]

Comment on attachment 8352891 [details] [diff] [review]
Patch

*** Original change on bio 869 attmnt 1146 at 2012-02-07 01:16:12 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 24

[aleth [:aleth]]

Comment on attachment 8352906 [details] [diff] [review]
Patch with build bits and spacing fixes

*** Original change on bio 869 attmnt 1161 at 2012-02-07 01:18:12 UTC ***

Thanks!

Comment 25

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 869 as attmnt 1182 at 2012-02-21 12:22:00 UTC ***

Moved the stored position to a persisted attribute, so it persists across restarts (thanks for the tip!).

We need to listen to the "deactivate" event to catch window move and close events, and "sizemodechange" does not fire on those.

TrayServiceImpl::CreateIcon (called from TrayServiceImpl::Minimize) is smart enough to check whether a tray icon already exists for the given window, so there is no problem with unifying the minimize calls in mintrayr.js. (Also, if there had been a problem, it would already have shown up in the existing code when minimizing to tray via the menu.)

Comment 26

[aleth [:aleth]]

Comment on attachment 8352906 [details] [diff] [review]
Patch with build bits and spacing fixes

*** Original change on bio 869 attmnt 1161 at 2012-02-21 12:22:44 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 27

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 869 as attmnt 1183 at 2012-02-21 13:47:00 UTC ***

Removed code duplication and improved comments.

No #ifdef is required in blist.xul as a persisted attribute is not actually written to localstore.rdf if it does not exist.

There is a possible small bug in the existing mintrayr code:  Open Preferences, put the buddy list behind the Preferences window (for example), then toggle "Always show tray icon". Then, the buddy list window is brought to the front as you remove the tick (it does not, however, gain focus). This is due to http://lxr.instantbird.org/instantbird/source/instantbird/components/mintrayr/content/mintrayr.js#86. It's an independent issue and should be tackled in a separate bug if necessary. However, I'm not sure it's actually a bug as the trayService must ensure the buddy list window is visible when the tray icon is removed.

Comment 28

[aleth [:aleth]]

Comment on attachment 8352932 [details] [diff] [review]
Patch

*** Original change on bio 869 attmnt 1182 at 2012-02-21 13:47:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 29

[Benedikt Pfeifer]

*** Original post on bio 869 at 2012-02-21 14:49:43 UTC ***

Maybe we can patch it out of Mozilla, so all OS have the same behaviour:
https://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#998

See discussion at: http://log.bezut.info/instantbird/120221/#m266

Comment 30

[Florian Quèze [:florian]]

Comment on attachment 8352933 [details] [diff] [review]
Patch

*** Original change on bio 869 attmnt 1183 at 2012-03-05 23:40:33 UTC ***

I dislike this code both before and after the patch, but I don't want to block this any longer. I'll keep clokep's review from comment 20; this way he'll be responsible if something breaks because of this :-P.

Comment 31

[Patrick Cloke [:clokep]]

*** Original post on bio 869 at 2012-03-06 00:40:23 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/f9a59d766cfd

Thanks for looking at this! Hopefully it works out. :)