Allow joining of one area into part of another
Needs ReviewPublic

Authored by Daan Michiels (daan) on Mar 23 2017, 4:53 PM.

Details

Summary

Right now, one can only join two adjacent areas if they have precisely an edge in common. This makes some area manipulation operations a little hard to do.
I wrote a patch to make the join operator more flexible, allowing operations like these:

Notes:
Areas have minimum dimensions. Right now, if a join operation as above would leave an area that is too small, the operation is not allowed. I think that's intuitive behavior, but please let me know if this (or anything else) should behave differently.

I'm new to Blender (and new to open source contributions, besides fixing typos), so I wasn't completely sure about the inner working of the windowing system. In particular, I didn't know which redrawing functions need to be called. This is marked in the patch with CODE REVIEW QUESTION.

Diff Detail

I tested the patch and it works ok... While it solves one of the issues noted here: https://wiki.blender.org/index.php/Dev:2.8/UI/Workshop_Writeup#Areas
It is a bit awkward to use, I think part of the reason is there is no support for merging an area in two areas like in the image below:

Because of this I found myself getting "stuck", with the patch I was able to get out of this but it was like a puzzle.
I would be against merging this patch without having support described in the image above.

Aaron Carlisle (Blendify) requested changes to this revision.Mar 23 2017, 6:01 PM
This revision now requires changes to proceed.Mar 23 2017, 6:01 PM
Daan Michiels (daan) added a comment.EditedMar 23 2017, 7:07 PM

Thanks, @Aaron Carlisle (Blendify)! I'm glad the patch works for you.
While I agree that it is possible to get "stuck", surely it is much easier to get stuck without the patch than with it? (So I don't see why this is a reason not to merge.)
This getting stuck is the reason I wrote it in the first place. The join operations that the patch allows are a strict superset of the join operations that are allowed without the patch.

About the second improvement to area joining that you refer to: yes, I agree this needs to be done. But it's a more complicated task than the first one, both in defining what the correct behavior is, and in the implementation.

For example, what is the correct behavior in this case?

+---+---+---+
|   |   |   |
| 1 +-+-+-+-+
|   | |   | |
+---+-+---+-+

Area 1 gets its upper-right corner dragged to the right.

Implementation-wise, the join operator will need to keep track of more than just two areas, and will need to find areas by different means than just finding the area under the cursor. Also, the code that draws the join overlay will need to be restructured.

Julian Eisel (Severin) requested changes to this revision.Mar 23 2017, 10:48 PM

Great to see this tackled!
I also don't really agree with @Aaron Carlisle (Blendify) here, I find this much more intuitive than what we have currently.
Functionality wise I'd say it's fine to put this into the 2.8 branch as it overlaps with quite some other (planned) 2.8 UI changes. Note that there might be some conflicts when porting the patch to the blender2.8 branch.

Had to apply this patch to make it compile without errors or warnings:

1diff --git a/source/blender/editors/screen/screen_edit.c b/source/blender/editors/screen/screen_edit.c
2index 9653ae6..09f40c0 100644
3--- a/source/blender/editors/screen/screen_edit.c
4+++ b/source/blender/editors/screen/screen_edit.c
5@@ -678,7 +678,7 @@ int screen_area_join(bContext *C, bScreen *scr, ScrArea *sa1, ScrArea *sa2)
6​ * screen_area_join_partially is for when area_jointype=2)
7​ * return value 0=failed, 1=success
8​ * used by the join operator */
9-int screen_area_join_partially(bContext *C, bScreen *scr, ScrArea *sa1, ScrArea *sa2)
10+int screen_area_join_partially(bContext *UNUSED(C), bScreen *scr, ScrArea *sa1, ScrArea *sa2)
11​ {
12​ ScrVert *v;
13​ int dir;
14@@ -1061,10 +1061,10 @@ static void draw_horizontal_join_shape(ScrArea *sa1, ScrArea *sa2, char dir)
15​ v2.x = sa2->v2->vec.x;
16​ v3.x = sa2->v3->vec.x;
17​ v4.x = sa2->v4->vec.x;
18- v1.y = max(sa2->v1->vec.y, sa1->v1->vec.y);
19- v2.y = min(sa2->v2->vec.y, sa1->v2->vec.y);
20- v3.y = min(sa2->v3->vec.y, sa1->v3->vec.y);
21- v4.y = max(sa2->v4->vec.y, sa1->v4->vec.y);
22+ v1.y = MAX2(sa2->v1->vec.y, sa1->v1->vec.y);
23+ v2.y = MIN2(sa2->v2->vec.y, sa1->v2->vec.y);
24+ v3.y = MIN2(sa2->v3->vec.y, sa1->v3->vec.y);
25+ v4.y = MAX2(sa2->v4->vec.y, sa1->v4->vec.y);
26
27​ width = v3.x - v1.x;
28​ height = v3.y - v1.y;
29@@ -1146,10 +1146,10 @@ static void draw_vertical_join_shape(ScrArea *sa1, ScrArea *sa2, char dir)
30​ v2.y = sa2->v2->vec.y;
31​ v3.y = sa2->v3->vec.y;
32​ v4.y = sa2->v4->vec.y;
33- v1.x = max(sa2->v1->vec.x, sa1->v1->vec.x);
34- v2.x = max(sa2->v2->vec.x, sa1->v2->vec.x);
35- v3.x = min(sa2->v3->vec.x, sa1->v3->vec.x);
36- v4.x = min(sa2->v4->vec.x, sa1->v4->vec.x);
37+ v1.x = MAX2(sa2->v1->vec.x, sa1->v1->vec.x);
38+ v2.x = MAX2(sa2->v2->vec.x, sa1->v2->vec.x);
39+ v3.x = MIN2(sa2->v3->vec.x, sa1->v3->vec.x);
40+ v4.x = MIN2(sa2->v4->vec.x, sa1->v4->vec.x);
41
42​ width = v3.x - v1.x;
43​ height = v3.y - v1.y;
44diff --git a/source/blender/editors/screen/screen_ops.c b/source/blender/editors/screen/screen_ops.c
45index f63dd45..6c3805f 100644
46--- a/source/blender/editors/screen/screen_ops.c
47+++ b/source/blender/editors/screen/screen_ops.c
48@@ -2605,6 +2605,8 @@ static int area_join_apply(bContext *C, wmOperator *op)
49​ }
50​ return 1;
51​ }
52+
53+ return 0;
54​ }
55
56​ /* finish operation */

