Created attachment 16868 [details] [review]
vertical and horizontal maximize toggle

submitted a patch to havoc.  Awaiting a reply.  The patch is attached
here as well. This is my first patch of any open source project, so
please be kind.  Any suggestions are very welcome.

Well, it's good that you're trying to help, but that patch isn't
right.    If we're to do this, we would want to replace
MetaWindow::maximized with maximized_horizontal and
maximized_vertical.  Maximization is then simply the combination of
both horizontal maximization and vertical maximization.  We would then
have to update all the contraints code so that if the struts change in
the work area the size of the window would also be updated; the fill
horizontal/vertical functions don't do this by themselves.

Also, the part of the patch where you set window->maximized if
window->maximized_horizontal and window->maximized_vertical is
completely broken -- the point of this code is the auto-maximize
windows that start larger than the work area.  The change you made
would basically just cause the automaximization to be completely hosed.

Doing this patch correctly is actually somewhat complicated.  You'll
need to modify contraints.c (meta_window_contrain) in addition to what
you have so far.  The session stuff will also need to be modified, in
a backwards-compatible way, and ideally in a way that won't cause
older metacitys to completely choke on the session file.

Basically, grep for any use of the maximized member variable anywhere
in the code, and extend it to support separate horizontal and vertical
maximization.

Sorry I think that came off overly harsh.  I don't want to sound mean
or anything :-).

Thanks for contributing to metacity, and I hope you'll take my
suggestions into account and work on a revised patch.

Thanks for the pointers.  I obviously didn't quite grasp the full
ramifications of what was going on.  I would like to try to make this
work though, but I have some questions.  

I understand your suggestion about replacing maximized with the
combination of max_vert and max_horiz  However, I tried to implement
this to be fairly unobtrusive.  What you suggest is a rewrite of how
windows know they are maximized.  While this sounds fine, is this
something that _should_ be done?  In other words, would anyone want
this if I went and tried to do what you are suggesting in changing how
a window knows it is maximized in all cases?  I'm always willing to
learn, I just want to make sure its not going to be all for nothing.  

Sorry to jump in late, I was on a bit of a vacation. 

Thanks a lot for the patch. My big-picture comment would concur 
with Rob, I think it's bad to have three flags that can be "out of
sync" - we need to have either just "maximized" or the two directional
maximizations.

The main reason I didn't do vert/horz maximization as states is
because the relationship of those to full maximization makes my head
hurt a little bit. However, I think essentially the right rule is that 
a window is "maximized" if it is both vertically and horizontally
maximized, and not maximized if it's only one of those.
You could perhaps introduce a META_WINDOW_MAXIMIZED() macro 
and some (but probably not all) places that currently use
window->maximized could use that.

I think there's a basic UI problem with these as states; which is that
you have a window that looks resizable (uses the unmaximized
decorations) but actually isn't resizable, or at least isn't resizable
in one of the directions. There's also no UI indication of the
modality of the state (as with the maximize/restore window button and 
the changed window border).

However, since vert/horz maximize are in the WM spec, and they are
just hidden keybindings, I'm OK with the UI problem - average users
aren't going to encounter it, and people that know about vert/horz
maximize probably won't care.

The only reason I'd say we might not want to add the separate
vert/horz states would be if they make part of the code such as
constraints or themes a lot more complicated. I don't think they 
will, but the only way to know is to try writing the patch. ;-)

Anyway, if you want to give it a shot, I think it's a useful thing.
Thanks a lot.

Ok, I'll give it a whirl.  For future reference, should I continue to
ask questions I'm undoubtedly going to have to this bug, or is there a
developer list to which I should subscribe?   

All discussion is in bugzilla for now, so asking stuff on this bug 
is fine. The only list is metacity-maint@bugzilla.gnome.org
which just gets all the bugzilla spam (every time any bug is changed);
you can be on that if you want, but I really doubt you want. ;-)

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

Adding the PATCH keyword.

removing PATCH since the patch isn't quite right.

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

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

It looks like quite a few people want this fixed, yet nobody is working on it. I
need it as well, so I'm going to try and help out. I've reworked the patch by
the original bug reporter. Some of the comments you guys made on that patch
would still apply, I think, but let's start with this though. Please let me know
what you think, and I'll try to do what needs doing.

Created attachment 32097 [details] [review]
vertical and horizontal maximization toggle

I am the original reporter of this bug and the author of the first ill-conceived
patch.  I looked at your patch.  I'm wondering if it is worth having a
window->maximized attribute at all.  Can't that just be implemented as both
window->maximized_horizontally and window->maximized_vertically?  There can be a
macro that joins these two when you want to check if it is maximized.  It just
seems redundant and likely to get out of sync.  

It is great that someone is working on this.  I have since switched to openbox
because of this and a few other issues coupled with my inability to provide a
better patch.  I would live to try metacity again though. 

it's certainly possible to replace the maximized attribute with a macro, but I
don't particularly see a reason to. It's not going to get out of sync with the
two directional flags, simply because it's never set without taking care of
those as well, and vice versa. (there is one exception, in
update_net_wm_state(), but that's kind of a noop, doesn't do anything).

So unless the metacity maintainers insist on this, I'd just leave it like this.

Bert: read comment 3 and comment 6 of this bug.  Just because there's not
currently a way for them to get out of sync doesn't mean that a patch in the
future could easily accidentally do so.  Keeping all three attributes is just a
maintainenance problem.  Also note that Rob pointed out in comment 3 a number of
additional things that need to be done.

