UI: Fix Scrollbar overlaps sidebar on zoom #108295

Harley Acheson · 2023-05-25T23:00:47+02:00

Harley Acheson commented

2023-05-25 23:00:47 +02:00

When zooming an area with panel category tabs, ensure that the vertical
scroll bar is repositioned to not overlap the tabs.

The central issue is that we don't reposition the scroll actionzones once they are created. In most cases this doesn't matter, but does here where we want a scaled position away from the right edge.

This PR makes it so that AZONE_REGION_SCROLL are hit tested against the area around the scrollbar itself, not the action zone rect. We always move and resize scrollbars properly, so no need to keep a separate rect in sync with them.

When zooming an area with panel category tabs, ensure that the vertical scroll bar is repositioned to not overlap the tabs. --- The central issue is that we don't reposition the scroll actionzones once they are created. In most cases this doesn't matter, but does here where we want a scaled position away from the right edge. This PR makes it so that AZONE_REGION_SCROLL are hit tested against the area around the scrollbar itself, not the action zone rect. We always move and resize scrollbars properly, so no need to keep a separate rect in sync with them. ![image](/attachments/fabe6519-17bb-4962-beb6-02f8f9394c6f)

image.png

27 KiB

Harley Acheson added 1 commit 2023-05-25 23:00:57 +02:00

55bf1f7e76 UI: Fix Scrollbar overlaps sidebar on zoom

When zooming an area with panel category tabs, the vertical scroll bar
can be placed in a position that overlaps the tabs.

Harley Acheson added this to the User Interface project 2023-05-25 23:02:19 +02:00

Harley Acheson added this to the 3.6 LTS milestone 2023-05-25 23:02:26 +02:00

Harley Acheson requested review from Pablo Vazquez 2023-05-25 23:02:48 +02:00

Harley Acheson requested review from Campbell Barton 2023-05-25 23:04:52 +02:00

Harley Acheson referenced this pull request

2023-05-26 01:30:33 +02:00

UI: Fix Scrollbar overlaps sidebar on zoom #106014

Campbell Barton requested changes 2023-05-26 05:53:43 +02:00

Campbell Barton left a comment

Did you check on handing this when the scroll-bars are initially placed?

The problem with moving the scroll bars while drawing is the action zones might not be updated for event handling, testing the patch it causes flickering opacity & size while dragging.

Did you check on handing this when the scroll-bars are initially placed? The problem with moving the scroll bars while drawing is the action zones might not be updated for event handling, testing the patch it causes flickering opacity & size while dragging.

source/blender/editors/screen/area.cc Outdated

						
				@ -3137,1 +3139,3 @@

				    mask.xmax -= UI_PANEL_CATEGORY_MARGIN_WIDTH;

				    const int category_tabs_width = round_fl_to_int(UI_view2d_scale_get_x(&region->v2d) *

				                                                    UI_PANEL_CATEGORY_MARGIN_WIDTH);

				    mask.xmax = mask.xmax - category_tabs_width;

Campbell Barton commented

2023-05-26 05:42:36 +02:00

Can still use -=.

Can still use `-=`.

Harley marked this conversation as resolved