Also found the following bugs:

  • While dragging it's possible to loose track of initial area, you can join completely independent areas then: F521041
  • I got a couple of errors like "error: area 0 edge 3 doesn't exist" printed. Think this comes from trying to join areas at the screen edge: F521040

Added some inline comments regarding code-style that doesn't follow our guidelines and some personal preferences.

source/blender/editors/screen/screen_edit.c
548–559

I realize this is not your code, but would still prefer to change this to use an enum for return values while you're rewriting large parts of this function anyway.

It's the old "Clean code doesn't need any comments" thing ;)

581–583

Same as above, use an enum for return values, makes things clear without having to read a comment.

610–611

Brackets for multi-line if's should be in a new line:

if ((a && bunch && of && booleans) ||
    (some && more && booleans))
{
    foo_bar();
}
823–826

Don't see a need for it, notifiers and redraws are already handled by operator callbacks.

1058

Better use more useful names for these, such as bottom_right, top_right, etc. Otherwise you just end up having to re-check which variable is what all the time.

source/blender/editors/screen/screen_ops.c
2704–2705

It's quite hard to understand what this does, could you explain some more? If it ends up being useful I would put it into an extra function, with a name that clearly indicates what it's actually doing.

2739

Missing space here: else if( should be else if (.

+---+---+---+
|   |   |   |
| 1 +-+-+-+-+
|   | |   | |
+---+-+---+-+

Area 1 gets its upper-right corner dragged to the right.

I can think of two ways to solve this:

  • Always use the first set of edges that form the same horizontal/vertical coordinates as the one dragged from - In your example this would be the screen boundary on the right.
  • Allow gradually increasing join size edge by edge - In your example this would look something like this:
Initial layout:
+---+---+---+
|   |   |   |
| 1 +-+-+-+-+
|   | |   | |
+---+-+---+-+
Mouse release after dragging over first vertical edge:
+-----+-+---+
|     | |   |
|  1  +-+-+-+
|     |   | |
+-----+---+-+
Mouse release after dragging over next vertical edge:
+-------+---+
|       |   |
|   1   +-+-+
|       | | |
+-------+-+-+
...

Implementation-wise, the join operator will need to keep track of more than just two areas, and will need to find areas by different means than just finding the area under the cursor. Also, the code that draws the join overlay will need to be restructured.

I definitely wouldn't mind a refactor of the area joining code, it's really hard to understand whats going on in it, far from self-explaining. Guess the main reasons for that are bad variable names and cryptic integer and character values instead of enums/defines.
Feel free to give it a go, otherwise I will ;)

Daan Michiels (daan) marked 5 inline comments as done.

I applied the compile fixes.

The error: area 0 edge 3 doesn't exist bug is resolved. It was not related to the window edge, but due to mistaken indices somewhere.

Fixed the parts that differed from the guidelines.
(Question: are my changes to screen_intern.h formatted well? I want to make sure.)

Refactored area_getorientation and area_jointype to use enums. This clears up the code quite a bit, and brought to light a simplification that could be made in the old arrow-drawing code.

Thanks for the helpful comments, @Julian Eisel (Severin)!

About the first bug you mention: This is by design. The old join operator also allows losing track of the initial area, and joining areas far away from it (see screenshot). In fact, the part of screen_ops.c where you say "could you explain some more?" is precisely the part that allows this behavior.
The old code has the following section (a couple of lines lower):

/* we are not bordering on the previously selected area 
 * we check if area has common border with the one marked for removal
 * in this case we can change areas
 */
if (area_jointype(jd->sa2, sa) != NOJOIN) {
	if (jd->sa1) jd->sa1->flag &= ~AREA_FLAG_DRAWJOINFROM;
	if (jd->sa2) jd->sa2->flag &= ~AREA_FLAG_DRAWJOINTO;
	jd->sa1 = jd->sa2;
	jd->sa2 = sa;
	if (jd->sa1) jd->sa1->flag |= AREA_FLAG_DRAWJOINFROM;
	if (jd->sa2) jd->sa2->flag |= AREA_FLAG_DRAWJOINTO;
}

The section you ask about is the partial-join equivalent of this. I can take out this behavior if you want, but I tried both behaviors and to me this feels the most natural.

About the useful names for the vertices: I chose v1, v2, v3, v4 because all the other area-related code uses this convention (e.g. in the definition of ScrArea). If you want, I can use longer names, though, at the cost of having two ways of naming in the code.

About your suggestions for the other flexible join type: yes, those seem like good options. My personal preference is the gradual increase as the mouse is dragged, I think.
(A third option would be to always expand the minimum distance necessary to absord at least one area, which is effectively the same as your gradual-increase proposal with the cursor crossing only a single edge.)

In any case, I probably won't have time to write this second kind of flexible join operation anytime soon. I'll keep revising this first type of flexible join until it's ready to be merged (I haven't put it into the 2.8 branch yet -- btw, what am I supposed to do for this? Rebase on blender2.8 and submit a new diff here?), but after that it's all yours.

This revision now requires changes to proceed.Nov 21 2017, 5:45 PM

I feel like the poke is meant for me, but I don't know what I'm supposed to do. I've made all the requested changes, or explained why I think they are not needed.
Is there an "I'm done revising"-button? I couldn't find one half a year ago, and I can't find one now.

Daan Michiels (daan) requested review of this revision.Nov 21 2017, 6:43 PM

Aha, I think I found it!

No, the poke was mainly for reviewers. Glad to hear that you are still active!