Closed Bug 507392 Opened 12 years ago Closed 12 years ago

remove moveToAlertPosition() usage

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: mkmelin, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(1 file, 5 obsolete files)

Attached patch proposed fix (obsolete) — Splinter Review
Got tired of seeing
"Trying to position a sizeless window; caller should have called sizeToContent() or sizeTo(). See bug 75649." in the (native) console all the time.

We could fix it by calling sizeToContent() before very call to moveToAlertPosition(), but I don't see the point of the method call at all. 

If i remove it completely dialogs are properly centered relative to their opener. With the call, they are a bit over center on the screen it seems. Besides, the moveToAlertPosition method is in toolkit/obsolete/content/dialogOverlay.js since forever. I guess it was needed some time pre 2003?

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/obsolete/content/dialogOverlay.js&rev=HEAD&mark=1.1

Some affected dialogs:
 - new folder
 - rename folder
 - folder properties
 - address book card/list properties
 - ...
Attachment #391608 - Flags: ui-review?(clarkbw)
Attachment #391608 - Flags: superreview?(neil)
Attachment #391608 - Flags: review?(bugzilla)
Well assuming these are all dialogs, then a different version of moveToAlertPosition is used and called within the dialog set up code anyway:

http://hg.mozilla.org/releases/mozilla-1.9.1/diff/9b2a99adc05e/toolkit/content/widgets/dialog.xml

Which means you're doing some of what bug 113722 was asking for :-)
Blocks: 113722
Yep, all dialogs, except customheaders.js - but i've another cleanup patch coming that will make that a dialog too. So would seem this is indeed the comm-central part of what's left of bug 113722.
This seems to work on some dialogs but not others, viz:
abAddressBookNameDialog
renameFolderDialog
askViewZoom
abCardOverlay
I guess the ones that it does work on are opened including the modal and centerscreen styles in their window features.
Attachment #391608 - Flags: review?(bugzilla) → review-
Comment on attachment 391608 [details] [diff] [review]
proposed fix

Per Neil's comment, looks like this needs another revision - modal dialogs are treated differently on mac, so I can't test easily.
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Surprisingly, can't repro those problems on linux. Maybe it's platform dependent?

I've added centerscreen where needed, but i don't see anything to do for abAddressBookNameDialog nor for askViewZoom.

grep -r -C 5 --exclude=*~ --exclude=*.mn abAddressBookNameDialog.xul mail mailnews suite calendar

grep -r -C 5 --exclude=*~ --exclude=*.mn askViewZoom.xul mail mailnews suite calendar
Attachment #391608 - Attachment is obsolete: true
Attachment #394638 - Flags: superreview?(neil)
Attachment #394638 - Flags: review?(bugzilla)
Attachment #391608 - Flags: ui-review?(clarkbw)
Attachment #391608 - Flags: superreview?(neil)
(In reply to comment #5)
> I've added centerscreen where needed, but i don't see anything to do for
> abAddressBookNameDialog nor for askViewZoom.
I see the new patch does fix askViewZoom, but I can't see a change for rename folder or address book name which still open in the top left of my screen :-(

In fact, you look as if you fixed rename folder for Thunderbird only - for suite you added centerscreen to a dialog that already has centerscreen...
Attachment #394638 - Attachment is obsolete: true
Attachment #394638 - Flags: superreview?(neil)
Attachment #394638 - Flags: review?(bugzilla)
Attached patch proposed fix, v3 (obsolete) — Splinter Review
third time's a charm?

titlebar is on by default, and resizable isn't applicable to modal dialogs, so i removed those.
Attachment #394840 - Flags: superreview?(neil)
Attachment #394840 - Flags: review?(bugzilla)
I discovered that my testing methodology was suspect - some of the dialogs have multiple entry points and I hadn't tried them all. Latest results:

New contact - pass (but see below)
Contact properties - pass
New list - pass
List properties - pass
New address book - pass
Address book properties - FAIL
New LDAP directory - pass
LDAP directory properties - FAIL

Folder properties - pass
New (sub)folder - FAIL
Rename folder - pass

New filter - pass
Custom headers - pass

Send progress - did not test
Shutdown - did not test

Zoom - pass

Also, the second parameter to openDialog should always be blank, otherwise you get problems should you happen to open the same dialog from more than one parent window, which is a particular problem for the new contact dialog, because that can be opened from any of the main Suite windows (although the addressbook version must have a different caller because it isn't affected by the bug).
Attached patch proposed fix, v4 (obsolete) — Splinter Review
The failures should now be fixed.

I don't know about Send progress and Shutdown - they do work for me but that doesn't say much...
Attachment #394840 - Attachment is obsolete: true
Attachment #395084 - Flags: superreview?(neil)
Attachment #395084 - Flags: review?(bugzilla)
Attachment #394840 - Flags: superreview?(neil)
Attachment #394840 - Flags: review?(bugzilla)
Attachment #395084 - Flags: superreview?(neil) → superreview+
Comment on attachment 395084 [details] [diff] [review]
proposed fix, v4

Phew! ;-)
Attachment #395084 - Flags: review?(philipp)
Comment on attachment 395084 [details] [diff] [review]
proposed fix, v4

:) Thx a log neil!

