GNOME Bugzilla – Bug 644930
One pixel of border resizing is frustrating for user
Last modified: 2011-08-10 12:43:22 UTC
I propose to remove the ability to resize a window by the 1px border. The current behavior requires super precise positioning to achieve it willingly, while still poses a good chance to do it accidentally. Every window now has a resize grip that is discoverable and reasonably sized. Users familiar with the functionality will likely use the modifier key + mousebutton to resize. While this functionality is hard to discover, just like finger gestures once learned, it becomes far superior way to move and resize windows. In addition long term goal is to minimize "window management" so people don't really have to resize all that often.
In future we might create an overlay on the focused window while <super> is held that indicates better than the window is resizable (similar to selected objects in a graphics editor get this bounding box overlay).
Makes some sense, not doing for 3.0, would violate UI freeze, and create more distraction.
Yeah definitely post 3.0.
*** Bug 644874 has been marked as a duplicate of this bug. ***
<owen> walters: we pontentially could do somtehing with input shapes to make the effective border bigger than the visual border, but think it's going to be fairly complex <owen> If we drop border resizing altogether, we're really going to have to have gtk+ tell the wm that it does corner resizing and do a enlightenment-13 style corner overlay on windows that dont' advertise that (can be a bit prettier with modern capabilities) because we have to accomodate motif apps, etc.
This is a joke, not serious right?
A good solution (a least in my daily experience) is the "invisible" active area currently on ubuntu natty. Basically the visible window border is 1 or 0 pixels, but the actual area you can use to resize the window is 5 pixels wide. This is good for theme makers (borderless themes) and for users (no need to hunt the exact and unique pixel). Implementation details are here: * http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/natty/metacity/natty/revision/101 * http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/natty/light-themes/natty/revision/28 * 01_unity_window_decorator.patch inside this file https://launchpad.net/ubuntu/+archive/primary/+files/compiz_0.9.4-0ubuntu7.debian.tar.gz
Resizing via the bottom corner only is not a good solution imho, trythat one the mac, it's annoying. The invisible border solution as in ubuntu natty Luca proposed sounds definitelly better to me.
I suggest adding Alt+(Right mouse button) for window resizing (there is window moving with alt+left mouse, so it seems consistent).
it's already there, it's just Alt+middle button. I don't know why.
Alt-right is menu, which sort of make sense. (There's a gconf key to change.)
(In reply to comment #0) > I propose to remove the ability to resize a window by the 1px border. The > current behavior requires super precise positioning to achieve it willingly, > while still poses a good chance to do it accidentally. > > Every window now has a resize grip that is discoverable and reasonably sized. > Users familiar with the functionality will likely use the modifier key + > mousebutton to resize. While this functionality is hard to discover, just like > finger gestures once learned, it becomes far superior way to move and resize > windows. In addition long term goal is to minimize "window management" so > people don't really have to resize all that often. The rezise grip doesn't appear on all applications, and only being able to resize a window in one bottom corner, doesn't really work that well as it makes it impossible to resize a window unless the bottom of the window is visible on the screen. The resize grip have annoyed me for years on the Mac, and now that Apple is finally abandoning it in Lion, we should not make the same mistake in Gnome The invisible border thing in Ubuntu sounds like a much better solution
I played a bit with the invisible border on ubuntu, the solution works pretty well since it's small enough to not be confusing and it's big enough to be able to grab it, also the pointer change its shape when overing it, so it's pretty clear what would going to happen clicking and dragghing that. I think in the ubuntu patches there are properties to define if there's an invisible border and its size in the theme, I think we should do it automagically with no theme tweaking involved. If the borders in metacity/mutter themes are smaller then say 4 px add a 4px invisible border around the window. As I said the solution works pretty well, it's not confusing and it's familiar for the people, while not having to waste screen estate for big window borders (see vista/win7 for example), the way to go imho.
What about tablets?
I guess the invisible border should just make things better on a touchscreen as well since the target is bigger, anyway I have no means to test it atm.
*** Bug 650766 has been marked as a duplicate of this bug. ***
I think that enforcing the resize grip for all windows is not a good solution, since it can obscure content (right aligned text in status bar) or controls (status icons, scrollbar buttons) in an application that are not aware of the resize grip (eg. KDE applications, Firefox, applications launched through wine).
*** Bug 652803 has been marked as a duplicate of this bug. ***
Created attachment 190633 [details] [review] Rough draft at invisible dragging bounds. This is an extremely rough WIP. It works, though. I've disabled the XShape-based rounded corners for now, as I'm planning on drawing the corners by making a path and pushing it to the clipping path, otherwise events would leak through to the parent window while hovering on the "cut out" place with the rounded corner while going to the left and hovering over the corners would be annoying. Yes, this means anti-aliased corners :) For now, the value is in the theme format: modify the frame_geometry XML tags and add a "padding" value. I'm going to turn this into a GConf setting in the future. This is implemented by making the frame's X window $padding size larger on the left, right and bottom, and clipping the actual window shape in the window actor. This means that effectively, running mutter uncomposited is dead. Open questions: * Right now I have one 'padding' value, which I add to the left, right and bottom. Should I allow for separate values for each border? * I'm curious how many applications/toolkits use the parent frame to get their extents instead of using _NET_FRAME_EXTENTS like a polite adult. * What should meta_get_outer_rect return -- just the visible region, or the input region as well? Should I add a new method?
The same value for all invisible borders is ok, no need to allow separate values. I don't even understand the other questions :-) Something which could be interesting is having the invisible borders size somehow tied to the input device, think about a touch screen with a connected mouse, bigger value for the "finger" and smaller for the mouse. If it's a mess to implement, as I fear, forget about it, though.
Created attachment 190947 [details] [review] MetaShapedTexture: Add invisible_clip This specifies a cairo rectangle that will use the cogl clip stack to clip the shaped texture. This will be replaced with a CoglTexture so we can paint some anti-aliased corners on it.
Created attachment 190948 [details] [review] theme: Add 'padding' value to theme This will be scrapped in the future in favor of a GConf/GSettings value, so don't bother with the theme format documentation now.
Created attachment 190949 [details] [review] Start passing 'padding' around Add an unused padding parameter to start passing around all over the place. This should give the padding value to almost all the places that need it.
Created attachment 190950 [details] [review] MetaWindow: Repurpose get_outer_rect and add get_input_rect get_outer_rect now returns the visible region, and a new get_input_rect method returns the boundaries of the full frame, including the possible invisible regions. When undecorated, both do the samething.
Created attachment 190951 [details] [review] frames: Temporarily disable shaping
Created attachment 190952 [details] [review] MetaWindowActor: set the invisible clip region This completes the illusion: we now calculate where to clip, making the invisible borders actually invisible.
Created attachment 190953 [details] [review] Compensate for new invisible border changes Start reflecting what the new geometry of real X window looks like: ______________________ | /______________|X\ | | | | | | | | | | | | | | |________________| | |______________________| A lot of code made the previously completely valid assumption that the top-left of the real X window matches with the top-left of the actual window frame contents, like some parts of the code that drew the title bar and calculated which controls users were hovering over.
Created attachment 190954 [details] [review] frames: Add a bit of padding to resize N on the title bar This is optional, and I'm not sure I like it just yet, but it adds a RESIZE_N area to the visible title bar.
Review of attachment 190948 [details] [review]: To me, this really *does* belong in the theme - if I had a theme like the GNOME 2 theme with thick borders, adding padding in addition to that (or at least, as much padding as we want to add for the GNOME 3 theme) doesn't make sense; so I'd generally like to see a theme version bump, theme version checks, and docs for this. (But am willing to listen to arguments for why it should be a config parameter instead :-) Maybe padding isn't the best name here - it's pretty generic, and padding is also in CSS spacing *inside* the visible border? I'm also more inclined to go with a four-sided approach - not that I can see any case where it's likely where we'll want anything other than left=right=bottom, top=0 - but it seems more natural - if I can set the width of the left and right visible click areas to be one thing and the bottom to be something else, shouldn't I be able to adjust the invisible parts to match? So, I guess my instinct would be to add: <border name="outside_click_border" top="0" right="10" bottom="10" left="10"/> to frame_geometry. (Doing it as a separate element rather than an attribute also means that the theme can do: <border version=">= 3.N" name="outside_click_border" .../> )
Review of attachment 190949 [details] [review]: Since we probably want unequal values on the 4 sides, I think an intro patch to make this: meta_ui_get_frame_geometry (ui, xwindow, MetaFrameGeometry *frame_geometry); would be a good idea (maybe with some sorting out of MetaFrameGeometry :-)
Review of attachment 190950 [details] [review]: Looks fine modulo necessary changes from other patches
Review of attachment 190951 [details] [review]: The end result shoudn't be "disabling" shaping - it rather should be applying the shape we want for the input region (which might be no shape)
(In reply to comment #29) > Review of attachment 190948 [details] [review]: > > To me, this really *does* belong in the theme - if I had a theme like the GNOME > 2 theme with thick borders, adding padding in addition to that (or at least, as > much padding as we want to add for the GNOME 3 theme) doesn't make sense; No, that's not the idea of the setting. It's meant as "minimum draggable border", so assuming it's set to 5px, the invisible border would depend on the visible border in the theme - if the theme uses a 1px border, an invisible border of 4px would be added, if it uses 10px, no invisible border is needed at all.
Review of attachment 190947 [details] [review]: Adding a separate setter for this essentially doesn't make sense to me - there's no difference in what this does to the ability to set a shape with: meta_shaped_texture_clear_rectangles()/meta_shaped_texture_add_rectangle()/meta_shaped_texture_add_rectangles() I think the best thing to do is to replace that set with: meta_shaped_texture_set_shape(cairo_region_int_t *region); (as a preparation patch) and then pass in a cairo_region_int_t that is the output shape we want - which is: (frame_bounds - inner_bounds_rectangle) union (window_shape) If we then at some point want to special case the case where the region is a single rectangle and avoid creating a mask texture, but just clip the output drawing ourselves - either by choosing which rectangles we draw, or by using a Cogl clip, then we can do that, but it's an optimization encapsulated within MutterShapedtexture.
Review of attachment 190952 [details] [review]: How I think this should work described in the other patch, more or less. One implementation note - you need to switch from tracking the shape of the frame xwindow in MetaWindowActor to tracking the shape of the client Xwindow - you could just switch out the code in MetaWindowActor to use the client xwindow instead, but I think it might be cleaner to use the fact that the core code is *already* tracking whether the client xwindow is shaped and when the shape changes, and then add a: meta_compositor_window_shape_changed() method to let the compositor know, and throw out the selecting/filtering for events in MetaWindowActor - when that function was called, we'd still set a "needs reshape" flag we'd check later before drawing in the same way as currently.
Created attachment 191161 [details] [review] Remove public MetaFrameGeometry, replace with MetaFrameBorders There were actually two MetaFrameGeometry structs: one in core/frame.h, and one in ui/theme-private.h. This was unnecessarily confusing. Remove the former, smaller one, and replace it with MetaFrameBorders. Additionally, add some extremely simple helper methods, and rename and replace things that dealt with MetaFrameGeometry-related offspring, like the functions that passed "top_height" pointers everywhere.
Created attachment 191162 [details] [review] MetaWindow: Repurpose get_outer_rect and add get_input_rect get_outer_rect now returns the visible region, and a new get_input_rect method returns the boundaries of the full frame, including the possible invisible regions. When undecorated, both do the samething.
Created attachment 191163 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and related yak shaving. This gets us parity with the core code: any time we need a frame border, it's done through MetaFrameBorders. Additionally, start us off towards that 'invisible borders' goal by making the math here more correct.
Created attachment 191164 [details] [review] core: Work towards invisible borders
Created attachment 191165 [details] [review] MetaWindowActor: set the invisible clip region This completes the illusion: we now calculate where to clip, making the invisible borders actually invisible.
Created attachment 191166 [details] [review] theme: add dummy values for testing
Created attachment 191167 [details] [review] disable shaping to test drawing code
OK, this patch set here replaces the "padding" approach with the four-border struct. I need to clean it up eventually, but I tested with visible and invisible borders combined, and it works in most places. It doesn't work on edge tiling or screen edge constraints for unknown reasons. I'd like for some people to test this and verify that it *is* working on their system. This still uses the separate rectangle approach -- hopefully I can get that over the finish line soon.
Created attachment 191189 [details] [review] MetaWindow: Repurpose get_outer_rect and add get_input_rect get_outer_rect now returns the visible region, and a new get_input_rect method returns the boundaries of the full frame, including the possible invisible regions. When undecorated, both do the samething. Fix a merge issue with the patches before, as well as fix broken math. This fixes screen edge constraint misalignment, except for the top of the screen. Surprisingly, maximized does display correctly.
Created attachment 191190 [details] [review] MetaWindow: Compensate for invisible border changes Fix some merge and rebase issues
Created attachment 191191 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and related yak shaving. This gets us parity with the core code: any time we need a frame border, it's done through MetaFrameBorders. Additionally, start us off towards that 'invisible borders' goal by making the math here more correct. Fix some rebase and merge issues, as well as some math.
Created attachment 191556 [details] [review] Remove public MetaFrameGeometry, replace with MetaFrameBorders There were actually two MetaFrameGeometry structs: one in core/frame.h, and one in ui/theme-private.h. This was unnecessarily confusing. Remove the former, smaller one, and replace it with MetaFrameBorders. Additionally, add some extremely simple helper methods, and rename and replace things that dealt with MetaFrameGeometry-related offspring, like the functions that passed "top_height" pointers everywhere. I'm just replacing the whole stack in case I fixed up anything. I don't believe there's anything major in this patch though.
Created attachment 191557 [details] [review] MetaWindow: Repurpose get_outer_rect and add get_input_rect get_outer_rect now returns the visible region, and a new get_input_rect method returns the boundaries of the full frame, including the possible invisible regions. When undecorated, both do the samething.
Created attachment 191558 [details] [review] MetaWindow: Compensate for invisible border changes
Created attachment 191559 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and related yak shaving. This gets us parity with the core code: any time we need a frame border, it's done through MetaFrameBorders. Additionally, start us off towards that 'invisible borders' goal by making the math here more correct. Fixed math error in populate_cache
Created attachment 191560 [details] [review] testing: Use simple values for debugging. renamed to "testing" so it's clearer this isn't supposed to land
Created attachment 191561 [details] [review] constraints: Fix constrain_titlebar_visible to account for visible borders
Created attachment 191562 [details] [review] constraints: Fix constrain_fullscreen
Created attachment 191563 [details] [review] MetaWindowActor: Compensate for invisible borders
Created attachment 191564 [details] [review] MetaShapedTexture: Fix indentation This is a code style fix to make the code easier to see and edit.
Created attachment 191565 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects
Created attachment 191566 [details] [review] MetaWindowActor: remove priv->shaped
Created attachment 191567 [details] [review] Track client window's XShape directly.
OK, the only thing that's happening but I can't figure out why is when you first go to/from the overview, the positions are wrong, but they're fine afterwards. Related to bug 651012 maybe?
There looks to be a few other issues: * XShaped windows don't work. I thought I tested and they worked, but apparently I didn't. It would be neat if I could put the mask from the stex into a PNG. * Edge tiling seems to have incorrect positioning. * The preview window for edge tiling doesn't work, but the effect does. * When coming back from edge tiling, sometimes the frame isn't painted correctly all the way.
Created attachment 191575 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects Fix shaped windows by: * punching out the client window * getting the shaped rectangles directly from the client window * offsetting the shape rectangles with the borders * unioning
More bugs: * Full-screen windows don't work sometimes, not sure when/how. Adjusting constrain_fullscreen to contain arbitrary numbers doesn't seem to affect these windows. * Coming back from full-screen windows will shift the window down and to the right a bit. Probably has to do with incorrect saved_rect handling. * When restarting mutter, all non-ARGB32, non-maximized windows get shifted around the screen to various places. Probably a rogue constraint.
Created attachment 191600 [details] [review] frame: Drop apply_frame_shape Because we don't need to set the input region from mutter, and we're tracking the client window directly, we don't have to send the shape back to the server. This fixes invisible borders on shaped windows and improves the performance on them dramatically. There's no visible lag on oclock like there was previously.
Created attachment 191607 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects Fix check_needs_reshape to account for the possibility of a not-framed window. Fixes the tile preview, menus and the screensaver.
Created attachment 191665 [details] [review] MetaWindow: Compensate for invisible border changes Welp, turns out all that fancy constraint stuff was due to broken math when computing the client's position. This commit (and the next one, the 'ui' commit) is somewhat of a big mess now, and I should break it apart, really.
Created attachment 191666 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and related yak shaving. This gets us parity with the core code: any time we need a frame border, it's done through MetaFrameBorders. Additionally, start us off towards that 'invisible borders' goal by making the math here more correct. This is a big, ugly commit and it *needs* to be broken apart: this introduces the theme correction where we set invisible borders along with the compensation for the rest of ui/.
Created attachment 191667 [details] [review] frame: Drop apply_frame_shape Because we don't need to set the input region from mutter, and we're tracking the client window directly, we don't have to send the shape back to the server. This fixes invisible borders on shaped windows and improves the performance on them dramatically. There's no visible lag on oclock like there was previously.
Created attachment 191668 [details] [review] MetaWindowActor: Remove priv->shaped Because we're no longer pushing the visible region back up to the X server, is_shaped becomes incorrect. Instead of trying to fix that, assume that most windows have a visible shaped region and dump the optimization. Fix up commit message.
Created attachment 191669 [details] [review] Track client window's XShape directly Because we're no longer pushing the visible region back up to the X server, we don't want to track the frame window, we want to track the client window. Fix up commit message.
Created attachment 191670 [details] [review] MetaWindowActor: add a preferred width/height This fixes unintented WindowOverlay positioning in the overview. this isn't working... I'll investigate tomorrow.
TODO: * investigate frame painting on the right side, I think it has to do with cached_pixels in frames.c * investigate MWA allocation * nab invisible borders from gconf/gsettings * testing. lots of testing Tested so far: * Complex button layouts with multiple buttons on the left and right. * Visible and invisible borders mixed and intermingled. * Edge tiling, maximized and full-screen. * Shaped and ARGB32 windows. Probably pre-existing, but ARGB32 windows don't work in the overview (workspace thumbnails or window clones) * Restarting the shell/mutter to make sure that nothing looks ugly or gets out of sync.
Created attachment 191703 [details] [review] MetaWindow: Compensate for invisible border changes Fixed up some rendering errors related to unmaximizing windows after a shell restart.
Created attachment 191704 [details] [review] MetaWindowActor: Compensate for invisible borders Oops, attached the wrong patch. I need to do something about those similar commit messages :)
Created attachment 191705 [details] [review] MetaWindowActor: Remove priv->shaped Because we're no longer pushing the visible region back up to the X server, is_shaped becomes incorrect. Instead of trying to fix that, assume that most windows have a visible shaped region and dump the optimization. Fixed a logic flow in set_visible_region that effectively made it leak memory and ignore the shape region.
Created attachment 191708 [details] [review] MetaWindow: Fix MAXIMIZED macros When we originally place a window, we don't set the "maximized" hint, even though it should be placed maximized. Effectively, this means that windows placed maximized get the wrong frame state, causing ugly rendering errors. This fixed a major ugly rendering error: 1. Open an application that remembers where it was last, e.g. Nautilus 2. Maximize it. 3. Close it. 4. Go to overview and drag out a new nautilus. 5. See a horribly mismatched and deformed frame.
Created attachment 191771 [details] [review] frame: Drop apply_frame_shape Because we don't need to set the input region from mutter, and we're tracking the client window directly, we don't have to send the shape back to the server. This fixes invisible borders on shaped windows and improves the performance on them dramatically. There's no visible lag on oclock like there was previously. Rebased to start of patch set so we don't have to maintain code we're chucking, it's just a giant removal patch.
Created attachment 191772 [details] [review] MetaShapedTexture: Fix indentation This is a code style fix to make the code easier to see and edit.
Created attachment 191773 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects Fixed a bad comment.
Created attachment 191774 [details] [review] MetaWindowActor: Remove priv->shaped Because we're no longer pushing the visible region back up to the X server, is_shaped becomes incorrect. Instead of trying to fix that, assume that most windows have a visible shaped region and dump the optimization. Not worth keeping this around either, rebased.
Created attachment 191775 [details] [review] Track client window's XShape directly Because we're no longer pushing the visible region back up to the X server, we don't want to track the frame window, we want to track the client window.
Created attachment 191776 [details] [review] Replace public MetaFrameGeometry with MetaFrameBorders There were actually *two* MetaFrameGeometry structs: one in theme-private.h, one in frame.h. The latter public struct was populated by a mix of (void*) casting and int pointers, usually pulling directly from the data in the private struct. Remove the public struct, replace it with MetaFrameBorders and scrap all the pointer hacks to populate it, instead relying on both structs being used in common code. This commit should be relatively straightforward, and it should not do any tricky logic at all, just a sophisticated find and replace. This should be relatively straightforward now that I'm not doing anything tricky.
Created attachment 191777 [details] [review] MetaFrameBorders: Add meta_frame_borders_clear Just a quick little commit to help clean things up for when we add invisible borders.
Created attachment 191778 [details] [review] MetaFrameBorders: Add invisible borders This just adds the invisible border field and populates it with data but doesn't use it in any way.
Created attachment 191779 [details] [review] MetaWindow: Repurpose get_outer_rect and add get_input_rect get_outer_rect now returns the visible region, and a new get_input_rect method returns the boundaries of the full frame, including the possible invisible regions. When undecorated, both do the samething. Reattaching to show patch order.
Created attachment 191780 [details] [review] MetaWindow: Compensate for invisible border changes Nothing too major in here.
Created attachment 191781 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and start compensating for invisible borders in all of the math. ah yes, the "yak shaving" commit. This is still big and ugly, but splitting it apart was hard. Leaving as is. Any other suggestions?
Created attachment 191782 [details] [review] testing: Use simple values for debugging. Simplified some of the code after rebase.
Created attachment 191783 [details] [review] MetaWindowActor: Compensate for invisible borders
Created attachment 191784 [details] [review] MetaWindow: Fix MAXIMIZED macros When we originally place a window, we don't set the "maximized" hint, even though it should be placed maximized. Effectively, this means that windows placed maximized get the wrong frame state, causing ugly rendering errors.
Review of attachment 191771 [details] [review]: Code looks like a correct deletion of a bunch of code. Commit message should be something like: Stop shaping the frame window In preparation for switching to handling the output shape purely by what we paint, stop applying a shape to the frame of the window. Even when we restore handling the output shape, this will change the behavior with respect to input; transparent areas between the frame and the contents will stop clicks rather than passing them through, but that is arguably at least as expected considering how that we decorate shaped windows with a frame all around. If we wanted to keep the pass-through behavior but avoid the lag caused by changing the frame shape we could switch to only shaping the /input/ shape of the frame, but not that inclined to keep a bunch of code (and a bunch of frame geometry calculations that you'd have to fix) to preserve some behavioral detail that could reasonably be either way. [ This is a big step further in making Mutter only a compositor and making the non-composited code paths not work. But we decided a long time ago we wanted to do that - it just hasn't been necessary much before ]
Created attachment 192045 [details] [review] MetaWindow: Compensate for invisible border changes Just attaching this before you get any farther. Goes in the same place the old patch did. This prevents a too-big bounding rectangle, which is always a good thing. The problem is that the calculations here make an unmaximized and maximized window have the same width/height in a very prominent case: maximize a window, restart mutter, unmaximize a window. There's no saved_rect for the window to point to, so it ends up thinking the bounding rectangle hasn't changed. I removed the check, but if you think there's a better check we can be using, go for it.
Review of attachment 191772 [details] [review]: So this is untabification? as far as I can tell, the actual indentation is fine. While I prefer spaces to tabs, I don't generally think it's worth patches just to change... we're very clear that the only legitimate setting to view or edit GNOME source code is with 8 space tabs. But if you want to do this: - Comment needs to be specific that you are untabifying. - It should remove all tabs in the entire file - It should add the appropriate modeline to the top so they don't come back (see meta-texture-tower.c for what it is)
Created attachment 192054 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects Updated and fixed.
Review of attachment 191773 [details] [review]: Mostly right, various comments ::: src/compositor/meta-shaped-texture.c @@ +221,3 @@ + if (priv->shape_region) + n_rects = cairo_region_num_rectangles (priv->shape_region); + else priv->shape_region == NULL can't happen - the code is set up so that unclipped implies no mask texture. @@ +226,1 @@ /* Cut out a hole for each rectangle */ This comment is just (existingly) wrong. It's not cutting out a hole, it's positively drawing the shape. @@ +337,3 @@ + if (priv->shape_region == NULL || + cairo_region_num_rectangles (priv->shape_region) < 1) It was a bit odd with rectangles, but it's definitely wrong with regions - an empty region doens't mean "no clip" it means "everything clipped away". I don't think it's worth special casing it, but if you were special casing it, you'd want to just return immediately. Use shape_region == NULL to mean "not clipped". @@ +441,3 @@ /* If there are no rectangles then use the regular pick */ + if (priv->shape_region == NULL || + cairo_region_num_rectangles (priv->shape_region) < 1) same here @@ +578,3 @@ + if (region != NULL) + { + priv->shape_region = region; You need to reference. Functions that just assume ownership of the arguments passed in are evil. And even referencing it's probably good to add a doc comment that makes it clear that this function takes a reference to region rather than copying it so it can't be modified subsequently. ::: src/compositor/meta-window-actor.c @@ +2131,3 @@ + + meta_frame_calc_borders (priv->window->frame, &borders); + summed = meta_frame_borders_sum (&borders); This won't compile and hence you break bisecting Mutter. Since this patch precedes the frame borders stuff, it needs to not use it. @@ +2157,3 @@ + rects[i].y + client_area.y, + rects[i].width, + rects[i].height }; c99'ism @@ +2158,3 @@ + rects[i].width, + rects[i].height }; + cairo_region_union_rectangle (region, &rect); Think it's slightly better to allocate an array of cairo_rectangle_int_t and use cairo_region_create_rectangles() @@ +2166,3 @@ + /* refcount from 1 to 2 */ + cairo_region_reference (region); Though you document the reference count, you don't document why unpaired reference/dereference is necessary. Instead of trying to do that, just say no and make this work normally - if functions take a region as an argument and wants to save it, it needs to take a reference, etc.
Review of attachment 191784 [details] [review]: Random insufficiently motivated change. Maybe you can split this out into it's own bug so you can actually describe a sequence of events where this change is needed?
Review of attachment 191774 [details] [review]: > Because we're no longer pushing the visible region back up to the X server, > is_shaped becomes incorrect. Instead of trying to fix that, assume that most > windows have a visible shaped region and dump the optimization. Blech, blah, you've given me random hunks of stuff that don't make any independent sense. Please: A) Make "Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects" do _only_ that, without changing any behavior (currently, it switches to computing the shape in a different way, so leaves in an inconsistent state, because then we aren't updating stuff at the right time.) B) Merge the remainder of that with this patch and the next patch - the basic idea of that patch would be something like "Calculate output shape directly, rather than using the shape of the frame window" Just a couple of drive-by comments here, will re-review once you've done the above. ::: src/compositor/meta-window-actor.c @@ +114,2 @@ guint no_more_x_calls : 1; + guint padding1 : 1; There's no need for padding in a private structure! And in a public structure, you'd have to add the padding where you removed a field not somewhere else. @@ +1663,3 @@ + /* Invisible clipping and corners rely on the shape that we set, + * so we need to update it after a sync. */ + meta_window_actor_update_shape (self); This comment is actively harmful. "Invisible clipping" - what's that? "corners" - how do they depend on the shape we set? "shape we set" - what shape is this? this function is a function called when the window has been resized to a new size. "we need to update it" - if "it" refers to shape we set, then should we be updating the stuff that depends upon it, not it? "a sync" - what's a sync? The comments that were here before weren't just here to look decorative, they were documenting non-obvious behavior about what depending on priv->bounding_region in different cases, and what we needed to do. If there is new non-obvious stuff to document, then document _that_. Do not add comments just because you feel guilty about removing comments and feel a need to add a similar amount of text.
Review of attachment 191775 [details] [review]: This patch looks plausible, but needs to be merged with the proceeding patch as described there.
Review of attachment 191776 [details] [review]: Good except for one comment comment ::: src/meta/common.h @@ +29,3 @@ #include <X11/Xlib.h> #include <glib.h> +#include <gtk/gtk.h> Mmm, could just define MetaBorder that is the same thing, and avoid the issue of introducing GTK+ includes across all the code, but I can't think of any particular harm in accidentally using GTK+ inside the core/ or compositor/ code, so I guess this is fine. @@ +307,3 @@ +struct _MetaFrameBorders +{ + /* What's shown on-screen? */ ? ? Beyond that, I think it's probably better to have a comment like: /* The border is made up of two pieces - an inner visible portion * and an outer portion that is invisible but responds to events */ rather than to try and give one line descriptions of 'visible' and 'invisible'. I'm fine if the comment is introduced as above in this patch, or you can add '(invisible border is yet to be added)' to the comment here and then delete it when adding the invisible border.
Review of attachment 191777 [details] [review]: ::: src/meta/common.h @@ +311,3 @@ }; +void +meta_frame_borders_clear (MetaFrameBorders *self); blank line before it, all on one line. This is a bit of a obscure name, so put a comment before it in the header /* sets all dimensions to zero */ (or doc-comment in the C, either way) ::: src/ui/preview-widget.c @@ +95,3 @@ META_FRAME_ALLOWS_MOVE; + + meta_frame_borders_clear (&preview->borders); This was: preview->borders.visible.left = -1 and the difference between -1 and 0 is significant, since we have: if (preview->borders.visible.top < 0) I think it's cleanest to have a borders_cached : 1 boolean in MetaPreview and then you can just drop the meta_frame_borders_clear() call here and in clear_cache().
Review of attachment 191778 [details] [review]: ::: src/meta/common.h @@ +315,3 @@ + +GtkBorder +meta_frame_borders_sum (MetaFrameBorders *self); void meta_frame_borders_get_total (MetaFrameBorders *self, GtkBorder *total); [ I have no real argument against value struct returns other than consistency ] Or consider just having MetaFrameBorders have a total field - causes 50% more copying data around, of course, but will result in more readable code. ::: src/ui/theme.c @@ +433,3 @@ + /* XXX: get from gconf/gsettings */ + int draggable_borders = 10; I've OK'ed C99 initializers, but let's still avoid mixed declarations and code. @@ +443,3 @@ + + /* No invisible borders on top. */ + borders->invisible.top = 0; seems to me it would be simpler to just keep the above the same - if someone is crazy enough to do a title-less theme, then you'd want invisible borders above.
Review of attachment 191779 [details] [review]: Looks fine, except for the doc comments could be clarified. Some suggestions. ::: src/core/window.c @@ +5128,3 @@ + * + * Gets the rectangle that bounds @window and, if decorated, its decorations. + * This includes the invisible input region. Invisible input region is pretty confusing. I'd say something like: Gets the total rectangle bounding @window that is responsive to mouse events. This includes both the visible portion of the border and (if present) any invisible area that we make responsive to mouse clicks to allow convenient border dragging. @@ +5146,3 @@ * * Gets the rectangle that bounds @window and, if decorated, its decorations. + * This does not include the invisible input region. * This includes only what is visible; it doesn't include any * extra reactive area we add to the edges of windows. */
(In reply to comment #100) > @@ +443,3 @@ > + > + /* No invisible borders on top. */ > + borders->invisible.top = 0; > > seems to me it would be simpler to just keep the above the same - if someone is > crazy enough to do a title-less theme, then you'd want invisible borders above. Only the border on top of the title bar can be used for resizing, so in my opinion treating it the same as other borders makes sense for "normal themes" (e.g. with title bar) too.
Review of attachment 191783 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +1351,1 @@ if (priv->attrs.width != window_rect.width || Can you do a pre-patch to this series that drops attrs (and the public property and the GType) - you can get the visual and override redirect status from the MetaWindow and input only windows are impossible, so you'll just need to have some width/height variables to replace this usage. I'd rather not have a xattrs structure rather than change it to have x/y/width/height corresponding to the input rectangle. @@ +1653,3 @@ + + bounding_rectangle.x = borders.invisible.left; + bounding_rectangle.y = borders.invisible.top; The width/height here are the width and height of the entire window, so including the input area, so this is inconsistent. One way or the other is a bug. @@ +1665,3 @@ + if ((old_bounding_rectangle.width == width-bounding_rectangle.x) && + (old_bounding_rectangle.height == height-bounding_rectangle.y)) spaces around binary operators, but don't see how the math makes any sense
Review of attachment 192054 [details] [review]: This was just a rebase on top of whitespace changes, see https://bugzilla.gnome.org/review?bug=644930&attachment=191773 and my request to merge part of this with other patches
Review of attachment 191782 [details] [review]: Make sure that you don't commit this
Created attachment 192257 [details] [review] MetaWindowActor: Compensate for invisible borders In comment #91 I meant to attach *this* patch.
Review of attachment 192257 [details] [review]: OK, looks better ::: src/compositor/meta-window-actor.c @@ +1345,2 @@ if (priv->attrs.width != window_rect.width || priv->attrs.height != window_rect.height) Same comment as before about getting rid of priv->attrs to avoid changing its meaning. @@ +1650,3 @@ + + width -= borders.invisible.left + borders.invisible.right; + height -= borders.invisible.top + borders.invisible.bottom; Need to change the comment then about what bounding_region is: /* A rectangular region with the unshaped extends of the window * texture */ is not correct
Review of attachment 192045 [details] [review]: Think this patch is a strong argument for having MetaFrameBorders->total. If you don't like that, please change the 'summed' variables in this patch to total_border, since it's not very readable like this. (long functions using a variable 'summed' far from where it was originally defined)
Review of attachment 191781 [details] [review]: A bit unhappy about how many mistakes I found in theme.c - the point of code review is *not* to find this sort of mistake. Some hints below how you probably could reorganize things so that you can check your work more effectively. ::: src/ui/frames.c @@ +874,3 @@ + +/* frame region surrounds the frame window -- that is, it subtracts + * ONLY the invisible borders. The frame window includes the invisible borders. Best to call this I think get_visible_rect() (matching visible). Could also call it get_outer_rect() to match the MetaWindow method but that's inherently a rather confusing name. @@ +2123,3 @@ + /* left */ + pixels->piece[1].rect.x = borders.invisible.left; + pixels->piece[1].rect.y = summed.top; I think it's actually pretty confusing that the computations here mix borders.invisible, borders.visible and this summed variable which happens to be the sum of the two. It would be clearer if you just had the addition inline @@ +2623,1 @@ return META_FRAME_CONTROL_CLIENT_AREA; Do Not like allocating a CairoRegion just to compute point-in-rectangle, and you leak it in the return case ::: src/ui/theme.c @@ +848,3 @@ /* center buttons vertically */ + button_y = (borders.visible.top - + (button_height + layout->button_border.top + layout->button_border.bottom)) / 2 + layout->button_border.top + borders.invisible.top; Looks like this is off by a borders.invisible.top (which implies that you need to test with an invisible.top != 0) @@ +967,2 @@ fgeom->top_left_corner_rounded_radius = layout->top_left_corner_rounded_radius; + if (summed.top + summed.right >= min_size_for_rounding) use of summed here looks wrong to me. summed can't have anything to do with whether we round or not, since we round the visible part. @@ +972,2 @@ fgeom->bottom_left_corner_rounded_radius = layout->bottom_left_corner_rounded_radius; + if (summed.top + summed.right >= min_size_for_rounding) And here @@ +3577,3 @@ + env->right_width = summed.right; + env->top_height = summed.top; + env->bottom_height = summed.bottom; The use of summed here looks wrong to me. The invisible borders need to be invisible to the theme. @@ +4638,3 @@ + GtkBorder summed; + + borders = fgeom->borders; If you are just abbreviating, you really should do borders = &geom->borders @@ +4648,3 @@ + titlebar_rect.x = borders.invisible.left; + titlebar_rect.y = borders.invisible.top; + titlebar_rect.width = fgeom->width - summed.left - summed.right; This is inconsistent with the old code which had: titlebar_rect.width = fgeom->width; so including the visible borders; consider redoing the computations of all the other rectangles to use full_rect - I think it will be more readable and make this patch much more verifiable. The same approach might be good for frame_layout_calc_geometry @@ +4685,2 @@ bottom_edge.width = fgeom->width; + bottom_edge.height = summed.bottom; Use of summed looks wrong
Created attachment 192279 [details] [review] Stop shaping the frame window In preparation for switching to handling the output shape purely by what we paint, stop applying a shape to the frame of the window. Even when we restore handling the output shape, this will change the behavior with respect to input; transparent areas between the frame and the contents will stop clicks rather than passing them through, but that is arguably at least as expected considering how that we decorate shaped windows with a frame all around.
Created attachment 192280 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects OK, this should be just a clean port.
Created attachment 192281 [details] [review] Track the shape of the client window directly Since we're not setting the frame's output shape any more, it doesn't make sense to calculate the output shape based on the frame window. Instead, track the client window directly and calculate the output shape based on that. Should be pretty consistent I hope.
Created attachment 192282 [details] [review] Replace public MetaFrameGeometry with MetaFrameBorders There were actually *two* MetaFrameGeometry structs: one in theme-private.h, one in frame.h. The latter public struct was populated by a mix of (void*) casting and int pointers, usually pulling directly from the data in the private struct. Remove the public struct, replace it with MetaFrameBorders and scrap all the pointer hacks to populate it, instead relying on both structs being used in common code. This commit should be relatively straightforward, and it should not do any tricky logic at all, just a sophisticated find and replace.
Created attachment 192283 [details] [review] MetaFrameBorders: Add meta_frame_borders_clear Just a quick little commit to help clean things up for when we add invisible borders. I included the preview_widget changes inline, thinking they were minimal. I will split them out if you want.
Created attachment 192284 [details] [review] Add draggable_borders preference Obsoletes the testing patch. This is a new patch. Unfortunately, I can't get gconf to play along with jhbuild: gconf-editor says that 'draggable-borders' isn't in a schema, even though $JHBUILD_INSTALL/etc/gconf/schemas/mutter.schemas says differently. It *does* work if you add the key manually, but the default doesn't. Due to the difficulties in making the preference "live" (need to redecorate all windows, not sure it's worth the performance hit and new positioning logic it would require), I've opted not to do that. Restart after you change the key.
Created attachment 192285 [details] [review] MetaFrameBorders: Add invisible borders This just adds the invisible border field and populates it with data but doesn't use it in any way. This uses the new preference.
Created attachment 192286 [details] [review] MetaWindow: Repurpose get_outer_rect and add get_input_rect get_outer_rect now returns the visible region, and a new get_input_rect method returns the boundaries of the full frame, including the possible invisible regions. When undecorated, both do the samething.
Created attachment 192287 [details] [review] MetaWindow: Compensate for invisible border changes
Created attachment 192288 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and start compensating for invisible borders in all of the math. (In reply to comment #110) > Review of attachment 191781 [details] [review]: > > A bit unhappy about how many mistakes I found in theme.c - the point of code > review is *not* to find this sort of mistake. Some hints below how you probably > could reorganize things so that you can check your work more effectively. > @@ +2123,3 @@ > + /* left */ > + pixels->piece[1].rect.x = borders.invisible.left; > + pixels->piece[1].rect.y = summed.top; > > I think it's actually pretty confusing that the computations here mix > borders.invisible, borders.visible and this summed variable which happens to be > the sum of the two. It would be clearer if you just had the addition inline With the s/summed/border.total/g changes, do you still think inline math would be clearer? Knowing the intent of the final shape I think it's perfectly readable, even if dense. > @@ +2623,1 @@ > return META_FRAME_CONTROL_CLIENT_AREA; > > Do Not like allocating a CairoRegion just to compute point-in-rectangle, and > you leak it in the return case I had used the region for another part of the code, but whatever it was, it's gone now. Changed to a rectangle. Additionally, because POINT_IN_RECT is a macro that just accesses the x,y,width,height field names, it will work for cairo_rectangle_int_t as well as MetaRectangle, which is a neat trick I didn't think about... even if it was already used by the existing code. > ::: src/ui/theme.c > @@ +848,3 @@ > /* center buttons vertically */ > + button_y = (borders.visible.top - > + (button_height + layout->button_border.top + > layout->button_border.bottom)) / 2 + layout->button_border.top + > borders.invisible.top; > > Looks like this is off by a borders.invisible.top (which implies that you need > to test with an invisible.top != 0) I do test with an invisible.top != 0 (this is what the hardcoded testing commit is for) and the math seems correct to me. borders.visible.top contains the *entire* titlebar height, so we center in there, and then offset by the top invisible border to get the final y.
Created attachment 192289 [details] [review] MetaWindowActor: Remove priv->attrs It was deprecated and most of the information was in the MetaWindow anyway.
Created attachment 192290 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects Fix unintentional whitespace change.
Review of attachment 192289 [details] [review]: Mostly looks good ::: src/compositor/meta-window-actor.c @@ +1266,3 @@ + if (priv->window->rect.width != window_rect.width || + priv->window->rect.height != window_rect.height) Pretty sure this is wrong - we need to be checking against the last size we got. (It looks like this will always be a mismatch for a decorated window, in fact, since you are comparing outer rect with priv->window->rect. So you need to add priv->last_width priv->last_height
Review of attachment 192279 [details] [review]: Looks good
Created attachment 192323 [details] [review] MetaFrameBorders: Add invisible borders This just adds the invisible border field and populates it with data but doesn't use it in any way. Populate the draggable_borders on the top better. Not sure how else to make it more "correct" in correspondence with the code in get_control.
Created attachment 192324 [details] [review] MetaWindowActor: Remove priv->attrs It was deprecated and most of the information was in the MetaWindow anyway. Fix an accidental fixup into the wrong patch.
Created attachment 192325 [details] [review] MetaWindowActor: Compensate for invisible borders
Created attachment 192326 [details] [review] MetaWindow: Fix MAXIMIZED macros When we originally place a window, we don't set the "maximized" hint, even though it should be placed maximized. Effectively, this means that windows placed maximized get the wrong frame state, causing ugly rendering errors. Not sure how to convince you this patch is sufficiently motivated other than to try it out yourself: maximize a window, restart and "pull it down".
Review of attachment 192326 [details] [review]: No, really, I want this in a separate bug, with the sort of justification you provided in IRC in the bug.
Review of attachment 192290 [details] [review]: Couple of issues ::: src/compositor/meta-window-actor.c @@ +1706,2 @@ + /* region must be non-null */ + priv->shape_region = region; You leak the old priv->shape_region,it looks like @@ +2093,3 @@ return; + meta_shaped_texture_set_shape_region (META_SHAPED_TEXTURE (priv->actor), NULL); It's weird to have this call here, since you always set the shape region (to null or a shape) below
Review of attachment 192281 [details] [review]: This looks good to me. At some point, I'd really like to get back the optimization of not having a mask texture for: - Undecorated windows - Decorated windows without invisible borders (I don't think maximized windows should have invisible borders, because they'll leak off onto monitors and cause problems) But this looks good for now - let's get the correctness done first, then worry about edge matters separately.
Review of attachment 192282 [details] [review]: LGTM
Review of attachment 192283 [details] [review]: Find with including the MetaPreview change here, but do mention it in the commit message Couple minor things beyond that ::: src/meta/common.h @@ +315,3 @@ +/* sets all dimensions to zero */ +void +meta_frame_borders_clear (MetaFrameBorders *self); All on one line - function definitions have the return type on the same line as the function name. ::: src/ui/preview-widget.c @@ +175,3 @@ preview->flags, &preview->borders); + preview->borders_cached = TRUE; In the wrong place - should set this also if preview theme is NULL
Review of attachment 192286 [details] [review]: Almost ::: src/core/window.c @@ +5128,3 @@ + * + * Gets the rectangle that bounds @window that is responsive to mouse events. + * This includes decorations, the visible portion of its border, and (if decorations are the visible portion of the border - if that's what you meant rather than a list of three things, you could just use -'s This includes decorations - the visible portion of its borders - and ... but many possible fixes.
Review of attachment 192324 [details] [review]: Review from https://bugzilla.gnome.org/review?bug=644930&attachment=192289 still applies here
Review of attachment 192323 [details] [review]: Looks good to me
Review of attachment 192287 [details] [review]: Looks good
(In reply to comment #120) > > @@ +2123,3 @@ > > + /* left */ > > + pixels->piece[1].rect.x = borders.invisible.left; > > + pixels->piece[1].rect.y = summed.top; > > > > I think it's actually pretty confusing that the computations here mix > > borders.invisible, borders.visible and this summed variable which happens to be > > the sum of the two. It would be clearer if you just had the addition inline > > With the s/summed/border.total/g changes, do you still think inline math would > be clearer? Knowing the intent of the final shape I think it's perfectly > readable, even if dense. It looked fine to me with borders.total - no trouble understanding what was going on. > > ::: src/ui/theme.c > > @@ +848,3 @@ > > /* center buttons vertically */ > > + button_y = (borders.visible.top - > > + (button_height + layout->button_border.top + > > layout->button_border.bottom)) / 2 + layout->button_border.top + > > borders.invisible.top; > > > > Looks like this is off by a borders.invisible.top (which implies that you need > > to test with an invisible.top != 0) > > I do test with an invisible.top != 0 (this is what the hardcoded testing commit > is for) and the math seems correct to me. > > borders.visible.top contains the *entire* titlebar height, so we center in > there, and then offset by the top invisible border to get the final y. Sorry, the end of th elong line got truncated in splitter so I missed the trailing + borders.invisible.top, it does look OK
Review of attachment 192288 [details] [review]: Caught one mistake, otherwise looks good to commit ::: src/ui/theme.c @@ +4643,3 @@ + titlebar_rect.x = visible_rect.x; + titlebar_rect.y = visible_rect.y; + titlebar_rect.width = titlebar_rect.width; Typo - should be = visible_rect.width
Review of attachment 192325 [details] [review]: Needs a rebase for the fixup of the ->attrs removal patch ::: src/compositor/meta-window-actor.c @@ +67,3 @@ /* If the window is shaped, a region that matches the shape */ cairo_region_t *shape_region; + /* A rectangular region with the visible extends of the window */ extents
Review of attachment 192284 [details] [review]: You should be able to handle changes very easily by in main.c: case META_PREF_THEME: + case META_PREF_DRAGGABLE_BORDERS: meta_ui_set_current_theme (meta_prefs_get_theme (), FALSE); meta_display_retheme_all (); break; ::: src/mutter.schemas.in @@ +71,3 @@ + + <schema> + <key>/schemas/apps/mutter/general/draggable_borders</key> draggable_borders sounds like a boolean. Suggest draggable_border_width @@ +79,3 @@ + <short>Draggable borders</short> + <long> + Determines the amount of draggable borders. You can do better than this :-)
Created attachment 192400 [details] [review] Replace public MetaFrameGeometry with MetaFrameBorders There were actually *two* MetaFrameGeometry structs: one in theme-private.h, one in frame.h. The latter public struct was populated by a mix of (void*) casting and int pointers, usually pulling directly from the data in the private struct. Remove the public struct, replace it with MetaFrameBorders and scrap all the pointer hacks to populate it, instead relying on both structs being used in common code. This commit should be relatively straightforward, and it should not do any tricky logic at all, just a sophisticated find and replace. Minor whitespace fixes
Created attachment 192401 [details] [review] MetaFrameBorders: Add meta_frame_borders_clear Just a quick little commit to help clean things up for when we add invisible borders. Additionally, do a little housekeeping in preview-widget as well. Replace = 0; in one more case, hopefully making the flow a bit easier to follow.
Created attachment 192402 [details] [review] prefs: Add draggable_border_width preference
Created attachment 192403 [details] [review] MetaFrameBorders: Add invisible borders This just adds the invisible border field and populates it with data but doesn't use it in any way. Hopefully the maximized case is a bit easier to follow.
Comment on attachment 192326 [details] [review] MetaWindow: Fix MAXIMIZED macros Whatever value I saw in this patch is missing now. I assume it's the same allocation bug that hit other things. Sorry for wasting your time by pushing this patch. I'll eat my foot now.
Created attachment 192404 [details] [review] MetaWindow: Repurpose get_outer_rect and add get_input_rect get_outer_rect now returns the visible region, and a new get_input_rect method returns the boundaries of the full frame, including the possible invisible regions. When undecorated, both do the samething. > decorations are the visible portion of the border - if that's what you meant > rather than a list of three things, you could just use -'s > > This includes decorations - the visible portion of its borders - and ... > > but many possible fixes. I did indeed mean for adjective clause, which is usually punctuated by commas, but that's a bit confusing. Changed to hyphens instead.
Created attachment 192405 [details] [review] MetaWindow: Compensate for invisible border changes
Created attachment 192406 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and start compensating for invisible borders in all of the math. Oops.
Created attachment 192407 [details] [review] MetaWindowActor: Remove priv->attrs It was deprecated and most of the information was in the MetaWindow anyway.
Created attachment 192408 [details] [review] MetaWindowActor: Compensate for invisible borders An existing typo, but fixed anyway.
Created attachment 192409 [details] [review] Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects OK, hopefully this is a bit better.
Review of attachment 192400 [details] [review]: marking acn based on being whitespace fixes on top of previous acn
Review of attachment 192401 [details] [review]: Good enough, though "do a little housekeeping in preview-widget as well" wasn't really what I meant by "do mention it in the commit message" - "avoid using magic border values as flags" or something was what I meant.
Review of attachment 192402 [details] [review]: ::: src/mutter.schemas.in @@ +79,3 @@ + <short>Draggable border width</short> + <long> + The amount of total draggable borders. If the theme's visible Maybe better as 'The minimum amount of total draggable borders, in pixels."
Review of attachment 192403 [details] [review]: Looks good
Review of attachment 192404 [details] [review]: Looks good
Review of attachment 192405 [details] [review]: Marking acn based on previous acn
Review of attachment 192406 [details] [review]: Looks good
Review of attachment 192407 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +1268,1 @@ meta_window_get_outer_rect (priv->window, &window_rect); Hmm, there's something a bit nasty here (though incredibly corner case), we actually care about the size of both the input rect: this is the size of the window we're creating the TFP for, and if we don't recreate the pixmap when the window when the window size changes, we'll freeze with an old contents of the window output rect: this ends up being our bounding rect But note that this function doesn't even get called if only the outer rect changes (see the code in window.c - it only calls meta_compositor_sync_window_geometry if the client or total frame size changes), so I'm going to suggest that you test for an input rect change, rather than an outer rect change and then we'll just hope that the case where the outer rect changes without the input rect changing is vanishingly rare. @@ +1269,3 @@ + if (priv->last_width != window_rect.width || + priv->last_height != window_rect.height) You don't actually update last_width/last_height, so this will never be false
(In reply to comment #160) > Review of attachment 192407 [details] [review]: > > ::: src/compositor/meta-window-actor.c > @@ +1268,1 @@ > meta_window_get_outer_rect (priv->window, &window_rect); > > Hmm, there's something a bit nasty here (though incredibly corner case), we > actually care about the size of both the > > input rect: this is the size of the window we're creating the TFP for, and if > we don't recreate the pixmap when the window when the window size changes, > we'll freeze with an old contents of the window > > output rect: this ends up being our bounding rect > > But note that this function doesn't even get called if only the outer rect > changes (see the code in window.c - it only calls > meta_compositor_sync_window_geometry if the client or total frame size > changes), so I'm going to suggest that you test for an input rect change, > rather than an outer rect change and then we'll just hope that the case where > the outer rect changes without the input rect changing is vanishingly rare. I *do* change it to an input_rect... in the next patch for MWA compensation. > @@ +1269,3 @@ > > + if (priv->last_width != window_rect.width || > + priv->last_height != window_rect.height) > > You don't actually update last_width/last_height, so this will never be false
Review of attachment 192408 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +1265,3 @@ MetaRectangle window_rect; + meta_window_get_input_rect (priv->window, &window_rect); OK, ignore that part of my comment on the last patch - this is OK here @@ -1552,1 @@ - if (priv->bounding_region != NULL) I don't see why you had to remove this short-circuit, which is likely still useful to the avoid size-changed signal, etc - just move it after you compute the new bounding rectangle.
Created attachment 192415 [details] [review] MetaWindowActor: Remove priv->attrs It was deprecated and most of the information was in the MetaWindow anyway. I swore I remember typing those lines. Probably a bad rebase.
Review of attachment 192409 [details] [review]: Looks good
Review of attachment 192415 [details] [review]: Looks good
(In reply to comment #162) > Review of attachment 192408 [details] [review]: > > ::: src/compositor/meta-window-actor.c > @@ +1265,3 @@ > MetaRectangle window_rect; > > + meta_window_get_input_rect (priv->window, &window_rect); > > OK, ignore that part of my comment on the last patch - this is OK here > > @@ -1552,1 @@ > - if (priv->bounding_region != NULL) > > I don't see why you had to remove this short-circuit, which is likely still > useful to the avoid size-changed signal, etc - just move it after you compute > the new bounding rectangle. Because of a certain edge case where the frame geometry could have changed but the bounding rectangle would be the same, because I subtract`the invisible borders. We won't fire a SIZE_CHANGED signal when I change the invisible border value, for example by pref, or in some cases, maximized vs. unmaximized. This means that it looks like your windows are "cut off". I could fix this by saving the old MetaFrameBorders to compare against as well, or saving the unmodified region and making sure to check that too, but I just took the short circuit out for simplicity.
Created attachment 192472 [details] [review] ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder ... and start compensating for invisible borders in all of the math. Fix up get_control to work a bit better, especially around the top-right border.
Created attachment 192473 [details] [review] MetaFrameBorders: Add invisible borders This just adds the invisible border field and populates it with data but doesn't use it in any way. Calculate the top border a little differently. Realistically, this is the best I can do.
Created attachment 192474 [details] [review] MetaWindowActor: Compensate for invisible borders Add back short-circuit, but it's a lot more involved.
Created attachment 193128 [details] [review] Stop shaping the frame window In preparation for switching to handling the output shape purely by what we paint, stop applying a shape to the frame of the window. Even when we restore handling the output shape, this will change the behavior with respect to input; transparent areas between the frame and the contents will stop clicks rather than passing them through, but that is arguably at least as expected considering how that we decorate shaped windows with a frame all around. Remove get_client_region: otherwise we get an unused function error while bisecting.
Created attachment 193129 [details] [review] Track the shape of the client window directly Since we're not setting the frame's output shape any more, it doesn't make sense to calculate the output shape based on the frame window. Instead, track the client window directly and calculate the output shape based on that. Remove an unused variable and an accidental overwrite of the shpae region.
Created attachment 193130 [details] [review] Replace public MetaFrameGeometry with MetaFrameBorders There were actually *two* MetaFrameGeometry structs: one in theme-private.h, one in frame.h. The latter public struct was populated by a mix of (void*) casting and int pointers, usually pulling directly from the data in the private struct. Remove the public struct, replace it with MetaFrameBorders and scrap all the pointer hacks to populate it, instead relying on both structs being used in common code. This commit should be relatively straightforward, and it should not do any tricky logic at all, just a sophisticated find and replace. Another accidental bisect breaker: @@ -5585,10 +5585,10 @@ meta_theme_get_frame_borders (MetaTheme *theme, style = theme_get_style (theme, type, flags); - borders.visible.top = 0; - borders.visible.left = 0; - borders.visible.right = 0; - borders.visible.bottom = 0; + borders->visible.top = 0; + borders->visible.left = 0; + borders->visible.right = 0; + borders->visible.bottom = 0; /* Parser is not supposed to allow this currently */ if (style == NULL)
Created attachment 193131 [details] [review] MetaFrameBorders: Add invisible borders This just adds the invisible border field and populates it with data but doesn't use it in any way. Fix an accidental copy-paste error that broke full-screen windows (and probably other things): @@ -307,7 +307,7 @@ meta_frame_borders_clear (MetaFrameBorders *self) self->visible.top = self->invisible.top = self->total.top = 0; self->visible.bottom = self->invisible.bottom = self->total.bottom = 0; self->visible.left = self->invisible.left = self->total.left = 0; - self->visible.right = self->invisible.bottom = self->total.right = 0; + self->visible.right = self->invisible.right = self->total.right = 0; } -- This should be it. For all the other changes (merge conflicts, etc.) I can re-attach them here, or you can browse my github branch: https://github.com/magcius/mutter/tree/wip/invisible-borders
Review of attachment 193128 [details] [review]: OK
Review of attachment 193130 [details] [review]: OK
Review of attachment 193131 [details] [review]: OK
Review of attachment 193129 [details] [review]: OK
Review of attachment 192472 [details] [review]: OK
Review of attachment 192474 [details] [review]: I have to say, I don't understand this - you have a function called update_bounding_region()... it doesn't do anything obvious but update the bounding_region. So the changing of it to not short-circuit some times when the bounding region doesn't change really doesn't make conceptual sense. So there at least needs to be some rename here from update_bounding_region() to update_bounding_region_and_borders() or something. Because of a certain edge case where the frame geometry could have changed but the bounding rectangle would be the same, because I subtract`the invisible borders. We won't fire a SIZE_CHANGED signal when I change the invisible border value, for example by pref, or in some cases, maximized vs. unmaximized. This means that it looks like your windows are "cut off". So, what does the size-changed signal correspond to? when is it supposed to be emitted?
Created attachment 193474 [details] [review] MetaWindowActor: Compensate for invisible borders
(In reply to comment #179) > Review of attachment 192474 [details] [review]: > > I have to say, I don't understand this - you have a function called > update_bounding_region()... it doesn't do anything obvious but update the > bounding_region. So the changing of it to not short-circuit some times when the > bounding region doesn't change really doesn't make conceptual sense. > > So there at least needs to be some rename here from update_bounding_region() to > update_bounding_region_and_borders() or something. Done. > Because of a certain edge case where the frame geometry could have changed but > the bounding rectangle would be the same, because I subtract`the invisible > borders. > > We won't fire a SIZE_CHANGED signal when I change the invisible border value, > for example by pref, or in some cases, maximized vs. unmaximized. This means > that it looks like your windows are "cut off". > > So, what does the size-changed signal correspond to? when is it supposed to be > emitted? uh, I have no idea why I mentioned SIZE_CHANGED: we only use it in workspace.js to reposition windows if the sizes are changed, and windows should be repositioned when their visible sizes changed. The real issue here is that we need to update the mask texture when the borders don't match, as the visible region could still be the same (but incorrectly aligned compared to the frame X window).
Review of attachment 193474 [details] [review]: Still have some nagging feeling that the structure could be done better :-), but let's go with this. This patchset is good to merge as far as I'm concerned.
Attachment 192323 [details] pushed as a1a2527 - MetaFrameBorders: Add invisible borders Attachment 192401 [details] pushed as ce9c7a2 - MetaFrameBorders: Add meta_frame_borders_clear Attachment 192402 [details] pushed as 6f58823 - prefs: Add draggable_border_width preference Attachment 192404 [details] pushed as a133d8b - MetaWindow: Repurpose get_outer_rect and add get_input_rect Attachment 192405 [details] pushed as be4ef9b - MetaWindow: Compensate for invisible border changes Attachment 192409 [details] pushed as 65e1b41 - Port MetaShapedTexture/MetaWindowActor to use cairo regions instead of XRects Attachment 192415 [details] pushed as e0e7899 - MetaWindowActor: Remove priv->attrs Attachment 192472 [details] pushed as eeb2efe - ui: Replace inline borders in MetaFrameGeometry with MetaFrameBorder Attachment 193128 [details] pushed as 183bcd6 - Stop shaping the frame window Attachment 193129 [details] pushed as 7e0a56f - Track the shape of the client window directly Attachment 193130 [details] pushed as e0fb83c - Replace public MetaFrameGeometry with MetaFrameBorders Attachment 193131 [details] pushed as a1a2527 - MetaFrameBorders: Add invisible borders Attachment 193474 [details] pushed as 2f254e5 - MetaWindowActor: Compensate for invisible borders