Created attachment 32213 [details] [review]
horiz/vert maximization, with window::maximized flag gone

This implements vertical and horizontal maximization toggling, and replaces the
window::maximized flag with a macro META_WINDOW_MAXIMIZED() that checks both
the horiz and vert maximization flags instead. Thus, maximization is now a
combination of the two.

I'm not sure what, if anything, needs to be done about meta_window_constraint()
beyond using the macro instead of window::maximized... Rob, can you please tell
me what you meant by that?

Also the session stuff I haven't touched beyond using the macro. It's basically
not possible to do it in a backward-compatible way, and do it properly as well.

You could abuse the maximized element to mean either horizontal or vertical, or
both, by then checking the values in the attributes against live screen values.
But that's silly, IMHO, and a lot of work.

Unfortunately, it's impossible to add any elements or attributes to the session
file with it still being backward-compatible -- since the parser stops
processing with an error when it encounters an unknown element or attribute. My
suggestion would be the following:

- For this new maximization functionality, add an attribute "direction" to the
"maximized" tag, containing either the value "vertical" or "horizontal".

- to ensure future backward compatibility with new features in the session
file, start ignoring unknown elements and attributes.

I'm going to need one of the maintainers to give me an opinion on this, since I
don't think it's up to me to decide what to do with this session file.

On the session file, my approach for now would be to simply not change the
format, so you would lose an only vert/horz maximized state across session save.

The real fix is to rename the session file (create a new file version, as in the
patch I have for theme files). But not worth it just for this. We could open a
bug about doing that someday.

Nobody uses session management anyway so it's not worth the risk of breakage here.

So... is this patch going to be applied, then?

Well, I'm not the maintainer (i.e. feel free to ignore me), but I thought I'd
point out a couple things:

Patches are easier to review if you also use the -p flag.

Can you really replace window->maximized with META_WINDOW_MAXIMIZED(window) in
constraints.c?  That may be correct (I haven't looked that closely), but I was
assuming that each of the two new states would have to be treated specially,
just as we currently have to treat the maximized state.  (See also below where I
noted some special cases in window.c where it looks like this)

In your changes to keybindings.c you ignore the indentation style; the rest of
the code uses 2 space indentations.

Also in keybindings.c, why not replace the
  if (window)
    if (window->has_resize_func)
with
  if (window && window->has_resize_func)

You have a few gratuitous changes from spaces to tabs which adds length to the
patch when nothing is really changing, making it harder to review.  It'd be nice
if you could remove these.

In your changes to the check_maximize_to_work_area function, you remove the call
to meta_window_get_outer_rect() which will result in rect being uninitialized
and give undefined behavior.  You also add "|| META_WINDOW_MAXIMIZED(window)" to
the if condition to see whether to maximize the window, but it seems odd to
maximize the window if it's already maximized.

In your changes to update_move(), you changed window->maximized to
META_WINDOW_MAXIMIZED(window) in order to determine whether to "shake the window
loose from being maximized".  Why can't this apply to vertically maximized
windows too?  (I guess a follow up question is why isn't horizontal shaking
loose allowed--even for windows maximized in both directions; but that's
something that existed before your patch...)

In that same function, you also changed window->maximized into
META_WINDOW_MAXIMIZED(window) in order to determine whether to not allow
movement, without doing further work.  It would seem that horizontally and
vertically maximized windows would require some extra code to ensure that they
don't move in the appropriate directions.

Like Elijah said and I've said before, we need actual code to maintain the
maximization constraint in constraints.c.  The key thing to notice here is that
if you do a "maximize horizontal" then add a panel on the left, or resize an
existing panel, the window's size and position need to change such that it
remains maximized in that direction, and not too wide or too narrow.  This
requires treating the two maximization states separately in constraints.c.

To clarify a little here: the functions to toggle the maximized
horizontal/vertical shouldn't actually calculate the maximized size of the
window.  What I would do most likely is convert the regular toggle maximize
function so that it takes two boolean arguments, one for horiz and one for vert
(or both to toggle both, or none to, um, well do nothing I suppose).  The actual
working of that particular function should only be a couple lines different from
what it is now.  The meat of this is in constraints.c.

Is this a little clearer?

Yes, this is what I needed. I'll get busy, and report back with a better patch :)

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

I'd like to add that the same problem is there for plain maximize.

e.g.
Maximize by hitting Alt+F10, hit again does nothing.

Lex: You probably don't have Alt+F10 bound to "toggle maximize".  By default, it
is bound merely to "maximize".  If a window is already maximized, you can't
maximize it again and expect it to do anything different.  You either need to
hit the unmaximize keybinding (Alt+F5 by default) or else go to your keybinding
preferences and change the Alt+F10 binding from maximize to toggle maximize.

should this be the default setting? 

i.e. Alt-F10 bound to toggle maximizated state instead of just maximize window.

Lex: Possibly.  If you think so, check to make sure there are no other bugs on
the subject, and if there aren't then file one.  It's not related to this report.

Unmaximize is Alt+F5, the current way is deliberate consistency with the
CUA/Windows/whatever keybindings.

updating per comments

[Cue Wizard of Oz music]

Ding! Dong!  The bug is dead!
The wicked bug,
The wicked bug,
Ding! Dong!  The wicked bug is dead...

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

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