(Just a reminder:) Maximize a browser window (or almost any window in the product) and close it. The next (browser) window opened will be in the correct size and location of the pre-maximized window, but it will not be maximized. Better 'twere maximized. Note we won't be able to do this on gtk, where we can't even tell that we're maximized. But Windows and the Mac are able to track that and restore in a normal or maximized state.

Mass moving M17 bugs to M18

mass-moving all bugs to m21 that are not dofood+, or nsbeta2+

This is a real annoyance on Windows. Also (and perhaps more importantly?), if I maximize a window and then open second mozilla window, it's not maximized, so I'm constantly having to hit the maximize button in the titlebar. Very annoying. Moving to M21 from future as per comments from Peter Trudelle. I think this one needs to be fixed to release a polished product.

no, this one is now future. We will have to leave this much polish till a future release.

Peter Trudelle, is this so hard to implement? Many people are suffering from this bug... I think it must be in release!

I agree, please take my frown face seriously when I say that this is one of the bugs that I most want fixed. :(

I'm suffering too, but we simply do not have time for this level of bugfix. We could possibly still take a good patch though. Adding helpwanted keyword. :`(

Added dependency on bug 30394 because before having persistent inheritage, we must have non-persistent inheritage, which is available in JavaScript via window.open() parameter (Navigator 3 and 4 behavior/compatibility).

*** Bug 41543 has been marked as a duplicate of this bug. ***

*** Bug 41543 has been marked as a duplicate of this bug. ***

Sorry 'bout the spam... having much trouble with Bugzilla. Personally, I don't think Mozilla should open up in the minimized state, in which case the last restored state should be persisted. i.e.- maximized or last window position and size.

Would this just be a dupe of bug 20847, rather than depending on it?

I would add my vote for this, but I don't have any more left ... :(

Nominating for RTM. This is driving me nuts being a laptop user.

cc self

*** Bug 42588 has been marked as a duplicate of this bug. ***

*** Bug 42588 has been marked as a duplicate of this bug. ***

rtm-

Adding myself hidday@geocities.com to CC. It seems there are alot of small bugs related to the minimizing/maximizing/window restoring on Win32.

adding to CC: mdaskalo@tlogica.com This is something very annoing.

We love you, Dan! :-) Gerv

Is this technology available only on Windows? Other nits: + if (sizeString.EqualsWithConversion("m")) + mSizeMode = nsSizeMode_Minimized; + if (sizeString.EqualsWithConversion("M")) + mSizeMode = nsSizeMode_Maximized; Could do "else if" here. Also, why not "minimized" and "maximized"? (In which case, I'd recommend using NS_LITERAL_STRING's...)

else-if and NS_LITERAL_STRING. Check. I used "m" and "M" to match those values that are already being persisted elsewhere. I could change the persistence code, too, if you like.

This bug was triaged rtm- on 10/4. It remains rtm-.

I don't like the values "m" and "M". I think we should use "minimized" and "maximized" as the values, even if it involves changing more code. This has no hope of being rtm+, so we may as well do it right.

Trust Peter to push process for process' sake. Restoring the helpwanted flag. External Mozilla developers, you know what to do.

No, this is minimal process for the sake of shipping product. As far as I could tell, the triage on this bug was changed for unknown reasons, perhaps even accidentally. We need to keep such triage visible and rational, and the decision has to include the assigned engineer, jrgm and myself, plus ekrock if he has expressed interest. If these people think this bug is severe enough, and the patch safe enough, to pass PDT criteria for N6, then I'm willing to let that happen. Even then, I think the concerned development and QA engineers will need to present their case directly to PDT, and the result is still almost certain to be 'rtm-'.

