As far as the maximization goes, we're violating this part of the EWMH:

  "The Window Manager should remove the property whenever a window is withdrawn,
   but it should leave the property in place when it is shutting down, e.g. in
   response to losing ownership of the WM_Sn manager selection."

In bug 137185, we fixed our bug with the first half of that requirement for the maximized state, and added a bug in the second half.  So, we should make it conditional on window->withdrawn.  Also, when the window is being withdrawn, we should be cleaning off more than just the maximized state.

Created attachment 73565 [details] [review]
Fix to preserve geometry and maximization state info over a restart.

After several gdb sessions, generously using hardware watch points, I was able to figure out that the maximization information was lost over a restart (running a new invocation of metacity --replace &) because the old metacity is simply resetting this info. As Elijah pointed out after a brief discussion online,
this is necessary only for withdrawn windows, and should specifically not be done when windows are massively being unmanaged.

After changing the code to only reset _NET_WM_STATE for withdrawn windows, it turned out that only fully maximized windows were properly restored to there maximized state. Uni-directional maximized windows were merely enlarged. This patch fixes this by calling meta_window_maximize_internal in all cases, passing the appropriate flags.

Finally, it turned out that the unmaximized state of maximized windows wasn't preserved correctly. In order to fix this, the window HAS to be unmaximized (that is, moved and resized, without changing _NET_WM_STATE) before closing. However, because of existing bugs in the past about 'flashing' behaviour when closing windows (see #317254) we can't just unmaximize windows in all cases.
This patch therefore only unmaximizes windows in meta_window_free when the correspondig screen is marked as closing.

Patch is tested for closing windows (they do not unmaximize), for withdrawn windows (_NET_WM_STATE is reset) and for restarting metacity (unmaximized geometry is preserved and the maximized state is preserved).

Created attachment 73604 [details] [review]
Fix to preserve minimization state info over a restart.

Minimized windows add the atom "_NET_WM_STATE_HIDDEN" to _NET_WM_STATE.
Metacity was ignoring this atom. This patch detects if if this atom is set in reload_net_wm_state and sets a new boolean 'minimize_after_placement' if so.
This boolean is tested in place_window_if_needed, and when set, meta_window_minimize() is called. Another bug in place_window_if_needed was fixed where it tested for !META_WINDOW_MAXIMIZED, which only works for fully maximized windows. The large hunk in the patch for place_window_if_needed is mostly a re-indentation however, as I did split a single if into two if's.

Finally, a bug in meta_window_new_with_attrs, introduced by the patch for bug #334899, was fixed - where all new windows are unminimized regardless. This patch does not minimize windows when they are being created as part of opening a new display. In order to achieve that, a new boolean was added to the display struct that is set while inside meta_display_open.

The patch was tested with the test case of bug #334899, and this still works: the close confirmation window pops up correctly. Also was tested restarting metacity with minimized windows - which not correctly remain minimized.

Why can't I type a comment without typos?
Obviously, the last line of the previous comment should read:
which now correctly remain minimized.

(In reply to comment #2)
> Created an attachment (id=73565) [edit]
> Fix to preserve geometry and maximization state info over a restart.

Patch looks good.  Please use -p in addition to -u in the future when creating patches; it makes them much easier to review.  Anyway, do you want to post a changelog entry and I'll commit this one?

Oops, missed that you added a few tabs in that last patch; those need to be removed.  That happens to be the only problem I could find in the second patch as well.  So, if you add a ChangeLog entry for them and fix up the tab thing, I'll commit.  (I'll mark accepted-commit_now despite the tab thing, though tabs should be pulled before the actual commit)

Thanks!

