378841 - View source window does not tile properly or remember size/position

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: General, defect)

Description

[froodian (Ian Leue)]

+++ This bug was initially created as a clone of Bug #331066 +++

The new view-source in window (yay!) has a few issues with tiling behavior:

I have my windows full-height.  When I open a new browser window, the new window is full-height, and, because it can't stagger downward and remain full-height, the new window just staggers to the right (as does each successive window).  This is the way tiling is supposed to work in this case.

The source window, however, does not behave the same way.  It 1) shrinks the height and 2) tiles too far down (instead of the standard titlebar+2px height, this is like titlebar-and-a-half).  Also, if I open a second source window while the first is open (say, to compare the sources of two pages), the second window opens exactly on top of the first instead of tiling at all!

Moreover, the source window always opens at the same size and position; it doesn't remember it's been resized.

Comment 1

[Chris Lawson (gone)]

The view-source window tiles properly (relative to its "parent" browser window) if the toolbar on the parent window is hidden. It doesn't tile properly relative to other view-source windows or other browser windows, however.

Hiding the toolbar on the parent window doesn't seem to have any effect on the lack of size/position memory of the view-source window, either.

I'll try to look into this more soon. I suspect what's at work here is our severe reluctance to let any window without full chrome mess with window size/position memory. Yet Another Reason why a new class of window for view-source would be useful, I guess :(

cl

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We can fix the remembering size/position part of this bug easily; just replace

[controller disableAutosave]; // don't save view-source window size/position

with

[[self window] setFrameAutosaveName:@"ViewWourceWindow"]; // allow saving view-source window size/position independently of standard BrowserWindows

I'd kinda like to do just that, and punt on tiling (and initial sizing).  I don't think that would make the current behavior any worse.

I wonder if we can fix the initial sizing (if the ViewSourceWindow key does not exist) by getting stringWithSavedFrame from our BW, then manually adjusting the size and tiling (we still have our own code to to that[1]), and then calling setFrameFromString: when opening the window?

[1] http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWindowController.mm#643, http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWindowController.mm#1090

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Partial fix: turns autosave back on with a custom name for view-source windows

On further investigation, I think there are 3 parts to this bug:

1) Setting the initial size and position of the window the first time it is ever opened

2) Saving the user-chosen size (and default position, but see below)

3) Tiling multiple view-source windows sanely


I have 2 working with the line in comment 2 (sans typo).*

I tried to implement part of 1 (setting the initial size only, not worrying about tiling) and didn't have any luck with that, at least not the way I was trying to implement the last paragraph of comment 2—I ended up causing the BrowserWindow to move, and not actually resizing the view-source window :(  Since part 1 works (successfully resizes the window on open once the key is set), I'm sure someone can probably figure this part out.

Part 3 is actually the hardest part, because, upon further investigation, Cocoa (or our own tiling code) is actually doing the right thing right now.  That is, we always tile the based on the current frontmost window.  Since the only* way to get a view-source window is to have a browser window frontmost, we're always going to tile based on the browser window.  If you trigger two v-s windows from the same browser window, they're going to "tile" on top of each other (unless you've moved the first one)—the same thing would happen if you used the "Open in New Window" context menu on each of the links in comment 2.  To really fix this, we'd have to add code to our tiling code to check to see if there is currently a view-source window at the size/position where we're about to open the new one, and then tile.  I'm not sure it's worth fixing, TBH.

* You can also open a new view-source window by having a view-source: URI in a tab and moving that tab to a new window via the tab widget CM.  That will reset the size of the next true source window (because the view-source: window that got created had toolbars, and back to all that fun stuff).

Ultimately, however, as I said in comment 2, just turning on autosave with a specified name fixes about 80% of what bothers me about this bug (preserving size during a session, and also across restarts), and I think it's worth taking that part alone now to get size preservation for the common case.

Comment 4

[Chris Lawson (gone)]

Comment on attachment 482643 [details] [diff] [review]
Partial fix: turns autosave back on with a custom name for view-source windows

LGTM

Comment 5

[Stuart Morgan]

Comment on attachment 482643 [details] [diff] [review]
Partial fix: turns autosave back on with a custom name for view-source windows

sr=smorgan

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is not working now and I have no idea why :(

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: mostly-working fix

I'm not sure how I ever had anything working in that last patch; it must have just been sheer luck of all the quirks in our code and bugs in my patch working in my favor while testing :P

I've made some more progress; the problems (generally speaking) were:
1) I wasn't ever setting the frame on the view-source window
2) I needed to use [controller window] when acting on the source window I was "creating", as [self window] was still the "parent" browser window
3) |windowDidLoad| and |autosaveWindowFrame| have a some hard-coded assumptions that they're only dealing with "real" browser windows, which lead to craziness and the bad interactions that caused us to disable autosave in bug 331066.

1 was easy to fix.  2 was easy to fix once I realized what the problem was, but terribly frustrating until I had that realization :P  I think I have 3 mostly fixed (I haven't really tried to tease apart the logic in |windowDidLoad| yet).

There are a couple of things left:

i) If a user has a source window open, has resized a browser window since opening the source window, but has the source window active when Cmd-N (or anything else that will call |openNewWindow:|), we'll autosave the source window at that point (when we really want to autosave the frontmost non-source browser window) and not pick up the new size.  TBH, this seems like kind of an edge case and I'm not sure we care about it; we don't really handle it correctly now, either (since mShouldAutosave is NO for source windows).

ii) Figure out if any changes are necessary in |windowDidLoad|.  I'm not sure about the hierarchy of the calls, and everything seems to be working OK, but that just may be luck.  Also, it's 3am and I wanted to be alseep an hour ago.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Note to self (or anyone else who attempts to pick this up): bug 478077 comment 4