GNOME Bugzilla – Bug 759002
nautilus-canvas-view: Take margins into account when positioning rename popover
Last modified: 2015-12-04 15:16:05 UTC
See https://launchpadlibrarian.net/228296549/rename.png from the Launchpad bug https://bugs.launchpad.net/ubuntu/+source/nautilus/+bug/1522383 We need to shift the popover by the size of the work area. These seem to already be exposed as the canvas view margins. I added (private) API to get this and made the popover position take this into account. Maybe it could be done better elsewhere?
Created attachment 316732 [details] [review] nautilus-canvas-view: Take margins into account when positioning rename popover Shells can fix docks, panels and other similar things which reduce the available space to draw on. This is called the canvas margin. When positioning the popover we need to adjust by this margin, otherwise the popover will be shifted away from the target file.
Review of attachment 316732 [details] [review]: ::: src/nautilus-canvas-view.c @@ +1191,3 @@ bounding_box->y -= gtk_adjustment_get_value (vadjustment); + bounding_box->y += top_margin; + bounding_box->y -= bottom_margin; Doesn't this mean that if y is 0, and top margin is 0 but bottom margin is 10 we end with -10? I think what you want is just top and left margin addition right?
(In reply to Carlos Soriano from comment #2) > Review of attachment 316732 [details] [review] [review]: > > ::: src/nautilus-canvas-view.c > @@ +1191,3 @@ > bounding_box->y -= gtk_adjustment_get_value (vadjustment); > + bounding_box->y += top_margin; > + bounding_box->y -= bottom_margin; > > Doesn't this mean that if y is 0, and top margin is 0 but bottom margin is > 10 we end with -10? > I think what you want is just top and left margin addition right? In that case would the icon be partially off the screen or will it be forced to all be on screen? In that case we could clamp to zero. I don't have such a system to test but I imagine(d) that this problem exists for bottom/right too so was trying to cater for it. Or is it not a problem for some reason I don't get?
(In reply to Iain Lane from comment #3) > (In reply to Carlos Soriano from comment #2) > > Review of attachment 316732 [details] [review] [review] [review]: > > > > ::: src/nautilus-canvas-view.c > > @@ +1191,3 @@ > > bounding_box->y -= gtk_adjustment_get_value (vadjustment); > > + bounding_box->y += top_margin; > > + bounding_box->y -= bottom_margin; > > > > Doesn't this mean that if y is 0, and top margin is 0 but bottom margin is > > 10 we end with -10? > > I think what you want is just top and left margin addition right? > > In that case would the icon be partially off the screen or will it be forced > to all be on screen? In that case we could clamp to zero. > They will be inside the work area since no matter the position they take they are "clamp" when setting the scroll area. > I don't have such a system to test but I imagine(d) that this problem exists > for bottom/right too so was trying to cater for it. > I test just modifying the margins manually. This problem doesn't exist when modifying right or bottom margins. Take a look how everything is calculated in icon_set_position and you will see that x is not actually modified with left margins, however it takes into account right margin because it clamps to the maximum width. Also I think the fix should be in nautilus_canvas_container_get_icons_bounding_box to take into account this, not need for new internal API.
Created attachment 316767 [details] [review] nautilus-canvas-container: Add the left and top margins when calculating icon bounding box Shells can fix docks, panels and other similar things which reduce the available space to draw on. This is called the canvas margin. When positioning things relative to icons within containers we need to adjust by this margin, otherwise they will be shifted away from the target. Do this when we calculate the bounding box.
(In reply to Carlos Soriano from comment #4) > (In reply to Iain Lane from comment #3) > > (In reply to Carlos Soriano from comment #2) > > > Review of attachment 316732 [details] [review] [review] [review] [review]: > > > > > > ::: src/nautilus-canvas-view.c > > > @@ +1191,3 @@ > > > bounding_box->y -= gtk_adjustment_get_value (vadjustment); > > > + bounding_box->y += top_margin; > > > + bounding_box->y -= bottom_margin; > > > > > > Doesn't this mean that if y is 0, and top margin is 0 but bottom margin is > > > 10 we end with -10? > > > I think what you want is just top and left margin addition right? > > > > In that case would the icon be partially off the screen or will it be forced > > to all be on screen? In that case we could clamp to zero. > > > > They will be inside the work area since no matter the position they take > they are "clamp" when setting the scroll area. > > > I don't have such a system to test but I imagine(d) that this problem exists > > for bottom/right too so was trying to cater for it. > > > I test just modifying the margins manually. > This problem doesn't exist when modifying right or bottom margins. > Take a look how everything is calculated in icon_set_position and you will > see that x is not actually modified with left margins, however it takes into > account right margin because it clamps to the maximum width. > > Also I think the fix should be in > nautilus_canvas_container_get_icons_bounding_box to take into account this, > not need for new internal API. Thanks for the feedback, here's a new patch to that function which works for me. It seems that nautilus_canvas_container_get_icons_bounding_box is in the end only currently called by the rename popover anyway, so there's nowhere else to test. For good measure I checked on my hidpi (×2 scale) machine and the popovers were fine. I didn't actually manage to reproduce the initial bug there though. Wonder why that is.
Review of attachment 316767 [details] [review]: Now code looks good except the following nitpicks. Feel free to push with those. ::: libnautilus-private/nautilus-canvas-container.c @@ +6416,3 @@ g_array_index (result, GdkRectangle, index).width = (x2 - x1) * EEL_CANVAS (container)->pixels_per_unit; + g_array_index (result, GdkRectangle, index).y = (y1 * EEL_CANVAS (container)->pixels_per_unit) + + container->details->top_margin; can you align them? Also not need for parenthesis for multiplications. Something like this (sadly bugzilla is not appropriate to paste code) http://fpaste.org/297419/49239718/
Also the commit message title is too long, could you write something like: canvas-container: conider margins in icon bounding box calculation
consider*
Pushed with suggested changes, thanks. I had added the () for clarity but it's true they aren't required. ok to push to 3.18 too? The following fix has been pushed: 846085a canvas-container: consider margins when calculating icon bounding box
Created attachment 316771 [details] [review] canvas-container: consider margins when calculating icon bounding box Shells can fix docks, panels and other similar things which reduce the available space to draw on. This is called the canvas margin. When positioning things relative to icons within containers we need to adjust by this margin, otherwise they will be shifted away from the target. Do this when we calculate the bounding box.
yes, feel free. Thanks!!
Pushed to gnome-3-18 18652bed8732d7b2978da4fcb22b88c246d27dd0 thanks!