source/blender/editors/screen/area.cc Outdated

						
				@ -3138,0 +3142,4 @@

				    BLI_rcti_translate(&region->v2d.vert, -category_tabs_width, 0);

				    /* Adjust the scroller's action zone. */

				    LISTBASE_FOREACH (AZone *, az, &CTX_wm_area(C)->actionzones) {

Campbell Barton commented

2023-05-26 05:47:07 +02:00

It would read more clearly if there was a function to lookup the action zone, e.g.

AZone *az = BKE_area_find_action_zone_by_type(area, region, AZONE_REGION_SCROLL);
if (az) {
  ...
}

It would read more clearly if there was a function to lookup the action zone, e.g. ``` AZone *az = BKE_area_find_action_zone_by_type(area, region, AZONE_REGION_SCROLL); if (az) { ... } ```

Harley marked this conversation as resolved

Harley Acheson added 1 commit 2023-05-26 19:10:13 +02:00

765d4589e5 Updated to incorporate changes requested by review

Harley Acheson commented

2023-05-26 19:22:20 +02:00

Did you check on handing this when the scroll-bars are initially placed?

I did and found no luck. In this panel draw we call UI_view2d_scrollers_draw_ex with a mask only in this case, but currently calculating the right side without using local zoom. But if corrected the azone is in the wrong spot.

The problem with moving the scroll bars while drawing is the action zones might not be updated for event handling, testing the patch it causes flickering opacity & size while dragging.

I'm not seeing a differing behavior from current while dragging over the scrollbars. I'm just test by dragging an outliner item over it and it seems to look the same as 3.5 while doing so. Are you testing a particular action that looks worse?

> Did you check on handing this when the scroll-bars are initially placed? I did and found no luck. In this panel draw we call UI_view2d_scrollers_draw_ex with a mask only in this case, but currently calculating the right side without using local zoom. But if corrected the azone is in the wrong spot. > The problem with moving the scroll bars while drawing is the action zones might not be updated for event handling, testing the patch it causes flickering opacity & size while dragging. I'm not seeing a differing behavior from current while dragging over the scrollbars. I'm just test by dragging an outliner item over it and it seems to look the same as 3.5 while doing so. Are you testing a particular action that looks worse?

Harley Acheson commented

2023-06-07 18:45:13 +02:00

@ideasman42 - testing the patch it causes flickering opacity & size while dragging

Still not sure what you are seeing. When testing I see the same behavior with size and opacity changes while approaching, and while dragging the scrollbar it looks the same to me. Are you describing something different, a different process, or state from the following?

> @ideasman42 - testing the patch it causes flickering opacity & size while dragging Still not sure what you are seeing. When testing I see the same behavior with size and opacity changes while approaching, and while dragging the scrollbar it looks the same to me. Are you describing something different, a different process, or state from the following? ![ScrollOffset.gif](/attachments/b8877808-4dba-4ae4-bd11-b4a67a47a524)

ScrollOffset.gif

419 KiB

Julian Eisel reviewed 2023-06-08 13:06:11 +02:00

source/blender/editors/screen/area.cc Outdated

						
				@ -3138,0 +3143,4 @@

				    /* Adjust the scroller's action zone. */

				    AZone *az = ED_area_actionzone_find_by_type(CTX_wm_area(C), region, AZONE_REGION_SCROLL);

				    if (az) {

Julian Eisel commented

2023-06-08 13:06:05 +02:00

I don't like the idea of manipulating action-zones in region drawing code, this has quite a bit of a code smell to me. region_azone_scrollbar_init() sets the azone rectangles, could we do this elsewhere, based on the modified mask rectangle?
We could do it as part of area_actionzone_refresh_xy(), but that might not be called when we need it. Another option is to support dynamically changing rectangles, e.g. there could be a AZone.get_rect() callback instead of Azone.rect.

I don't like the idea of manipulating action-zones in region drawing code, this has quite a bit of a code smell to me. `region_azone_scrollbar_init()` sets the azone rectangles, could we do this elsewhere, based on the modified mask rectangle? We could do it as part of `area_actionzone_refresh_xy()`, but that might not be called when we need it. Another option is to support dynamically changing rectangles, e.g. there could be a `AZone.get_rect()` callback instead of `Azone.rect`.

Harley marked this conversation as resolved

Harley Acheson commented

2023-06-09 02:39:15 +02:00

@JulianEisel - I don't like the idea of manipulating action-zones in region drawing code, this has quite a bit of a code smell to me

Yes, a bit whiffy to me too.

It looks like it might be possible to modify area_actionzone_refresh_xy but might make a bit of a mess. Instead of testing the point against the azone's rect we could (just for AZONE_REGION_SCROLL) test against the az->region->vert (+- V2D_SCROLL_HIDE_WIDTH) instead. That way the azone's rect doesn't have to be changed? Would look weird to treat one type of zone differently though and to ignore its actual position.

> @JulianEisel - I don't like the idea of manipulating action-zones in region drawing code, this has quite a bit of a code smell to me Yes, a bit whiffy to me too. It looks like it _might_ be possible to modify `area_actionzone_refresh_xy` but might make a bit of a mess. Instead of testing the point against the azone's rect we could (just for AZONE_REGION_SCROLL) test against the az->region->vert (+- V2D_SCROLL_HIDE_WIDTH) instead. That way the azone's rect doesn't have to be changed? Would look weird to treat one type of zone differently though and to ignore its actual position.

Harley Acheson changed title from ~~UI: Fix Scrollbar overlaps sidebar on zoom~~ to WIP: UI: Fix Scrollbar overlaps sidebar on zoom

2023-06-21 02:09:59 +02:00

Harley Acheson added 1 commit 2023-06-21 02:12:40 +02:00

b17bb6ae70 Do not move scroll action zones. Instead use scroller position rather than azone position.

Harley Acheson added 1 commit 2023-06-21 02:45:43 +02:00

7c1fb46f07 Small changes, cleanup.

Harley Acheson added 1 commit 2023-06-21 02:55:07 +02:00

2c5a5d96c7 simplification.

Harley Acheson commented

2023-06-21 03:02:47 +02:00

This looks to be a much nicer solution, but needs a bit more testing.

Harley Acheson added 1 commit 2023-06-21 17:19:06 +02:00

7d6311a985 Comment changes, putting UI_panel_category_is_visible back in, etc

Harley Acheson changed title from ~~WIP: UI: Fix Scrollbar overlaps sidebar on zoom~~ to UI: Fix Scrollbar overlaps sidebar on zoom

2023-06-21 17:22:41 +02:00

Campbell Barton approved these changes 2023-06-22 03:57:47 +02:00

source/blender/editors/screen/area.cc Outdated

						
				@ -1062,7 +1062,6 @@ static void region_azone_scrollbar_init(ScrArea *area,

				{

				  rcti scroller_vert = (direction == AZ_SCROLL_VERT) ? region->v2d.vert : region->v2d.hor;