GNOME Bugzilla – Bug 358311
Unidirectional maximized windows do not honour regions.
Last modified: 2007-04-08 23:41:33 UTC
Please describe the problem: When using unexpanded struts, the work area of a screen isn't a rectangle anymore. However, the xinerama work area is set to be the largest rectangle that doesn't overlap with any strut. Using this rectangle is fine when one fully maximizes a window, but in the case of unidirectionally maximized windows it can be surprizing that certain parts of the screen aren't used for maximization. Steps to reproduce: 1. Create a new panel (strut) 2. Change the properties of the panel to be not-expanded 3. Put the (now small) panel somewhere at the top of monitor, a bit to the side. 4. Open a small window that, if maximized vertically will NOT bumb into the panel at the top. 5. Maximize the window vertically Actual results: The window will maximize till a height equal to the bottom of the panel (as if the window was very wide and hitting it). Expected results: The window should have maximized all the way to the top of the monitor. Does this happen every time? Always Other information: I'm working on a patch for this. This bug is totally unrelated to the shake loose code, but - after fixing this bug, the shake loose code needs change too in order to correctly detect when the remaximize. I'm still working on the latter, as well as making it work for the full xinerama: At the moment the code only takes struts into account of the "current" xinerama, this is nonsense for unidirectional maximized windows as they can still be on two or three monitors: the regions of several xinerama's need to be taken into account (depending on the direction of maximization).
Created attachment 73675 [details] [review] Fix for this bug This patch fixes the behaviour as described in the initial comment. Windows are now maximized as far as possible without hitting struts, as they should be. The patch actually just adds intelligent support for unidirectional maximization in constrain_maximized. However, it contains a hunk here and there that sneaked in while I was browsing the code and couldn't help myself from fixing it: a typo in a comment - a more sensible way to avoid a compiler warning - and a speed up ('continue', instead of setting factor = 0 followed by cpu intensive calls that make no sense).
Created attachment 73778 [details] [review] Same as previous, without introducing TAB's This patch is the same - but does not introduce new TAB's to the code (and removes them from lines that have been altered).
Comment on attachment 73778 [details] [review] Same as previous, without introducing TAB's Some overall points: - The algorithm being used is a bit difficult to follow which makes it hard to verify correctness. I'll try to ask for explanations below. - My understanding (haven't worked out the details completely yet) is that you're basing your algorithm mostly off the spanning/maximal rects available in usable_screen_region and usable_xinerama_region. In other words, you're looking at valid areas for the window to go and (mostly) trying to fit it into one of those. This has problems in that it is okay to take the window offscreen in the non-maximized direction, which complicates things a bit and which I think you are trying to handle with the relevant_area stuff. - I'm trying hard to make constraints.c short and readable. Thus, ugly rectangle computations should go in boxes.[ch]. Ideally, such computations should have corresponding test cases in testboxes.c. - I think the algorithm would be much easier to follow and more efficient if you turned it on it's head. Rather than looking at valid areas (usable_screen_region and usable_xinerama_region), look at the struts. I envision the following algorithm (in pseudocode): if (maximized_horizontally && maximized_vertically) set "work_area" to work_area_xinerama else if (maximized_horizontally) set "work_area"'s x,width to full xinerama size (ignore struts) clip out any vertical struts the window overlaps with else if (maximized_vertically) set "work_area"'s y,height to full xinerama size (ignore struts) clip out any horizontal struts the window overlaps with +meta_rectangle_find_extents (const GList *region, + MetaRectangle *extents) Not that it matters, but I'll just point out that this function would be faster when dealing with multiple partial struts if you started with the full screen (or xinerama) size and then just clipped out the relevant struts. I believe that region has O(N^2) rectangles (where N is the number of struts) in more pathological cases (I never bothered figuring out if it was N^2 or some other factor of N, but it doesn't really matter). Of course, anyone using very many partial struts probably deserves any slowdown they get for using a dumb setup. :) + int const very_large = 1000000; + int left = very_large; + int right = -very_large; + int top = very_large; + int bottom = -very_large; Why not just use INT_MIN and INT_MAX rather than some arbitrary large number? @@ -811,29 +839,28 @@ meta_rectangle_clip_to_region (const GLi Yeah, your changes to this function look better. :-) - /* Determine distance necessary to put rect into comapre_rect */ + /* Determine distance necessary to put rect into compare_rect */ Good eye. - work_area = info->work_area_xinerama; + if (window->maximized_horizontally && window->maximized_vertically) + work_area = info->work_area_xinerama; + else Perhaps we should rename work_area? I associate work_area with _NET_WORKAREA, which is fine in the fully maximized case. What you use work_area for is different than the value of _NET_WORKAREA, though. + FixedDirections fixed_direction; Why not just keep using window->maximized_horizontally and window->maximized_vertically? You're basically using fixed_direction as an alias for which of those two is true, which just requires reviewers and maintainers to do a little more work to figure out what you mean. + /* Initialize relevant_area with the minimal rectangle + * that containing all workspace. This can be smaller + * than window->screen->rect in the case of expanded + * panels. We need to know that in order for this panels are not the only things which can be struts. They are the canonical example, but some accessibility apps (screen readers, IIRC, and maybe others) and other applications can also have windows that are "screen reducing". + /* Set work_area to the rectangle that we would maximize + * to if there were no struts. This isn't what you actually do below. You are only setting work_area to the rectangle that we would maximize to if there were no _partial_ struts (as per your other comment, xinerama_extents ignores "expanded panels"). + * At the same time, clip relevant_area to the maximum size + * (of the window) in the direction that we maximize in, + * and possibly adjust it also for expanded struts on + * this xinerama in the direction we do not maximize in, + * that weren't 'detected' yet because there is no such + * strut on the adjacent monitor (in the direction that + * we are maximizing in). Slightly confusing, partially because it's just a lot of words, but partially also because you were sure to always clarify in all cases except one: Expanded panels on this xinerama absolutely will be detected in all cases with your algorithm; their existence will be reflected in xinerama_extents. I believe you were just referring to the fact that such panels don't affect relevant_area. + /* Check if the edge of the xinerama is (likely) the screen edge. + * If so, clip the relevant area to the xinerama extents. + * This is needed to account for expanded struts at the top/bottom + * of the xinerama. + */ + if (BOX_BOTTOM(xinerama_extents) > + BOX_BOTTOM(relevant_area) - info->entire_xinerama.height / 4) This comment doesn't match what the code does either. I'm not sure whether you mean "screen" edge (the edge of usable_screen_region) or screen edge (edge of window->screen->rect), but the code does neither. If it did either of those, it would be using == or != (possibly inside of some loop for the former), rather than >. I'm guessing the code was a hack meant to avoid a loop? If so, it doesn't work since struts can be half the monitor width or more in _both_ directions (in fact, there used to be some bugs with acessibility screen readers because metacity didn't handle such cases well). Also, if you just want to check whether a xinerama edge coincides with a screen edge, wouldn't comparing window->screen->rect and info->entire_xinerama by easier? + BOX_RIGHT(relevant_area) - info->entire_xinerama.height / 4) + BOX_LEFT(relevant_area) + info->entire_xinerama.height / 4) copy&paste & search&replace bug on those lines. (s/height/width/) + /* At this point, work_area is equal to the window, unidirectionally + * maximized to the extend of the current xinerama. That means: + * maximized while taking expanded struts into account, but not + * yet taking unexpanded struts into account. + * + * At this point, relevant_area is a rectangle that, in the direction + * that we maximize in is equal to the current xinerama excluding + * possible expanded struts at its edge, and in the direction that + * we do not maximize in, equal to the whole screen excluding possible + * expanded struts at its edge. For some reason, I found this quite confusing the first time or two I read it. It makes sense now, but it's hard to build up the model you're aiming for to understand it. I'm not sure how to make it clearer given your algorithm, though. + /* Skip rectangles that the fixed direction doesn't fit into. */ + if (BOX_LEFT(relevant_rect) < BOX_LEFT(*compare_rect) || + BOX_RIGHT(relevant_rect) > BOX_RIGHT(*compare_rect)) + continue; This comparison ignores the frame of the window, thus letting the frame of the window be hidden behind struts when it shouldn't be. (Due to edge resistance, you can't just slowly move a window towards a partial strut to see this; you have to move it up to it, have it just past the edge, and then slowly move the window back towards the edge you just jumped past). + * Just using compare_rect->height would work, in the + * sense that in that case simply the largest space is picked, + * to maximize into, but that would take away all control from + * the user about where to maximize a window. Rather we include + * saved_rect some how, in a way that theoretically the window + * can end up anywhere (in a sensible way). I don't understand why saved_rect is being included. + * Consider the following: + * + * saved_rect + * <----------------> + * <----lenA--> lenB == 0 + * A <---------STRUT--------> B + * <------------------Xinerama-side----------------------> + * + * In this case we want to maximize into area A. Um? Even when the window doesn't fit? This comment seems to imply that you will be vertically maximizing the window in a way that ignores the partial strut. That isn't what the code does (well, ignoring my comment about the window frame), but the comment is once again a bit misleading or difficult to understand. + * saved_rect + * lenA == 0 <----------------> lenB == 0 + * A <---------STRUT--------> B + * <------------------Xinerama-side----------------------> + * + * We just want to maximize to the largest of A and B. Why should the window jump left or right? Why not just be maximized where it already is? Or am I just totally misunderstanding your comment? + overlap = MAX(1, + MIN (BOX_BOTTOM(window->saved_rect), + BOX_BOTTOM(*compare_rect)) - + MAX (BOX_TOP(window->saved_rect), + BOX_TOP(*compare_rect))) * + compare_rect->height; Above you went to all kind of lengths in your ascii art to show these horizontal lengths, but then you start comparing BOX_BOTTOM and BOX_TOP values? I don't understand. @@ -992,13 +1238,15 @@ do_screen_and_xinerama_relative_constrai Nice cleanup in this section. :)
Oh, and as always, thanks for working on this. :) (And re-reading some of my comments, it looks like I can't type worth beans today; lots of typos and problems; let me know if any of what I said doesn't make sense)
Created attachment 74462 [details] Examples of unidirectionally maximized windows. Lets tackle this in this order: 1) We agree on what we want the code to do 2) We agree on the algorithm 3) We agree on some code details 4) We agree on the patch I'm not even sure we agree on 1) yet, so I made this drawing. It is now 6:30 am ... so I'm going to bed first ;). I'll add comments tomorrow.
Ok, good morning. I hope you can see the png image of the last attachment; it has a transparent background. You might want to set the background of your browser to white. The colors have the following meaning: Black outlines four xinerama monitors. Red are struts; c and d are expanded, a, b, e and f are partial struts. Green represents the current value of saved_rect. Blue is the window maximized vertically. Magenta is the window maximized horizontally. Note that you need to apply the xinerama patch of bug #356715 before it is garanteed that saved_rect is indeed what it should be: having the same x-coordinates as only vertically maximized windows, or the same y-coordinates as only horizontally maximized windows. Without, I believe this is only the case as long as you don't move the maximized windows. Case 4 in the image is the simplest case-- so, lets look at that one first. Window 4 overlaps with two xinerama's, the larger area being on xinerama 4. Maximizing it horizontally therefore chooses xinerama 4 to jump to. It it should maximize horizontally there. It should ignore strut 'f', unlike what metacity does currently. Maximizing it vertically doesn't change the x-coordinates, however -- in this case it should be aware of struts 'd' and 'e' on xinerama 3 (even though xinerama 4 would be the xinerama returned when asking for "it's" xinerama). Case 3 trivially maximizes vertically. When maximized horizontally, the attached patch would expand it up till strut 'e' and not overlap with 'e' to expand till the right edge of xinerama 3. After reading your comments, I am no longer certain you want this behaviour, but it seems the only logical thing to do: consider this window being a little higher, so it expands fully horizontally. Then the user moves it downwards. If the strut was a top-strut and you were grabbing the window at the title-bar, the strut would stop you from moving it down (up) any further. However, if you first unmaximized it (to the green outline) - then of course you could move it further down. Subsequentially maximizing it horizontally again isn't allowed (as per our rationally in http://bugzilla.gnome.org/show_bug.cgi?id=356715#c4) to have the window jump vertically imho, so the only reasonable way to maximize it is the magenta rectangle. If however the window overlaps with an expanded strut, as is very well possible, we have case 2. In that case, maximizing it vertically should ignore the expanded strut as well as it ignores the fact that it is partially off-screen (compare window 4 being partially on xinerama 3 and maximizing it vertically). Maximizing case 2 horizontally should pick xinerama 2, and then take strut 'c' into account, of course. Finally, the most difficult case. In case 1, the unmaximized window wants to overlap with a partial strut. Maximizing it horizontally is no different than case 2: it should maximize horizontally, taking strut 'a' into account. Maximizing it vertically is a different story however. Assuming we do what we did in case 3, and take strut 'a' into account, there are two possible areas to expand to: the blue one and the yellow one. The algorithm should pick one, based on the resulting size (blue is larger), but possibly also taking the position of saved_rect, and/or the grab position of the pointer in the case of moving the window (horizontally), into account. Do you agree? Or do you have any good arguments against the described behaviour?
I really like the picture and agree with most the behavior. However, I do disagree with the horizontal maximization of 3 and the vertical maximization of 1. First, let me address your points on the matter, then I'll add some extra comments of my own: > Case 3...consider this window being a little higher, so it expands fully > horizontally. Then the user moves it downwards. If the strut was a top-strut > and you were grabbing the window at the title-bar, the strut would stop you > from moving it down (up) any further. I think you meant "up" rather than "down (up)" considering the start of that sentence. Is that right? If not, I don't understand. If so, it's not actually true -- try it out. A partial strut at the top doesn't and shouldn't prevent the window from being moved upward as long as enough of the titlebar remains visible for the user to click on. It does provide some slightly stronger edge resistance since it's a "screen" (which I use loosely to mean the screen minus struts) edge instead of a window edge, but it does not preclude movement. So, I don't see the connection you made for coming to this conclusion; it seems a bit the opposite. Here's where my line of thinking came: Let me create a modified version of case 2 that I'll call 2m, where the window has been moved from the position in case 2 until it's fully onscreen. Then maximize the window vertically, which, as you point out, would trivially make it take the full height of the monitor. This is case 2m and I'll talk about what happens to it after we do more things: Let me start with where I think we agree. Let's say we're in case 2m (vertically maximized window in the top-right monitor that is fully onscreen) and the user starts dragging their window to the right until it would overlap the strut. What do we do? Well, as you pointed out, that strut should be ignored so we just let the user take the window off the screen and it looks basically like the old case 2 you showed. Okay, hopefully we agree so far. Let's return to case 2m and make another modification. Instead of strut c being fully expanded, let it be a strut missing about 10 pixels of the height of the monitor (i.e. it's a non-expanded panel that has been stuffed chock-full of all kinds of applets). In other words, the difference between this new case and case 2m is hardly noticeable. Now, pull the window to the right until it overlaps the strut. It seems the behavior should be the same as before given that the case is nearly identical. To do otherwise would mean trying to slam the window into a 5 pixel high margin (assuming the 10 pixels were split evenly between top and bottom), which isn't even enough room for the titlebar. Now, the case may be a little extreme, but what if the panel covers 95% of the height of the right side? 90% 85%? In short, my argument is this: Right and left side struts shoudln't affect vertical maximization; top and bottom side struts shouldn't affect horizontal maximization.
(In reply to comment #3) > - The algorithm being used is a bit difficult to follow which makes > it hard to verify correctness. I'll try to ask for explanations > below. Nothing is too difficult; it all comes down to understanding it first ;) I decided to comment on your remarks, but please note that hardly anything is really relevant anymore: because the behaviour has to be changed (ignoring struts perpendicular to the maximization direction), I'll have to start from scratch. > - My understanding (haven't worked out the details completely yet) > is that you're basing your algorithm mostly off the > spanning/maximal rects available in usable_screen_region and > usable_xinerama_region. In other words, you're looking at valid > areas for the window to go and (mostly) trying to fit it into one > of those. This has problems in that it is okay to take the window > offscreen in the non-maximized direction, which complicates things > a bit and which I think you are trying to handle with the > relevant_area stuff. The chosen algorithm was trying to do what I showed in case 1 and 3. In order to achieve that, you NEED to do what I did. I believe your understanding as mentioned here is mostly correct, though I objectc to words like "complicates things" and "trying to". It's not really that complicated, and the use of the relevant_area is just... well... brilliant would be a better word ;) :p > - I'm trying hard to make constraints.c short and readable. Thus, > ugly rectangle computations should go in boxes.[ch]. Ideally, > such computations should have corresponding test cases in > testboxes.c. I had put certain code in boxes.c at first - I can't remember why I did put them back. It's not that relevant anyway, because I'll have to rewrite everything anyway. > - I think the algorithm would be much easier to follow and more > efficient if you turned it on it's head. Rather than looking at > valid areas (usable_screen_region and usable_xinerama_region), > look at the struts. I envision the following algorithm (in > pseudocode): > if (maximized_horizontally && maximized_vertically) > set "work_area" to work_area_xinerama > else if (maximized_horizontally) > set "work_area"'s x,width to full xinerama size (ignore struts) > clip out any vertical struts the window overlaps with > else if (maximized_vertically) > set "work_area"'s y,height to full xinerama size (ignore struts) > clip out any horizontal struts the window overlaps with This is the algorithm you wanted (horizontal maximization ignores vertical struts and vica versa). Using the struts for that, instead of the valid areas seems, indeed, the only way to do that. The other way around, trying to implement what I did, I think that using the valid areas is the best method (you'd have problems implementing case 1, for example - because you don't know - while 'subtracting' strut 'a' that later you will subtract 'b', nor do you know the order, actually. That would complicates things too much ;) > +meta_rectangle_find_extents (const GList *region, > + MetaRectangle *extents) > > Not that it matters, but I'll just point out that this function would > be faster when dealing with multiple partial struts if you started > with the full screen (or xinerama) size and then just clipped out the > relevant struts. I believe that region has O(N^2) rectangles (where N > is the number of struts) in more pathological cases (I never bothered > figuring out if it was N^2 or some other factor of N, but it doesn't > really matter). Of course, anyone using very many partial struts > probably deserves any slowdown they get for using a dumb setup. :) You're right; I wasn't aware that there WAS a list of struts at all actually. Can you tell me where I can find the list of all struts? > + int const very_large = 1000000; > + int left = very_large; > + int right = -very_large; > + int top = very_large; > + int bottom = -very_large; > > Why not just use INT_MIN and INT_MAX rather than some arbitrary large number? I was too lazy to look them up (for some reason I can never remember their name). It works either way - the assembly code is equal in size/speed too :p > - work_area = info->work_area_xinerama; > + if (window->maximized_horizontally && window->maximized_vertically) > + work_area = info->work_area_xinerama; > + else > > Perhaps we should rename work_area? I associate work_area with > _NET_WORKAREA, which is fine in the fully maximized case. What you > use work_area for is different than the value of _NET_WORKAREA, > though. Sure. I just used the name that the unaltered code below this used. If I change the name of the variable, the diff would become larger and include code that wouldn't really be changed, except for a change of name of one variable. I consider that harder to review and therefore try to avoid changes of that kind (even though I'd normally include such improvements - when it was just my own code). > + FixedDirections fixed_direction; > > Why not just keep using window->maximized_horizontally and > window->maximized_vertically? You're basically using fixed_direction > as an alias for which of those two is true, which just requires > reviewers and maintainers to do a little more work to figure out what > you mean. You're right... this sneaked in because before I had code in boxes.c, that was general. Of course, I couldn't use window->maximized_horizontally there. I used 'fixed_direction' there (like so much other code in boxes.c). When later I moved the code back to constraints.c I didn't remove the use of that variable. > + /* Initialize relevant_area with the minimal rectangle > + * that containing all workspace. This can be smaller > + * than window->screen->rect in the case of expanded > + * panels. We need to know that in order for this > > panels are not the only things which can be struts. They are the > canonical example, but some accessibility apps (screen readers, IIRC, > and maybe others) and other applications can also have windows that > are "screen reducing". I'll try to use consistently "expanded strut" and "partial strut" in comments. > + /* Set work_area to the rectangle that we would maximize > + * to if there were no struts. > > This isn't what you actually do below. You are only setting work_area > to the rectangle that we would maximize to if there were no _partial_ > struts (as per your other comment, xinerama_extents ignores "expanded > panels"). You're right... I never write code top to bottom. I write it with a rather complex method (that they no doubt don't teach at universities ;), it's my own method that builds on "assumptions" (meaning, while writing a piece of code, I assume certain edge conditions are already fulfilled, while they are not yet - I assume certain functions handle things, while the functions do not yet exist, etc.) The code "grows", like a tree: everywhere, in bits and pieces. Sometimes I make the mistake to collapse code into different functionality (that is, instead of doing A and then B, I do AB at once, for example) without updating the comments. The reason for that is that I normally don't write comments; I'm not used to updating them I'm afraid. I'll try even harder to make sure that my comments reflect what the code really does, in the end. > + * At the same time, clip relevant_area to the maximum size > + * (of the window) in the direction that we maximize in, > + * and possibly adjust it also for expanded struts on > + * this xinerama in the direction we do not maximize in, > + * that weren't 'detected' yet because there is no such > + * strut on the adjacent monitor (in the direction that > + * we are maximizing in). > > Slightly confusing, partially because it's just a lot of words, but > partially also because you were sure to always clarify in all cases > except one: Expanded panels on this xinerama absolutely will be > detected in all cases with your algorithm; their existence will be > reflected in xinerama_extents. I believe you were just referring to > the fact that such panels don't affect relevant_area. The second half of the comment refers to the code quoted below, and well case 2 and strut 'c': relevant_area.x and .width for monitor 2, for a vertically maximized window, wouldn't take strut c into account, because there is no expanded strut on the right-side of monitor 4, and therefore the extents of usable_screen_region, used as initialization for relevant_area, extends to the right screen edge. I'm afraid the comment is correct, though very unclear, I guess. > + /* Check if the edge of the xinerama is (likely) the screen edge. > + * If so, clip the relevant area to the xinerama extents. > + * This is needed to account for expanded struts at the top/bottom > + * of the xinerama. > + */ > + if (BOX_BOTTOM(xinerama_extents) > > + BOX_BOTTOM(relevant_area) - info->entire_xinerama.height / 4) > > > This comment doesn't match what the code does either. I'm not sure > whether you mean "screen" edge (the edge of usable_screen_region) or > screen edge (edge of window->screen->rect), but the code does neither. > If it did either of those, it would be using == or != (possibly inside > of some loop for the former), rather than >. I'm guessing the code > was a hack meant to avoid a loop? A loop?? Heh :). No, no. It DOES assume however that struts are always less high than entire_xinerama.height / 4, if that is not the case as you point out below, then it is not correct (although, the comment DOES say "likely"). > If so, it doesn't work since struts > can be half the monitor width or more in _both_ directions (in fact, > there used to be some bugs with acessibility screen readers because > metacity didn't handle such cases well). Also, if you just want to > check whether a xinerama edge coincides with a screen edge, wouldn't > comparing window->screen->rect and info->entire_xinerama by easier? Confusing names there... I'd have use gdb to print values before I can be sure what they mean :p. From this comment, I'd deduce that entire_xinerama means all of everything (monitor 1, 2, 3 and 4 combined) and window->screen->rect would be just monitor 2 (for a window on monitor 2). Problem there is, of course, that 'window->screen' is fuzzy to begin with: for this algoritm it is hardly interesting to know which monitor contains the largest part of the window, using window->screen seems a bit arbitrary to me. I think that it can be used, but only really when all monitors have the same resolution (but then again, if they don't - things become rather fuzzy anyway. > + BOX_RIGHT(relevant_area) - info->entire_xinerama.height / 4) > + BOX_LEFT(relevant_area) + info->entire_xinerama.height / 4) > > copy&paste & search&replace bug on those lines. (s/height/width/) Nah, I didn't see a reason why struts would be wider than high. I used the same "constant" for both directions. > > > + /* Skip rectangles that the fixed direction doesn't fit into. > */ > + if (BOX_LEFT(relevant_rect) < BOX_LEFT(*compare_rect) || > + BOX_RIGHT(relevant_rect) > BOX_RIGHT(*compare_rect)) > + continue; > > This comparison ignores the frame of the window, thus letting the > frame of the window be hidden behind struts when it shouldn't be. > (Due to edge resistance, you can't just slowly move a window towards a > partial strut to see this; you have to move it up to it, have it just > past the edge, and then slowly move the window back towards the edge > you just jumped past). This code is in fact duplicated code, from meta_rectangle_clamp_to_fit_into_region, meta_rectangle_clip_to_region and meta_rectangle_shove_into_region. meta_rectangle_clamp_to_fit_into_region is only called in constraints.c and passes &info->current as 'rect' (where I use relevant_rect). Same for meta_rectangle_clip_to_region and meta_rectangle_shove_into_region. relevant_rect is basically work_area, which is basically info.current... Hence this code. I fail to see why in the first three cases it is correct, and in the new, duplicated code, not :/ Nevertheless, ignoring where the code came from -- I'll make note that you want the frame to be taken into account. Altough... when maximizing horizontally, isn't the frame left and right removed? > + * Just using compare_rect->height would work, in the > + * sense that in that case simply the largest space is picked, > + * to maximize into, but that would take away all control from > + * the user about where to maximize a window. Rather we include > + * saved_rect some how, in a way that theoretically the window > + * can end up anywhere (in a sensible way). > > I don't understand why saved_rect is being included. That was to give the user a little bit more control of where a window would be maximized in the case of choice (case 1). But since the algorithm will be changed, there will be no choice anymore, so this won't be relevant anymore. > + * Consider the following: > + * > + * saved_rect > + * <----------------> > + * <----lenA--> lenB == 0 > + * A <---------STRUT--------> B > + * <------------------Xinerama-side----------------------> > + * > + * In this case we want to maximize into area A. > > Um? Even when the window doesn't fit? This comment seems to imply > that you will be vertically maximizing the window in a way that > ignores the partial strut. That isn't what the code does (well, > ignoring my comment about the window frame), but the comment is once > again a bit misleading or difficult to understand. I think you just didn't understand what this comment is about. It is about case 1 and strut 'a' (maximizing the window vertically). 'A' would be the yellow choice, and 'B' the blue choice. > + * saved_rect > + * lenA == 0 <----------------> lenB == 0 > + * A <---------STRUT--------> B > + * <------------------Xinerama-side----------------------> > + * > + * We just want to maximize to the largest of A and B. > > Why should the window jump left or right? Why not just be maximized > where it already is? Or am I just totally misunderstanding your > comment? yes (misunderstand). The vertical dimension in this picture is not relevant, there is only dimension: the horizontal one. See the vertical direction in case 1. > + overlap = MAX(1, > + MIN (BOX_BOTTOM(window->saved_rect), > + BOX_BOTTOM(*compare_rect)) - > + MAX (BOX_TOP(window->saved_rect), > + BOX_TOP(*compare_rect))) * > + compare_rect->height; > > > Above you went to all kind of lengths in your ascii art to show these > horizontal lengths, but then you start comparing BOX_BOTTOM and > BOX_TOP values? I don't understand. Come on :p -- think a little bit more abstract :p. The Ascii Art drews easier horizontally; we have to deal with BOTH directions. This code deals with the vertical case, a bit lower we have the horizontal case (I didn't repeat the hugh comment there though). It's just like previously I said "down (up)" and then continued with the grammar for the picture (strut at bottom) while using the (incorrectly presumed) behaviour of a top strut :p :p But I guess it's just me - I think very abstract and to me certain things are so much the same that (apparently) I don't make enough effort to distinguish them in communication. Hmm... I once made a nice puzzle: Suppose you have hypercube of N dimensions, and you paint each of the 2N "outsides" out of a palet of k colors. Then how many different permutations can you make? The reasoning to find the correct answer demands that you can understand the symmetry between all dimensions (and exactly what makes them different): Each and every axis is 100% equivalent to the other dimensions (in regard to permutations: two painted hypercubes are the same when they can be made completely the same after rotation in N-dimension space) EXCEPT for one bit: you might have to mirror non-symmetrical cases: only cubes that have a different color on the opposite sides for *every* axis degenerate with a factor of two. Once understanding that, it's rather easy to find the formula (that is, the insight might take you a week, after which finding the formula is a matter of a few more hours). I'll work on a new patch to implement the new understanding of the algorithm.
Created attachment 74634 [details] Corrected image reflecting desired behaviour. Considering bugzilla a bit as documentation, I felt we needed an updated image.
Created attachment 74635 [details] [review] Unrelated code clean-up hunks from last patch. Having to do a rewrite, I wanted to get those non-related hunks out of the way. Please commit (or not)... * src/boxes.c (meta_rectangle_clip_to_region): * src/constraints.c (do_screen_and_xinerama_relative_constraints) Some minor code clean up.
Created attachment 74649 [details] [review] Try two I still had questions about this, but since you're not online allow me to put the ball in your park by attaching this patch. It seems to work, but I am very unsure about the correctness of finding the struts to take into account. Differences with http://bugzilla.gnome.org/attachment.cgi?id=74634&action=edit : strut b and d are ignored: they aren't in the all_struts list! Was this something you forgot to mention, or should I fix it so it will "see" such struts too? Also, struts that are in the middle of a xinerama should be ignored, but does that also hold for struts that are put 'on top' of other struts? For example, if you use an auto hiding panel - there remains 1 pixel on the screen. You can put another strut against that-- but it isn't a real strut that way. Shouldn't it behave like a real strut?
ping Elijah? What's up with the sudden silence?
Created attachment 84627 [details] [review] Version 4 (20070315) of "maxconstraint" patch. This is an update of the patch to match the current state of SVN.
Basically, the patch looks good for the most part. I was able to find three issues, two of which are trivial to fix. Also, other than one of the trivial fixes, the problems were really caused by the existing metacity code rather than your patch. Anyway, the three issues: - I'd prefer big box-like computations to be in boxes.[ch], in an attempt to keep functions in constraints.c relatively short and readable. - There's no need to check for xineramas of different sizes; the correct way to handle that (as I've said on some other bug in bugzilla somewhere) is to add struts corresponding to the "offscreen" regions on the smaller xinerama. Then this case is automatically handled, as is placement, keeping windows at least partially on the screen, etc. It's really a separate bug, but fixing it will remove the need for the special-case code you added. - Your heuristics for determining which struts have an orientation perpendicular to the window's maximization direction are not very robust. For example, take a gnome panel at the bottom of the screen, make it a partial strut (by unselecting the 'Expand' option) and make sure to move it all the way to the left. For me, that gives a bottom panel occupying 95% of the screen width (I have a pretty full panel), and having horizontally maximized windows smashed into the remaining 5% at the right isn't very nice. The first point is easily fixed by just moving most of your new code into a function in boxes.c. The second point merely means removing a bit of your code. The third point means an extensive overhaul of strut handling throughout metacity to make struts be recorded as both a side (left/right/top/bottom) as well as a rectangle. That sounds kind of nasty, but some of the strut parsing/setup/data structures were in need of a good overhaul anyway. So, I went ahead and did the strut overhaul. Then I made a few small changes to your patch needed to get it to work with the overhaul and to make the two trivial corrections noted above, then committed it.
Created attachment 85747 [details] [review] strut handling overhaul + adaptation of Carlo's patch (Just sticking this patch here for completeness of the record, and to make the job of interested backporters and such have an easier time finding it)
And committed your cleanup patch too. There were a couple other functions where identical cleanups to yours were valid so I did the same thing to those functions too. Thanks!