Details

Reviewers

Brecht Van Lommel (brecht)
Jacques Lucke (JacquesLucke)

Commits

rBS822c67364e85: UI: Fix odd behavior in region sizing, simplify code
rB822c67364e85: UI: Fix odd behavior in region sizing, simplify code

Summary

FIX:

Region max size is independent from dpi, and is possible to have bigger size that the tools size.

It will minimize when bigger, and will not open from tab click or drag up. To open it need to drag the tab[+] down to scale it down.

When a region have a bigger size from the space it can fit will not appear with clicking on tab (will need to drag). This case can happen when shrinking the size of a space.
File browser's tool's properties now have correct max size(can go up to top).

REMOVED: A piece of code about header size that have no use anymore.
And a part about header edge, that no longer needed.

Diff Detail

Repository: rB Blender

Event Timeline

George Vogiatzis (Gvgeo) created this revision.Mar 28 2019, 7:11 PM

Can you give specific steps to redo the issue this is fixing?

George Vogiatzis (Gvgeo) added inline comments.Mar 28 2019, 7:20 PM

source/blender/editors/screen/area.c
989	No longer a problem, it is no longer possible to hide the header from edge.
source/blender/editors/screen/screen_ops.c
2365	Header size is coded in other place, this part makes no difference anymore.

set scale 2.
in a tool regions(eg. file browser), maximize the tools properties region up to top.
shrink the Y size of the file browser. so that the tool properties get minimized.
then try to open the tool properties from clicking the [+} tab or by dragging it up.
Will not appear, can restore only if you drag it down or get screen bigger again.

Also fixes the maximum size Y the region can get when scale is not 1.

George Vogiatzis (Gvgeo) mentioned this in D4601: Add new region Footer, and Move Text Editor File Path to the bottom.Mar 28 2019, 9:06 PM

While this patch fixes more regions, there is still a bug left, I want to fix together. Maybe I will separate the old code removal part into a different patch.

Added fix 2, 3.
Clean up more code with the this update.

George Vogiatzis (Gvgeo) marked 2 inline comments as not done.Mar 29 2019, 7:31 PM

George Vogiatzis (Gvgeo) added inline comments.

source/blender/editors/screen/area.c
989	region_azone_edge_poll
source/blender/editors/screen/screen_ops.c
2365	`region_rect_recursive`

0001-2523.mkv7 MBDownload

Video with bugs 1 and 2 .

George Vogiatzis (Gvgeo) retitled this revision from Fix Tools Properties dpi bug. And remove old code for Header Size. to Fix Regions Max Size bugs. And remove old code for Header Size..Mar 29 2019, 8:15 PM

George Vogiatzis (Gvgeo) planned changes to this revision.Mar 30 2019, 3:15 PM

improved area_max_regionsize logic

William Reynish (billreynish) added a reviewer: Jacques Lucke (JacquesLucke).Apr 1 2019, 6:45 PM

I should mention a related bug, in case it is a 5 minute fix. But it is such a minuscule issue, that I don't believe opening a bug report worth it (you seem to have enough of these).

In file browser when clicking the tab to open the RGN_TYPE_TOOL_PROPS, if the click is not fast enough, in region_scale_modal along with the LEFTMOUSE case fires the MOUSEMOVE too.
It does not appear to do it consistently, but does not happen in other regions.
In a case where the region was hidden from shrinking the file browser size, the MOUSEMOVE will fire before LEFTMOUSE, causing the region taking the full size. The desired function is to leave a header size free.

Seems to work in my test. I made some comments inline.

In general it would be nicer if you would not try to fix 3 bugs at the same time (that would make it easier to review for me, as I don't know the code either).

source/blender/editors/screen/screen_ops.c
2295	style
2296	This will break when another flag is added in the future. Better use a mask to get the lower bits only and then do the `ELEM` check.
2323	style: use spaces to indent a second line of the condition.

This revision now requires changes to proceed.Apr 2 2019, 1:48 PM

It to me a while to understand the description, I think this would be more clear for the commit.

Fix odd behavior in region sizing, simplify code.

* When resizing sidebars, don't collapse when the region becomes too big but
  instead clamp the region size to the available space.
* Fix clicking the tab to expand sidebars no working if the sidebar is too
  wide to fit. Instead make it less wide so it does fit.
* Fix incorrect limit on tool properties region height, for example in the
  file browser.

Other than that the issues pointed out in code review this seems fine.

Moved dist calculation from rectangle, in the else statement.

Fixed style, added mask.

George Vogiatzis (Gvgeo) marked 3 inline comments as done.Apr 2 2019, 6:59 PM

Will commit in a moment.

source/blender/editors/screen/screen_ops.c
2288	hmm, this will still break when another flag at a higher bit is added. I understand that this is a tricky issue due to the mix of flags with non-flags in the same variable. I'll fix it myself.

This revision is now accepted and ready to land.Apr 3 2019, 2:41 PM

Closed by commit rB822c67364e85: UI: Fix odd behavior in region sizing, simplify code (authored by George Vogiatzis (Gvgeo), committed by Jacques Lucke (JacquesLucke)). · Explain WhyApr 3 2019, 3:02 PM

This revision was automatically updated to reflect the committed changes.

I totally forgot about the lower bits part you mention.

I tried to avoid doing this way and include something unknown.
I wanted to make sure it only had RGN_SPLIT_PREV flag.
I would prefer to stop working, than create a possible hidden bug in the future.

But I see that both of you agree on this.
Must be a 'try to include all cases' rule.

Fix Regions Max Size bugs. And remove old code for Header Size.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 14471

source/blender/editors/screen/area.c

source/blender/editors/screen/screen_ops.c

Fix Regions Max Size bugs. And remove old code for Header Size.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 14471

source/blender/editors/screen/area.c

source/blender/editors/screen/screen_ops.c

Fix Regions Max Size bugs. And remove old code for Header Size.
ClosedPublic
Actions

Revision Contents
Changeset List