At the risk of upsetting everyone i'm resetting the severity. hyatt/waterson: Instead of maximized and minimized which are really nice in an English speaking world [and potentially painful to anyone who has w/ English], ... Could we use X and N [or n]? maXimized, miNimized. brendan: helpwanted, this patch sounds good and appears to have a good architecture [otherwise why would people complain about the state variable values], but we don't even have it on the trunk :-( ... Could you help work out this last kink? danm: thank you very much for the patch, i'm sorry that it wasn't perfect enough for some ... let's see if we can get this into the trunk. Blake: important patches that have minimal work left, sound interesting?

I'm requesting review of my patch. If I get it checked in I will change this to [rtm+] since i have just stolen the bug from danm per danm@netscape.com 2000-10-18 09:58. I'm setting a milestone too. As this is my bug, a few ground rules: (a) if you have questions contact me on irc (b) don't apply netscape management rules to my bugs (b1) pdt [rtm++] still applies (b2) pdt should not consider this bug until I say it is ready (b3) don't mess with the [rtm*] in the status while it is my bug (c) please don't spam my bug(s), that's what newsgroups and irc are for. Please excuse my rules, but other related bugs got into big fights over this nonsense, so i'm putting these here to prevent that. wrt #define I think that we could easily change that after this bug was checked into trunk, IMO testing and usability is more important on trunk than a bit of style in a header file. I talked to people on irc and #define seemed vaguely acceptable I think a const nsLiteralString might be prefered but i'm not sure. scc: comments?

Repeating waterson's review, better pay attention: + + if(NS_SUCCEEDED(docShellElement->GetAttribute(NS_ConvertASCIItoUCS2("sizemode"), sizeString))) Don't use NS_ConvertASCIItoUCS2, it wastes malloc space and cycles on platforms that have two-byte L"foo" characters. Use NS_LITERAL_STRING. + { + mSizeMode = nsSizeMode_Normal; + if (sizeString.EqualsWithConversion(wsmMinimized)) Use Equals, not EqualsWithConvertion, as there is no conversion required now that you are using NS_LITERAL_STRING (inside the wsmMinimized macro). Name your constants using ALL_CAPS, not interCaps: WSM_MINIMIZED, etc. + mSizeMode = nsSizeMode_Minimized; + if (sizeString.EqualsWithConversion(wsmMaximized)) As waterson pointed out, there should be an else before this if. + mSizeMode = nsSizeMode_Maximized; + // defer telling the widget until we're visible + } Same comments apply here: - sizeString.AssignWithConversion("n"); + sizeString.AssignWithConversion(wsmNormal); if (sizemode == nsSizeMode_Minimized) - sizeString.AssignWithConversion("m"); + sizeString.AssignWithConversion(wsmMinimized); else if (sizemode == nsSizeMode_Maximized) - sizeString.AssignWithConversion("M"); + sizeString.AssignWithConversion(wsmMaximized); About the following, in addition to renaming the macros to use Java/JS/Mozilla house style (ALL_CAPS), I agree with hyatt: why are we using the cybercrud school of abbreviation? Why not "minimized", "normal", and "maximized"? +#define wsmMaximized NS_LITERAL_STRING("X"); +#define wsmNormal NS_LITERAL_STRING("n"); +#define wsmMinimized NS_LITERAL_STRING("m"); /be

danm, i'm starting to really sympathize brendan: thanks, BUT danm's patch was 'in the style of' the file he was patching. I'll attach my new patch, but here's my list of things that i didn't patch (I request that you read the entire file and give comments here on any other things to which you object, I'll fix them too if given the proper guidance) many instances of this: PR_snprintf(sizeBuf, sizeof(sizeBuf), "%ld", (long)x); sizeString.AssignWithConversion(sizeBuf); NS_ENSURE_SUCCESS(domdoc->GetElementById(NS_ConvertASCIItoUCS2(aID), aDOMElement), NS_ERROR_FAILURE); static const char *prefix = "@mozilla.org/appshell/component/browser/window;1"; nsAutoString topic; topic.AssignWithConversion(prefix); topic.AppendWithConversion(";"); nsCOMPtr<nsIJSContextStack> stack(do_GetService ("@mozilla.org/js/xpc/ContextStack;1")); nsCOMPtr<nsIScreenManager> screenmgr = do_GetService ("@mozilla.org/gfx/screenmanager;1", &result); prefres = prefs->CopyCharPref("browser.chromeURL", &urlStr); urlStr = "chrome://navigator/content/navigator.xul"; nsCOMPtr<nsIJSContextStack> stack(do_GetService ("@mozilla.org/js/xpc/ContextStack;1")); NS_ENSURE_SUCCESS(service->Notify(removeme, topic.GetUnicode(), aData), NS_ERROR_FAILURE); scc: if you find me on irc and have time to tutor me, i'd appreciate it. Patch3 incoming.

danm, asside from #define waterson thinks i've fulfilled his requests please review (r=). If you have a prefered style for the constants I'll use it, I was thinking that some sort of constant static window class element would be more correct but I don't know the style for that.

timeless: I don't care whose patch was in what style, I comment on bad or just old practice and style, and the presence of it elsewhere in the file is no excuse for doing the wrong thing when adding or modifying. See rule/tip #13 in http://www.mozilla.org/hacking/reviewers.html#rules-and-tips, especially the link to new string tech. It's a good idea to look at the rest of the file, too -- I was lazy, and didn't want to turn over a rock in case this bug had any chance of getting [rtm+]'d. Right now I'm still short on time to review the whole thing, but some quick comments on your comment: PR_snprintf(sizeBuf, sizeof(sizeBuf), "%ld", (long)x); sizeString.AssignWithConversion(sizeBuf); There is indeed a conversion from ASCII to pseudo-UCS2, but scc has a better way to do this now: nsPrintfCString. I hope it can be used here without trouble; I will let scc take over on this one. NS_ENSURE_SUCCESS(domdoc->GetElementById(NS_ConvertASCIItoUCS2(aID), aDOMElement), NS_ERROR_FAILURE); Looks like a bone fide conversion, not a string literal. Or did you want me to barf on the NS_ENSURE_SUCCESS with its hidden return statement? static const char *prefix = "@mozilla.org/appshell/component/browser/window;1"; nsAutoString topic; topic.AssignWithConversion(prefix); topic.AppendWithConversion(";"); I think we need another scc jihad through the tree to avoid gratuitous conversions from string literals that may be expressible in 2-byte Unichars as literals. nsCOMPtr<nsIJSContextStack> stack(do_GetService ("@mozilla.org/js/xpc/ContextStack;1")); nsCOMPtr<nsIScreenManager> screenmgr = do_GetService ("@mozilla.org/gfx/screenmanager;1", &result); prefres = prefs->CopyCharPref("browser.chromeURL", &urlStr); urlStr = "chrome://navigator/content/navigator.xul"; nsCOMPtr<nsIJSContextStack> stack(do_GetService ("@mozilla.org/js/xpc/ContextStack;1")); NS_ENSURE_SUCCESS(service->Notify(removeme, topic.GetUnicode(), aData), NS_ERROR_FAILURE); Not sure what to say about these. Some look just fine. /be

I can't believe travis polluted every file he touched with his fibonacci number indentation and bracing. Latest patch looks good to me, apart from that mess. /be

I want "minimized", "normal", and "maximized" as the values. This is consistent with other attribute values in XUL (we use full English words for all XUL attribute values and not abbreviations).

The logic of timeless' patch looks oddly familiar, and fine by me. Reviewers have been particular about the details, though. So as long as we're still ripping on you, I'll express a small desire to see the constant's you're defining moved out of the header file into the source. By my way of thinking, they're more an internal implementation issue; not really part of the interface. I'll also echo Brendan's indentation comment: as long as you're touching nearly every line of LoadPositionAndSizeFromXUL, please do restore it to its pre-Travis, Mozilla-standard 2 space indentation, and your choice of any brace style in common usage. My reading of the comments is that the latest patch will meet everyone's expectations if timeless uses nsPrintfCString, reformats the much-touched function to meet Mozilla standards, considers moving the #defines to the source file, and heeds David about the full-on English attribute values. (Yes, that makes them less international, but there's no getting around XUL files' highly English bent.) As far as I'm concerned, there's no need to prove your intentions by posting yet another patch. r=danm. Hey, hang in there. You're almost done.

Ok, I made one coding error s/(#define.*);/$1/ to undo After this fix the code compiles and apparently works for Jerry_Baker so all i need is scc's explanation of string handling Hyatt: your bone was fixed before you repeated your comment. I'm sorry you didn't check my latest avail patch http://bugzilla.mozilla.org/showattachment.cgi?attach_id=17530.

+ default : + mode = SW_RESTORE; + } Please put a break; after the default: case, even though it is last (compiles with half a brain will eliminate the branch-to-next-instruction). Recall the tale from the '80s (comp.risks archives) where a giant AT&T central office switch crashed because of a missing break after default (and then someone added a case after the default). What is going on here with control flow? + if (aIID.Equals(NS_GET_IID(nsIPrompt))) { + // XXX until nsIWebShellWindow goes away: + nsCOMPtr<nsIWebShellWindow> webShellWin = + do_QueryInterface(NS_STATIC_CAST(nsIXULWindow*, this), &rv); + if (NS_FAILED(rv)) return rv; + return webShellWin->GetPrompter((nsIPrompt**)aSink); + } + if (aIID.Equals(NS_GET_IID(nsIWebBrowserChrome)) && + NS_SUCCEEDED(EnsureContentTreeOwner()) && + NS_SUCCEEDED(mContentTreeOwner->QueryInterface(aIID, aSink))) + return NS_OK; + else No braces around multi-line if/then, and else after return is a non-sequitur, but worse: if aIID is nsIWebBrowserChrome, but any of the && clauses after that test fail, you fall into code that tries to compare aIID to other IIDs. I think you (we, really -- you didn't write this, but we're revising it ;-) want: + if (aIID.Equals(NS_GET_IID(nsIWebBrowserChrome))) { + rv = EnsureContentTreeOwner(); + if (NS_FAILED(rv)) return rv; + return mContentTreeOwner->QueryInterface(aIID, aSink); + } Now recall that we left off with an else after return non-sequitur: + else + return QueryInterface(aIID, aSink); - NS_IF_ADDREF(((nsISupports*)*aSink)); - return NS_OK; + NS_IF_ADDREF(((nsISupports*)*aSink)); + return NS_OK; The final code, the NS_IF_ADDREF and return NS_OK, is not reachable! Either do just the following in place of the above: + return QueryInterface(aIID, aSink); or tell me what I'm missing here. +NS_IMETHODIMP nsXULWindow::GetDocShell(nsIDocShell** aDocShell) { + NS_ENSURE_ARG_POINTER(aDocShell); Could we have the opening brace of all method bodies on a newline, by itself, and unindented? This special case for an opening brace (which you, I, and much of this code typically puts on the same line as the if, for, while, switch, or do keyword introducing the block) allows readers and grep-like automation (vim [[ in particular) to find the opening brace of a function body fast. Same for: +NS_IMETHODIMP nsXULWindow::GetZlevel(PRUint32 *outLevel) { + *outLevel = mZlevel; + return NS_OK; } Looks like you changed more methods after this one, too. Oops, you did the same thing to GetInterface's opening brace, but I didn't notice above. Really, the special case where a method or function opening brace goes on its own line should be preserved in this file, I think. More in a bit, have to get my car. /be

Timeless, thanks again for taking on this cleanup. Back to the code: + nsContentShellInfo* shellInfo = (nsContentShellInfo*)mContentShells.ElementAt(i); NS_STATIC_CAST(nsContentShellInfo*, mContentShells.ElementAt(i)) instead of old C-style cast here. + if (shellInfo->primary) { - nsContentShellInfo* shellInfo = (nsContentShellInfo*)mContentShells.ElementAt(i); - if(shellInfo->primary) - { - *aDocShellTreeItem = shellInfo->child; - NS_ADDREF(*aDocShellTreeItem); - return NS_OK; - } + *aDocShellTreeItem = shellInfo->child; + NS_ADDREF(*aDocShellTreeItem); + return NS_OK; } + } Bracing looks funky (travis-style, indented to same level as the block's code, and each brace on its own line). Next method: + PRInt32 count = mContentShells.Count(); + for(PRInt32 i = 0; i < count; i++) { + nsContentShellInfo* shellInfo = (nsContentShellInfo*)mContentShells.ElementAt(i); + if (shellInfo->id.Equals(aID)) { + *aDocShellTreeItem = shellInfo->child; + NS_ADDREF(*aDocShellTreeItem); + return NS_OK; } + } Closing braces don't line up with opening control keywords. Same prob later, in ShowModal. At this point, how about fixing what I've commented on, and then attaching diff -u and diff -wu patches (the latter will help see past much of the tab/indentation fixing). Thanks, /be

Brendan, not that anyone cares about the mn6 branch, but do you think i'd actually be able to check in my complete rewrite of these files on that branch? I think i'd like to be able to check in a basic version of this patch (once i get instructions on using that function from scc, or maybe w/o that) to the mn6 branch..

Umm ... Just to clarify, this patch *just* persists maximized state (as in the original report), not minimized state, right? Persisting minimized state would be very confusing for the user.

adding Self to CC. Sorry for the spam.

why nobody's checking this in? at least in the trunk! would like to vote, but don't have votes any more.

timeless: there's a way-too-long line after: @@ -409,48 +407,13 @@ // nsWindow constructor // //------------------------------------------------------------------------- -nsWindow::nsWindow() : nsBaseWidget() The member initializers should be broken up into 80-char line runs, or even better, one member per line. This hanging indented arguments example and the one after it need to underhang the first argument (indented to the column after the opening parenthesis): @@ -1151,8 +1106,7 @@ nsWidgetInitData *aInitData) { return(StandardWindowCreate(aParent, aRect, aHandleEventFunction, - aContext, aAppShell, aToolkit, aInitData, - nsnull)); + aContext, aAppShell, aToolkit, aInitData, nsnull)); } I think the - lines are closer to ideal, and just need to be indented a bit more to the right. Another example, from the diff -u patch: + if (NULL != deferrer) { + VERIFY(((nsWindow *)par)->mDeferredPositioner = ::DeferWindowPos(deferrer, + mWnd, NULL, aX, aY, 0, 0, + SWP_NOZORDER | SWP_NOACTIVATE | SWP_NOSIZE)); + } else { + VERIFY(::SetWindowPos(mWnd, NULL, aX, aY, 0, 0, + SWP_NOZORDER | SWP_NOACTIVATE | SWP_NOSIZE)); } The assignment expression in the "then" part has a long left-hand side, so it seems best to break after the = operator: + if (NULL != deferrer) { + VERIFY(((nsWindow *)par)->mDeferredPositioner = + ::DeferWindowPos(deferrer, mWnd, NULL, aX, aY, 0, 0, + SWP_NOZORDER | SWP_NOACTIVATE | SWP_NOSIZE)); + } else { + VERIFY(::SetWindowPos(mWnd, NULL, aX, aY, 0, 0, + SWP_NOZORDER | SWP_NOACTIVATE | SWP_NOSIZE)); } and indent the right-hand side one more unit (two spaces). I also fixed the hanging indent in the "else" clause's multiline actual argument list. Total nit below, but I think most people do not put a space before the colon that ends a case label (or a goto label): + case nsSizeMode_Maximized : + mode = SW_MAXIMIZE; + break; Certainly, other switches in this file do not have such spaces. Here's a case where the long argument expression should be on the second line of arguments, underhanging the first arg, and not split across the line break: @@ -2121,8 +2098,8 @@ NS_IMETHODIMP nsWindow::ScrollWidgets(PRInt32 aDx, PRInt32 aDy) { // Scroll the entire contents of the window + change the offset of any child windows - ::ScrollWindowEx(mWnd, aDx, aDy, NULL, NULL, NULL, - NULL, SW_INVALIDATE | SW_SCROLLCHILDREN); + ::ScrollWindowEx(mWnd, aDx, aDy, NULL, NULL, NULL, NULL, SW_INVALIDATE | + SW_SCROLLCHILDREN); Just put the SW_INVALIDATE | SW_SCROLLCHILDREN on the second line. Thanks for undertaking this cleanup, it's big and daunting. I suggest more divide and conquer (one method at at time, or only affected methods) next time. Eventually, the whole file will be much more readable, but you'll have an easier time getting reviews. I'll review a new patch and get blizzard to sr= it. Please assuage mpt's concern about minimized state persistence. /be

As a Mozilla Advocate, I have to say that one of the most annoying things about using mozilla for the past year is this bug. PLEASE, if at all possible get an RTM++ for this so it is fixed in Netscape 6! Please!

A few people broke the rules set forth in this bug by me in Comments From timeless@mac.com 2000-10-18 17:20 As this is my bug, a few ground rules: (c) please don't spam my bug(s), that's what newsgroups and irc are for. Thank you for wasting my time, and upsetting me. You were removed. If you quietely add yourself to the cc list I will not remove you. However, I would suggest you use silent voting instead, because in the future the CC list might be used by the developer (I am the developer for this bug as of that date). Attached is a zip containing a cvs tip diff in -u and -wu format. Brendan said he would review. scc: please check the diff -wu for any conventions that you would like fixed. someone: please test this patch. everyone else: I will have a more complete set of bug commenting guidelines which you should read before commenting on any bugs, but especially mine. mpt: If you can explain to me how someone could send netscape a newwindow signal that would cause this code to load a window minimized then you may explain it in a future bug. This patch has already become way too hard to manage which is why it took so long to work on. I suspect that any such path is a bug in something else, and therefore I will not fix it now, nor will I fix it in response to this bug which clearly says to preserve both states. i'm adding a rule (d). (d) don't change the scope or purpose of my bugs. This means that after this fix is committed (if it ever gets committed), anyone who finds a flaw in it may feel free to assign a new bug to me as caused by this bug (so this bug would block your new bug). This is standard procedure, but I feel the need to spell it out as other people have broken similarly obvious guidelines.

This patch should not be allowed to land if it persists the minimized state. That would be a far worse regression than the defect it is intended to fix.

*** Bug 60057 has been marked as a duplicate of this bug. ***

why shouldn't it persist minimized state? Ok, a few novices in windows/mac use won't see that there's mozilla in the task menu, but this would be a much less problem than the current one and the bug summary says it _should_ persist the current state. And the only case it is possible to persist minimized state is startup because you can't open a new window out of a minimized window or am I wrong?

Windows shouldn't persist minimized state because: (1) If my current Mozilla window is minimized and I choose `File' > `New' > `Navigator Window', or press Ctrl+N, if the new window is minimized I'll be quite annoyed. (4.x behavior is, correctly, to open a non-minimized window in this case.) (2) If Mozilla windows are minimized when grandma shuts down Windows, and then when she next runs Mozilla it persists that state and opens its windows minimized, she'll be hopelessly confused.

> If my current Mozilla window is minimized and I choose `File' > `New' > `Navigator Window', or press Ctrl+N, How is this possible if the window is minimised? I see your point about 2), though. Gerv

2. is outside the scope of the bug because i don't expect this persistance to happen beyond the current session. If i am wrong i am sure danm or I can fix it so that when moz starts it ignores minimization.

Changing summary; it would be acceptable to keep track of whether the last state was minimized, and some niche-market utilities may actually want to persist that for some strange reason, but this bug was originally filed for persisting only the maximized/normal states, and that is what the default behavior should be for the current suite of apps. Doing otherwise would hopelessly confuse 99.44% of all users (including mozillians), not just the mythical 'grandma'.

Enjoy.

It looks to me like this bug was originally filed with the summary "window maximized/minimized state not persisted". timeless (the bug's assignee) put in quite a bit of effort pushing the process forward and making this happen. In his 2000-11-13 15:42 comments the assignee specifically requested that other's "don't change the scope or purpose of my bugs." My read of the process is that the bug's summary belongs initially to the reporter, (possibly briefly to QA and Development as the bug is triaged) and comes to rest sqarely with the developer the bug is assigned to. Changing the summary and scope of a bug without the assignee's permission (especially after patches are beign reviewed) is inappropriate and dangerous. I fail to see where "this bug was originally filed for persisting only the maximized/normal states". The only summary change I can find in teh Bug History is: trudelle@netscape.com changed | short_desc window maximized/minimized state not persisted | window maximized state not persisted | 2000-12-04 17:32:03 I'm deeply concerned that a contributor to this project who had been working diligently within the review and approval process on a bug which is as high profile as this was shown so little respect. -Asa

per timeless's good reporting rules, just cc:ing myself!

I don't understand what the issue is here. Ensuring that a new patch does not cause another problem before checking it in doesn't seem like an unusual request to me. It doesn't seem like a good idea to rush to check this in and then worry about the problem it will cause later on. I understand that the original summary said to persist minimized state, but dan didn't even mention it in the description, and the consensus now seems to be that that would be harmful. Let's just fix the patch to keep track of, but not actually restore, the state if it's minimized...

There is certainly not a consensus about the harms of minimization. No one has given a valid reason for not persisting minimized state. The only way to find out if it is bad is to try it. If somoene builds the binaries (i'll try building this weekend, I just got MSVS last week and I have projects and papers to write first).

I have seen valid reasons. I would think the fact that no other app does it, along with the fact that it can be extremely confusing, would be reason enough. Yes, the patch needs to be tried. If there isn't a problem, we're arguing over nothing. If there is, it should be fixed.

The original text of the bug report mentions only maximized and normal state. I think it is inappropriate for a 'summary' to include something not in the full report, especially something that could cause a worse defect than the one reported. As the module owner, I think I have some ownership of this bug as well, but I'd be glad to abide by the accepted mozilla.org processes and ownership models for changing bug fields, that I have supposedly disregarded, if someone would kindly point out where they are defined. I certainly meant no disrespect to Timeless, and do appreciate his work on this patch while DanM was prevented from checkin it in. reassigning back to the DanM, who should still find the attachment 'oddly familiar', although nicely scrubbed.

*** Bug 62329 has been marked as a duplicate of this bug. ***

<sulk> mumblegrumbleMacgrumblemumble. <sigh> OK, so the ideal solution is not to persist minimised. However, I still think we should check this patch in and then tweak it later :-) Gerv

Is this bug related to the fact that you can get zero size windows by clicking on an url in your favourite mail program while Mozilla is minimized? (Well, they aren't completely zero size. Windows won't let them go any smaller than what's required to display the window's title bar).

I hope not, since this patch was never checked in. German/mpt. interesting point. I don't think this is enough to prevent persistance of state, i think it means that the specific chrome based entry points should tell the new window mechanism not to persist minimized. Cases so far: [don't consider minimized] Chrome commandline/dde [do consider minimized] webjavascript Are there any other cases?

It's a joy to have this bug back. Take fifteen. Here's a patch much like the original, simpler patch from 17 Oct except it won't minimize a window and it addresses Chris Waterson's objections to that patch. Oh, and I was mistaken in earlier claims where I said nothing need be done on the Mac. This one covers the Mac, too. Nothing happens on Linux because there's no specific zoom state, as far as I know. I made no attempt to un-Travis the indentation or braces. Forgive me; I even stuck with the Travis indentation where new code was surrounded by them. Don't really want to reformat the file right now. Lookin' for reviews once more.

Oh, and there's something wrong with RDF persistence of window state right now. That prevents this patch from working quite right. I'll get around to tracking that down someday, too. Sigh.

See bug 61471 for RDF persistence woes.

I didn't install the patch, but the Windows code looks like it ought to do the trick.

r=hyatt

mac part looks good, r=pink

sr=brendan@mozilla.org. I'd still like to get the main good effects of timeless's patch in. I'll help. Dan, you up for it? /be

yes. a different checkin for the cleanup would be cleaner. i'm leaving the bug open until we get that part fixed, too.

Nothing has changed in terms of window state persistence in my latest win32 tip build (or in a nightly I just downloaded). Is this because of the RDF persistence bug you mentioned, or...?

Man, it's sure working for me now. Your sidebar must be open to see it; bug 61471 is preventing flushing of window state on application close if the sidebar is closed. That said, my windows build this morning is showing an embarrassing problem with redrawing the window contents after maximizing. Sigh. I swear it wasn't there yesterday.

*** Bug 62984 has been marked as a duplicate of this bug. ***

I just downloaded build 2000121508 because this is a fix I've been waiting on for a while... I loaded it, maximized the window, enabled the sidebar, and exited. Then I loaded and disabled the sidebar. All new browser windows are opening maximized. Unfortunately, minimizing a maximized browser window and then restoring it causes it to lose its maximization. Was this regression caused by the fix to this bug?

Still doesn't work for me in win tip build or a new installer nightly I just downloaded. Deleted profile, etc. Opened sidebar. Do I suck or what?

Uh oh. I see what's happening. This works for me if I use the Windows restore/maximize buttons (upper right corner) to change the state, but not if I double click the titlebar, which is my usual method of changing window state. Looking at the patch, I don't understand why that would be...do we not get notification in that case? I'll split off a separate bug for this.

This works *sometimes* for me. It seems that the state of the last closed window is persisted, not the state of the current window. Try this: 1. Launch Mozilla. 2. Open a Nav window, maximize it. 3. Open a new window via Ctrl-N (or right click on a link and open it in a new window) 4. Make sure that the new window opened is not maximized. 5. Close that window. 6. Go back to your original maximized window. 7. Open a new window again (Ctrl-N). Expected result: New window is opened in a maximized state. Actual result: New window is opened in the restored state (the state in which the window closed in step 5 was). This is a 4.x parity concern. In N4, pressing Ctrl-N or otherwise opening a new window from a maximized window, resulted in the new window being maximized, no matter the state of the last window closed. In other words, although it is great progress that the maximized state of a last closed window is persisted, it is, however, necessary to perist the state of the *current* window, not the last window, during "new window" commands (i.e. Ctrl-N via keyboard or menu, right-click and select Open In New Window, clicking on a link with window="_top").

> Expected result: New window is opened in a maximized state. > Actual result: New window is opened in the restored state (the state in which > the window closed in step 5 was). This is arguable. > This is a 4.x parity concern. In N4, pressing Ctrl-N or otherwise opening a > new window from a maximized window, resulted in the new window being > maximized, no matter the state of the last window closed. No, on Linux, it behaves just as you describe the current "Actual result" of Mozilla. I guess, you are speaking of Windows. So, you cannot speak about 4.x parity.

I think that 4.x got this behavior very wrong, I always found it maddening that it would continue to open windows the size of the last window closed, even if I resized each new one. It persisted the window state only on closing, I believe due to programmer convenience. In order to get new windows to open as they want, users would have to carefully size one, then *close it*?!! That seems very counter-intuitive, and a huge waste of users' time. It should instead try to persist the most recent sizing for the same type of window. Perhaps this needs to be a separate bug? I think I've filed it before...

So, what should the code do, exactly? Persist the size of the last manually resized window?

I think what most people expect it to do is to copy the size/state of the current window. For example, if I'm in a maximized window, and I hit ctrl-n, I want another maximized window. However, if I'm in a small window, I'd like another small window when I open a new window.

That sounds reasonable if you're opening the same type of window as the current window. (i.e., Ctrl-N in mail should not open a browser window the size of the current mail window.)

That's not true, if the user didn't set the window size himself. If some ill-minded web application programmer opened a window with a certain size for me, and I say "new window", I want a normally sized window. Note that your suggestions is no persistantce at all technically, at least not during runtime. The new window just inherits the size of the old one.

I suggest if you have multiple windows of one kind of different sizes in the session you should not try to persist those sizes, and you should just leave the last saved value as it is. But I agree that you should open a window at the same size if it's the same kind, or all other windows of that kind are of one size.

Looks like I just clobbered danm's dep change. Are dependency changes not considered in midair collisions? Restoring.

What about only persisting size for windows that the user personally resized. Ie if a javascript popup occurs, it is not counted as a user resize.

But when would the state get saved and when will it be used? Suppose I have one maximized window, then open up another window, resize it, and go back to my original, maximized window and hit ctrl-n. If this does anything else than give me a new maximized window, I'll be highly annoyed.

From the patch, in windows/nsWindow.cpp: + switch (aMode) { + case nsSizeMode_Maximized : + mode = SW_MAXIMIZE; + break; + case nsSizeMode_Minimized : + mode = SW_MINIMIZE; + break; + default : + mode = SW_RESTORE; Should that last one be SW_NORMAL instead? Probably makes no difference in practice, but just a thought.

Dean: SW_SHOWNORMAL, right? I believe the distinction boils down to whether the window is already visible. And all three cases similarly have dual constants. They could arguably all three go something like case nsSizeMode_Maximized : mode = mIsVisible ? SW_MAXIMIZE : SW_SHOWMAXIMIZED; However in practice I haven't noticed that any of this makes any difference. Still, see the patch in bug 63268, which addresses this while it fixes another related problem. All: recent discussion in this bug seems to have turned to the question of which window has its size and state persisted. At the moment, it's the last window that changed, which isn't quite right. Making it right should be as simple as persisting the state info when a window is activated. This should be a one- line change, but the unpleasant nsWebShellWindow/nsXULWindow dichotomy makes this a little more difficult. I can get around to fixing this soon but not immediately. If someone beats me with a patch, that'd be OK, too.

Dan, it actually makes no difference. SW_NORMAL == SW_SHOWNORMAL == 1 SW_MAXIMIZE == SW_SHOWMAXIMIZED == 3 It never occurred to me about mIsVisible. I think that using ShowWindow will always show a hidden window, unless you specify SW_HIDE. Maybe if mIsVisible == PR_FALSE then don't call ShowWindow, but when mIsVisible changes, call ShowWindow with the new mode.

Hrm, is it just me or does this only work for browser windows, and only when you open them manually (as opposed to obnoxious web sites which use "target" to open windows)?

Not sure what you mean by obnoxious websites' windows. We explicitly do not persist size info for spam windows, to keep them from changing your browser settings. Saving special settings for spam windows is a thought -- what, minimize one and have them collect in the taskbar from then on, instead of opening? But that would be a Request For Enhancement. As for only affecting the browser window, yes, that's true. The feature is enabled, and can now easily be turned on for your favourite window. It should probably be turned on for all the Task windows.

I was referring to sites that use target=_blank (or similar) to open new windows when you click on a link. They don't get maximized, which makes a bad site worse :) 4.x maximizes them, but I don't recall what MSIE does.

A new window explicitly created from an existing window (i.e. by pressing Ctrl+N, or choosing `Open Link in New Window') should inherit all the properties of the existing window, including maximized/restored state (but not minimized state). (Note that `explicitly created' does not include windows which open by themselves, e.g. JS popups or target="_new". Internet Explorer always opens these in a non-maximized window.) A new window created from nowhere (e.g. by double-clicking the Navigator icon, or by pressing Ctrl+N when no windows are open) should inherit all the properties of the window where the state was last manually changed by the user.

"Note that `explicitly created' does not include windows which open by themselves, e.g. JS popups or target="_new". Internet Explorer always opens these in a non-maximized window." IE always opens target="_blank" or "_new" windows unmaximized, as it also does to windows opened through "Open Link in New Window". I always thought these were among IE's most annoying "features". All new windows should assume the user wants them to be the same size as the last window, except those explicitly declared to be different through Javascript.

For what reason would anyone ever want new windows to open maximized, yet not want windows produced by target=_blank to open maximized? I'm not clear on what circumstances would make this behavior appropriate.

I love this bug. Glad to see there are so many different opinions. Here's what you're going to get from me before I close it "fixed." It follows the opinions of some. I mean besides just me. Ah, the warm glow. New windows will be opened to the specifications of the most recently active window of the same type (browser, mail, ...). Except for the Linux build, which (Pavlov tells me) doesn't provide any decent, cross-window-manager way of informing the app that a window has been maximized or un-, and prefers to allow the window manager to position the new window. Linux will inherit window size, only. That's it. It's arguably not quite right. It's arguably totally right. People are doing both. Those on my side can share that warm glow I'm working on.

> All new windows should assume the user wants them to be the same size as the > last window, except those explicitly declared to be different through > Javascript. I don't know what you mean with "explicitly", but we must not assume the user wants something, unless he explicitly (i.e. manually, through the Mozilla chrome) triggered it himself. Again: If my bank account access app opened a window of size 400x300, why would I want to have subsequent windows open in the same size?? > If you For what reason would anyone ever want new windows to open maximized, > yet not want windows produced by target=_blank to open maximized? Because I newbie might not notice at all, that a new window opened. > I love this bug. I can very well imagine that. :-( > New windows will be opened to the specifications of the most recently active > window of the same type > Except for the Linux build, which [...] will inherit window size, only. Windows don't inherit the screen *position* (x,y coordinates of the top leflt corner), do they? This would be a serious usability bug, because the user might not notice at all, that a new window opened. (I know, *I* was sometimes confused, even.) But IIRC, this problem preexists your fix. Is there another bug about it?

To relief danm's nerves and follow german's reasonable suggestion: Please post replies to .ui (I posted my comment there <news://news.mozilla.org/3A441714.30609@bucksch.org>).

Should not this bug include not only browser windows, but also Mail, Composer and Address Book windows? I notice that in the latest builds, the browser window remembers its maximized state, but the Mail window doesn't. :-(

*** Bug 63652 has been marked as a duplicate of this bug. ***

*** Bug 63652 has been marked as a duplicate of this bug. ***

(spam) Adding myself since I'm interested in seeing when this will be done for MailNews

I just checked in a patch to make min/max state persistent on mail/news and editor windows. Here's a patch to ensure new windows are opened to the state of the frontmost window of their type. (It wasn't as complicated as I had feared). On Dasher, On Comet, On Spam Machine! Yah! Index: nsWebShellWindow.cpp =================================================================== RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp,v retrieving revision 1.323 diff -u -r1.323 nsWebShellWindow.cpp --- nsWebShellWindow.cpp 2000/12/14 23:27:59 1.323 +++ nsWebShellWindow.cpp 2000/12/27 23:49:20 @@ -451,7 +451,15 @@ break; } case NS_MOUSE_ACTIVATE:{ + // replace persistent size data with the newly activated window's + void* data; + aEvent->widget->GetClientData(data); + if (data) { + nsWebShellWindow *win = NS_REINTERPRET_CAST(nsWebShellWindow *, data); + if (win->mChromeLoaded) + win->StoreBoundsToXUL(PR_TRUE, PR_TRUE, PR_TRUE); + } break; }

A patch for Mail/News? Yay! Not to be greedy, but can we get that patch applied to the Bookmarks, email Composition, and Address Book windows? :-)

Casey: unless I goofed, those windows should now all persist their zoom state. Note that the above patch (which by the way actually applies to bug 17149, which this bug has grown to encompass) only works on one platform; there are disappointing platform-specific issues about the reporting of window activation. Needs more work.

Don't forget about Search Bookmarks/History and the History window.

Adding a comment to make this bug report larger. PS - note the above patch is, unwisely, mostly removal of duplicated code. The actual functional part is restricted to two lines, numbers 186 and 187.

r=saari

WinME (probably all win9x variants) Moz.2000123120: Open two maximized windows, then minimize one of them. Working with the one remaining maxed window, open a new window (CTRL-N, open link in new window, whatever). This new window will not be maximized like the active window. Netscape 4.x will open the new window maximized, and I'd hope that behavior is carried over... Anyway, I'm not real programming saavy, so bear with me. danm, does the patch you posted (2000-12-29 14:35) address this? If so; huzzah!

New-windows-in-frontmost-window's-state patch checked in. Kevin: yes, it'll fix the problem you mentioned just above.

*** Bug 64348 has been marked as a duplicate of this bug. ***

I experience a weird behavior when I maximize a Mozilla 0.6 Browser window on Linux. All new windows are created unmaximized but the same size as the maximized window. With my window manager it's not easy to force the window different size then since its borders are hidden.

The fix was not in 0.6. Please use nightly builds or wait for 0.7.

I have noticed that the maximize fix does not work for the Mail window, but it does for all others now. Navigator, Address Book, Composer, Manage Bookmarks - these will remember that they had previously been maximized, but not the Mail window.

Casey: The main mail window (Tasks->Mail) and Message Compose work for me, but the mail message window (individual window for reading mail) doesn't. Is that the one you meant? That window probably just wants a one-line change to the xul. Anybody know offhand which file that is? Blake: ditto for Search Bookmarks/ History and History. hramrach: Linux builds don't know about min/max state; they remember only their size. See my comments above, 2000-12-22 18:01.

do you mean messageWindow.xul

It does not work for me when I try Tasks >> Mail; I still have to maximize the window manually. All other windows I can think of work just fine. I have downloaded new builds almost every night, looking for this fix to take effect, but it refuses to on my computer! :-(

timeless: well, that's an appropriately named window. Yup, that's it. Thanks! This patch fixes that problem Index: mailnews/base/resources/content/messageWindow.xul =================================================================== RCS file: /cvsroot/mozilla/mailnews/base/resources/content/messageWindow.xul,v retrieving revision 1.25 diff -r1.25 messageWindow.xul 44c44 < persist="width height screenX screenY" --- > persist="width height screenX screenY sizemode" Sigh. How many peoples' approval do I need to get this checked in? I'm starting a discussion with caseyperkins offline, though I do enjoy modifying this heavily watched bug report.

The Bookmarks/History search window is bm-find.xul (xpfe/components/bookmarks/resources/), although it already seems to persist window state. History is history.xul (xpfe/components/history/resources).

Ok, casey's right. If I max a window (nav, mail, composer, add book) then minimize it and use the 'Tasks' menu on some other netscape window to bring it back up the target window is no longer maximized - as it should be. This acts kinda like the bug fixed with the "New-windows-in-frontmost-window's-state patch." Footnote: This is observed with build 2001010804 (a day or so old).

Not only is the window "no longer maximized" - specifically, it is Restored, as if you had clicked the Restore button on the title bar. I noticed this because I had tiled my Address Book window with another program the other day, and after trying what you said (opening the Address Book, minimizing it, and bringing it back up via Tasks), it appeared in that Restored state. Same for the Composer window. That being said, what I originally had in mind was that the Mail window would never come up Maximized no matter how I opened it, or whether or not I had previously minimized it. Other windows would, though.

Now I tried 0.7 which behaves the same. I think it's possible to store the previous window size or define a default window size on Linux. (to allow 'unmaximizing' of windows that started in the 'maximized' size)

Blake: thanks for tracking those down. Both windows persist position and size, though they do position incorrectly. I'll get to those. Kevin: OK yes, I see that selecting a window from the Tasks menu resets its zoom state. That kind of wants to be a new separate bug. I suppose we can leave it as another piece of this ever-growing one, though. We're still trying to figure out what's wrong with Casey's Mail window which always refuses to persist its zoom state, no matter how you ask. hramrach: The problem is that on Linux there is no general, window-manager agnostic message stating that a window has been maximized. Or so I'm told. Certainly nothing I know about. It's no problem noting and persisting a change in size and/or position. In fact, that's what we're doing. The problem is realizing that the window has been maximized, and we therefore should *not* persist its size or position.

danm: I had a quick look at the new GNOME/KDE window manager spec, and to my limited knowledge it looks like it has a way of doing this ( http://www.freedesktop.org/standards/wm-spec/ ). One would expect most window managers will support this in the near future if they don't already.

Matthew: yes, you're right, we should probably support that spec. Alright, I'll try hooking up the Linux build that way. It remains to be seen which window managers actually send a _NET_WM_STATE event, but if those guys are staking out a standard, we'll try to follow. *My* window manager is going to have to support this stuff before I'll be able to actually check in a patch. I'll post a patch or a capitulation once I figure that out. With the recent move of timeless' 7000-line-diff cleanup effort to bug 65415, this issue becomes the only thing I know of that keeps this bug open.

I still see this problem in mail when the mail window layout is: ----- | | | ----- | | ----- that is, the top half has both folders and messages, and the bottom half is only for message display. The file is mail3PaneWindowVertLayout.xul I believe.

I just fixed the vertical mail layout problem that kerz mention (thanks to kerz for the fix.)

One more comment (sorry if already mentioned, but I am lost in this bug's comments!): From a maximized window, open a Javascript popup with specified width/height, minimize it and restore it back. Then the popup window gets maximized! If the "parent" window was not maximized, the popup retains its original dimensions. For example try the review of Dreamweaver 4 on Builder.com on a maximized window: http://home.cnet.com/webbuilding/0-7255-8-4535879-1.html?tag=st.bl.3880.pro_h.7255-8-4535879 Click on the "screenshots" link, minimize and restore the popup window. My build: 2001-01-30-04 Win98

Can't see any problem with the popup using 2001021506 on Linux/icewm.

As I understand it, in Linux we cannon (so far) persist a maximized state, only size and position of the window. That is probably why you don't observe the problem with the popups in Linux. Still present in Windows 2001-02-14-04 build.

->moz0.9.1

From danm@netscape.com 2001-01-16 17:02: (...) It remains to be seen which window managers actually send a _NET_WM_STATE event, but if those guys are staking out a standard, we'll try to follow. *My* window manager is going to have to support this stuff before I'll be able to actually check in a patch. (...) According to its authors Sawfish currently is 99% compliant so this shouldn't be a problem. You can check the state of its support of the freedesktop's wm specs on their bugzilla: http://bugzilla.eazel.com/show_bug.cgi?id=4268.

I don't think danm uses sawfish, but i do know that we have lots of sawfish bugs. considering this bug is marked pc/nt should we use a new bug to track sawfish/x11 managers?

I've recently moved to sawfish, with optimism. Haven't yet had a chance to look at this problem, being swamped for 0.9. I don't feel a great need to open a new bug for this one remaining facet of this bug. Besides, I love modifying this and just sitting back and watching the spam it generates.

No time for us to fix this in the current release cycle. ->future/helpwanted

WFM on Windows XP with build 12/23. This has WFMed for over a year. Why is this bug still open?

If this works for you, then mark it WFM...

The bug hasn't been fixed on Linux, see comment 108.

Not sure if this is related, but in 1.3a on Windows 2000, the window restore/maximized is correct UNTIL I exit Mozilla and restart. If I close Mozilla with a window maximized, then restart Mozilla, the 'restore' state of the Window uses the full screen. This was previously working correctly, ie. on restart it would remember the true last restored window size.

Mozilla 1.3 seems to save maximized state, but it doesn't revert to normal state when the window is unmaximized. New bug #198874 with that.

Confirming Ari's comment 153, I'm happy that 198874 will address my issue, and that since he created it, others have reproduced same problem.

This seems to be fixed in Firefox & gtk2. I vote for closing the bug.

In Firefox 2, the help window doesn't remember it's maximized state on Windows.

It's a pretty old bug and probably the original bug has been fixed, but in Firefox 2.0.0.9-3.1 (on opensuse 10.3/KDE 3), the maximised window state is not remembered. Once after an upgrade, it started with a smaller window and since then I can't get it to remember its maximised state. Also, the window position is not remembered. If I change the window size, the size is remembered, but at restart it's always positioned to the left of the screen.

Removed localstore.rdf from the profile, and the problem is gone. I still have the old file, if anybody is interested in it...

Well, that conclusion was a little too swift. The problem with the maximised state is gone. But the position of a normal window is not honoured at startup, although it is saved in localstore.rdf: <RDF:Description RDF:about="chrome://browser/content/browser.xul#main-window" sizemode="normal" width="839" height="515" screenX="374" screenY="177" /> </RDF:RDF> At startup, the normal window appears with the mentioned dimensions, but its top-left position is "0,0".

Edwin: on linux we don't actually restore the position of the window, instead relying on the window manager to do that for us. Not every window manager does that or is set up to do that, however. If you want to change that behavior, I suggest filing a new bug.