Requesting r? from philipp for a few lines in calendar/
(In reply to comment #7)
> titlebar is on by default, and resizable isn't applicable to modal dialogs, so
> i removed those.

Except on mac where resizable and titlebar both seem to have the effect of making the modal dialog not resizable.
Comment on attachment 395084 [details] [diff] [review]
proposed fix, v4

r- due to the previously mentioned mac issue. I've not tested on Linux or Windows yet, but I'm guessing they probably fine.
Attachment #395084 - Flags: review?(bugzilla) → review-
(In reply to comment #12)
> (In reply to comment #7)
> > titlebar is on by default, and resizable isn't applicable to modal dialogs, so
> > i removed those.
> 
> Except on mac where resizable and titlebar both seem to have the effect of
> making the modal dialog not resizable.

Since I *removed* resizable=no:s i'm really surprised if that made them *not* resizable. Were they really resizable before? And if so, should they?

When i wrote resizable isn't applicable to modal dialogs i mean whatever you set it to there's no difference - at least on windows/linux.
Sorry I got that wrong. The modal dialogs are currently *not* resizable, with your patch they are resizable.

So resizable is applicable to modal dialogs on Mac which is what I was trying to say.
So, are there any rules/guidelines for which mac modal dialogs should be resizable and which shouldn't? What so applications usually do?

Re titlebar, did mac need/want that too?
(In reply to comment #16)
> So, are there any rules/guidelines for which mac modal dialogs should be
> resizable and which shouldn't? What so applications usually do?
> 
> Re titlebar, did mac need/want that too?

So from what I've seen so far, titlebar and resizable seem to do the same thing - they make the dialog not resizable. Mac never displays a title bar on a modal dialog afaik.
My vague memory (it's always hard to force modal dialogs in random programs when you want to see them) is that the only modal dialogs that are supposed to be resizeable are things like filepickers (or the New Filter dialog) that have listboxes in them.
Attached patch proposed fix, v5 (obsolete) — Splinter Review
Status quo regarding resizable=no then.
Carrying fwd sr=neil

Though if they aren't supposed to normally be resizable that sounds like a mac bug.
Attachment #395084 - Attachment is obsolete: true
Attachment #396074 - Flags: superreview+
Attachment #396074 - Flags: review?(bugzilla)
Attachment #395084 - Flags: review?(philipp)
Attachment #396074 - Flags: review?(philipp)
Assuming it's because I've still got the patch applied locally, Send Progress didn't work for me, it opened at the top left :-(

(In reply to comment #15)
> Sorry I got that wrong. The modal dialogs are currently *not* resizable, with
> your patch they are resizable.
Odd, since resizable=no is supposed to be the default...
Attached patch proposed fix, v6Splinter Review
Progress dialogs now working too.
Attachment #396074 - Attachment is obsolete: true
Attachment #396242 - Flags: superreview+
Attachment #396242 - Flags: review?(bugzilla)
Attachment #396074 - Flags: review?(philipp)
Attachment #396074 - Flags: review?(bugzilla)
Attachment #396242 - Flags: review?(philipp)
OK, so I've trawled through window watcher, app shell and widget, and what happens if you have a dialog but fail to specify (either positively or negatively) any of the chrome features (including titlebar or close, which default to on, and can therefore be safely used to disable this behaviour), then you get a widget-defined set of default flags. And in Windows widget that's no features, while in Mac widget that's all features. (I'm not sure about GTK2; it passes -1 which may not even be valid?)
neil: did you file a bug for it? I assume if you already tracked down where it's happening it might be too hard to fix.
Yes, please file a bug for making the chrome feature handling consistent.
Comment on attachment 396242 [details] [diff] [review]
proposed fix, v6

Sorry for the delay in getting back to this r+a=Standard8
Attachment #396242 - Flags: review?(bugzilla)
Attachment #396242 - Flags: review+
Attachment #396242 - Flags: approval-thunderbird3+
Comment on attachment 396242 [details] [diff] [review]
proposed fix, v6

Sorry for the delay, r=philipp
Attachment #396242 - Flags: review?(philipp) → review+
changeset:   4007:a7ee2565bfa6
http://hg.mozilla.org/comm-central/rev/a7ee2565bfa6

Thanks to all!
->FIXED 

I filed bug 520026  to make the feature handling consistent.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.0b4 → Thunderbird 3.0rc1
Version: unspecified → Trunk
Comment on attachment 396242 [details] [diff] [review]
proposed fix, v6

>+                      "chrome,modal,centerscreen,modal",
Oops! (Both times, I think.)
Yeah, one time. Code blindness :(
I'll check in a removal of the duplicate modal once the tree is happier.
changeset:   4017:2bc754381c2e
http://hg.mozilla.org/comm-central/rev/2bc754381c2e
You need to log in before you can comment on or make changes to this bug.