(In reply to comment #6)
> Oops, missed that you added a few tabs in that last patch; those need to be
> removed.

I don't understand, there should be no TAB's in the source code
of metacity?

I mean...

/usr/src/metacity/metacity-cvs>cvs diff 2> /dev/null
/usr/src/metacity/metacity-cvs>cat src/*.c src/*.h | grep ' ' | wc --lines
1524

Where the grep contains a single TAB.
Since there are already currently 1524 lines containing a TAB in the source code, I am not sure if I understand you.

(In reply to comment #7)
> I don't understand, there should be no TAB's in the source code
> of metacity?

That would be nice.  Unfortunately, there have been many in charge of patch review/committing and not all have been diligent (I'm sure I didn't check patches closely enough in some cases as well).  There are other coding style problems in various places as well.  I don't like changing them just to fix them since that introduces spurious conflicts with backporting and such, but I do fix them if they are on lines I'm modifying anyway (or are very close).

In general, while I can't make the coding style perfectly consistent in the code, I'm trying to keep it to be reasonably consistent (with itself; it's not actually the same coding style I use in my own projects, but I think self-consistency is much more important).

Created attachment 73777 [details] [review]
The previous two combined, but without TABs.

No changes here - I just removed the TABs in the modified code (basically, I replaced ^+(^I)* in the diff with +...spaces... and manually fixed any TABs introduced between statements and comments.

I'm using a system that automatically switches enviroment and history files when changing directory, the editor (vi) is part of that automatic environment. I changed it to:

/usr/src/metacity>alias vi
alias vi='vim -c "set tags=/usr/src/metacity/metacity-objdir/src/tags" -c "set expandtab"'

Note the added 'set expandtab' ;) So, no more TABs from me for this project in the future.

Most of my recent projects have a little Emacs header 
/* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
or for metacity a better one is:
/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */

My .emacs has indent-tabs-mode nil in it, but putting this in the files fixes it for at least those contributors using emacs, which helps a lot.

Having the file header is a big plus if you work on multiple projects with different coding styles. Just sticking this on top of every file in metacity would not hurt.

I don't know if there's a vi equivalent...

I guess this is a total tangent to the bug we're posting to ;-) 

(filed Havoc's suggestion as #358866)

	Patch from Carlo Wood to ensure that maximized and minimized
	properties are maintained across restarts.  #358042.

	* src/constraints.c (place_window_if_needed): fix up partial
	maximization handling and add minimize_after_placement handling.

	* src/display.[ch] (struct MetaDisplay, meta_display_open): add a
	new display->display_opening flag to allow handling startup
	differently where needed.

	* src/window-props.c (reload_net_wm_state): handle
	_net_wm_state_hidden as well, setting
	window->minimize_after_placement appropriately

	* src/window.[ch] (struct MetaWindow, meta_window_new_with_attrs):
	add a window->minimize_after_placement field

	* src/window.c (meta_window_new_with_attrs): only unminimize the
	window and its transients if the display isn't being opened,
	(unmaximize_window_before_freeing): don't reset the state unless
	the window is becoming withdrawn, if the screen is being closed be
	sure to save the unmaximized state of the window so the next
	window manager can restore it

Heh - I thought I had to write that... I was just finished:

        Fix to preserve geometry and maximization/minimization state info
        over a restart.  #357695.

        * src/display.h (struct _MetaDisplay):
        new display_opening bit.

        * src/display.c (meta_display_open):
        Set the new display_opening bit while inside meta_display_open.

        * src/window.h (struct _MetaWindow):
        new minimize_after_placement bit.

        * src/window.c (meta_window_new_with_attrs,
        unmaximize_window_before_freeing):
        Initialize new minimize_after_placement bit.
        Only call unminimize_window_and_all_transient_parents when not being
        called from meta_display_open.
        Only call set_net_wm_state in unmaximize_window_before_freeing when
        withdrawing a window. Put the window back at it's unmaximized
        position without calling set_net_wm_state when being called while
        closing the screen.

        * src/window-props.c (reload_net_wm_state):
        Set minimize_after_placement if new window has
        atom_net_wm_state_hidden.

        * src/constraints.c (place_window_if_needed):
        Also delay placement of unidirectional maximized windows and minimized
        windows. Once placed, always call meta_window_maximize_internal, also
        for unidirectional maximized windows (and not just fully maximized
        windows). Call meta_window_minimize for minimized windows.


Oh well. Either is fine by me :p

Created attachment 73945 [details] [review]
Proposed additional text

I don't think vi has a way to add directives to the source code.
I saw you added the directives for emacs in the meantime, perhaps
add some new text to HACKING for vi users?

Sorry to abuse this bug - but it is what was already being used
off-topic for this :p

The proposed text uses 'I', being a quote from you (elijah), from
comment #8. I decided to leave it that way because HACKING is using
'I' already in several places, so it's the 'style' and I wanted to
stay consistent ;) :p

(In reply to comment #13)
> I don't think vi has a way to add directives to the source code.

It does-- see bug 358866.

Comment on attachment 73945 [details] [review]
Proposed additional text

Ah, I stand corrected. I changed the status of the patch to 'rejected' because it is off-topic here (otherwise 'needs-work' might have been better. Lets continue this topic in bug #358866