Page MenuHome

UI : Changes to Screen Area Context Menu
ClosedPublic

Authored by Harley Acheson (harley) on Aug 11 2019, 3:10 AM.

Details

Summary

Right-clicking on an edge between editors brings up a context menu for "Area Options".

One item is shown as "Split Area", but I find that a bit unclear. You have clicked on a space between two areas so referring to a single area is confusing. Similarly an area can be split either horizontally or vertically and this will only do one of them, depending on the orientation of the edge. But that is not communicated to the user. This patch changes this item to say "Vertical Split" or "Horizontal Split" depending on what it would create. This should help lead the user into guessing how to do the other split direction.

The other item is "Join Area", which has the same problem. Join which area to where? This patch shows it as "Join Areas" and only shows this item if such a join is possible.

Following shows the menu at a horizontal edge where joining is possible:

And the following shows the menu at a vertical edge where a join is not possible:

And this also changes the border width thresholds so joins actually work at larger resolutions.

Diff Detail

Repository
rB Blender

Event Timeline

Brecht Van Lommel (brecht) requested changes to this revision.Aug 12 2019, 12:52 AM

The intent is fine, but this will fail in some corner cases.

The logic here must match area_join_init exactly. Right now it is missing the ED_area_is_global tests. It's also not clear that the specified min/max coordinates will in fact lead to joining along actedge in all cases.

I suggest to:

  • Change SCREEN_OT_area_split to have a cursor property like SCREEN_OT_area_join, rather than 4 min/max properties.
  • Have a function that takes x/y coordinate and returns sa1/sa2, and use that in both area_join_init and screen_area_options_invoke.
This revision now requires changes to proceed.Aug 12 2019, 12:52 AM

Good points. I didn't think about how showing the item only when it will probably work would make any false positives more noticeable and confusing.

Besides this might give me more time to get a "Swap Areas" option on that menu too. Needs to be a slight change from current operator since it would be non-interactive.

This patch now adds both "Vertical Split" and "Horizontal Split" no matter what edge you are on. It seemed confusing to only have vertical on one and horizontal on the other. The area could be square so you are just as likely to do either from any edge.

This also adds a "Swap Area" item. It does so immediately without interaction since this is non-destructive.

As per prior review this is now checking for global area to avoid having the options show up when not possible.

Note I did not though, change the min_x, min_y, max_x, Max_y to using a single "cursor" array instead. Operators like split can use "cursor" because they need to know about one area. But merge and swap need to know two. They do this by starting with mouse event position and move outward from there, and since it is on an edge that will find bordering areas.

Brecht Van Lommel (brecht) requested changes to this revision.EditedSun, Aug 18, 4:06 AM

This code should be deduplicated, it's still not clear that these actually match.

The code in screen_area_options_invoke computes everything based on context and a cursor position, merge and swap operators can surely do the same. There is no need to pass min/max coordinates.

This revision now requires changes to proceed.Sun, Aug 18, 4:06 AM

@Brecht Van Lommel (brecht) -
This code should be deduplicated, it's still not clear that these actually match.
The code in screen_area_options_invoke computes everything based on context and a cursor position, merge and swap operators can surely do the same. There is no need to pass min/max coordinates.

Sorry, I don't see it.

There are some area operators that need to know where the mouse is at the time they are invoked to determine what area(s) to act on. But sometimes you need to call those same operators with specific target areas in mind, which is why they are currently made to accept passed coordinates.

In the case of SCREEN_OT_area_options() it a menu of items constructed when you right-click on a edge. Before adding any items we must check if those operations are possible based on the mouse position when you clicked. And if they ARE possible then we have to pass THOSE mouse coordinates to those operators. If not, they would operate on the mouse coordinates at the time the operator was invoked, when you click on it on the menu, and that is wrong location.

So with Swap we have an existing invoke() for when we click on an action zone - it can rightly depend on mouse position at that time and start a modal operation to select the areas to swap. But for SCREEN_OT_area_options() we need to tell it exactly which areas to swap and do so immediately rather than interactively. So this requires a new exec(). Those two areas are based on the mouse position when you right clicked to invoke this menu, and can't be based on mouse position when you select the "Swap" item from the popup.

The mouse cursor indeed should be based on the position from where you open the menu, not where you clicked the menu item.

And this is what SCREEN_OT_area_split does, it passes the mouse coordinates with RNA_int_set_array(&ptr, "cursor", &event->x);. The same thing can be done for SCREEN_OT_area_swap.

The mouse cursor indeed should be based on the position from where you open the menu, not where you clicked the menu item.
And this is what SCREEN_OT_area_split does, it passes the mouse coordinates with RNA_int_set_array(&ptr, "cursor", &event->x);. The same thing can be done for SCREEN_OT_area_swap.

Yes, SPLIT needs to identify one area and does so based on mouse position or by passing an x and y coordinate in that "cursor" struct. one X and Y are enough to identify a single area.

But (the existing) MERGE needs to identify two areas. And so it does so by moving outward from the mouse position to make a rect that is wider than the editor gaps. This way if the mouse is a place between two areas, regardless of edge direction, that rectangle can identify both areas. In a nutshell we have to pass it enough information to select two areas and a single "cursor" struct is not enough do so.

The need of a non-interactive SWAP are the same as the existing MERGE: identify two areas. It would certainly be possible to move the work around a bit and just send the popup mouse position to MERGE and let it expand out to a rectangle and identify those areas. But that would be silly because we have already identified both areas in the previous tests to make sure that a swap was possible. That is why the information sent to swap is not based on cursor position but is actually the extents of the two areas involved.

Not sure how to explain this better than to just show the code:

1diff --git a/source/blender/editors/screen/screen_ops.c b/source/blender/editors/screen/screen_ops.c
2index eccd85ab276..3eceef45ef9 100644
3--- a/source/blender/editors/screen/screen_ops.c
4+++ b/source/blender/editors/screen/screen_ops.c
5@@ -1132,6 +1132,52 @@ static void SCREEN_OT_actionzone(wmOperatorType *ot)
6
7 /** \} */
8
9+/* -------------------------------------------------------------------- */
10+/** \name Area edge detection utility
11+ * \{ */
12+
13+static ScrEdge *screen_area_edge_from_cursor(const bContext *C,
14+ const int cursor[2],
15+ ScrArea **r_sa1,
16+ ScrArea **r_sa2)
17+{
18+ wmWindow *win = CTX_wm_window(C);
19+ bScreen *sc = CTX_wm_screen(C);
20+ ScrEdge *actedge;
21+ rcti window_rect;
22+
23+ WM_window_rect_calc(win, &window_rect);
24+ actedge = screen_geom_area_map_find_active_scredge(
25+ AREAMAP_FROM_SCREEN(sc), &window_rect, cursor[0], cursor[1]);
26+
27+ *r_sa1 = NULL;
28+ *r_sa2 = NULL;
29+ if (actedge == NULL) {
30+ return NULL;
31+ }
32+
33+ int borderwidth = (4 * UI_DPI_FAC);
34+ ScrArea *sa1, *sa2;
35+ if (screen_geom_edge_is_horizontal(actedge)) {
36+ sa1 = BKE_screen_find_area_xy(sc, SPACE_TYPE_ANY, cursor[0], cursor[1] + borderwidth);
37+ sa2 = BKE_screen_find_area_xy(sc, SPACE_TYPE_ANY, cursor[0], cursor[1] - borderwidth);
38+ }
39+ else {
40+ sa1 = BKE_screen_find_area_xy(sc, SPACE_TYPE_ANY, cursor[0] + borderwidth, cursor[1]);
41+ sa2 = BKE_screen_find_area_xy(sc, SPACE_TYPE_ANY, cursor[0] - borderwidth, cursor[1]);
42+ }
43+
44+ bool isGlobal = ((sa1 && ED_area_is_global(sa1)) || (sa2 && ED_area_is_global(sa2)));
45+ if (!isGlobal && area_getorientation(sa1, sa2) != -1) {
46+ *r_sa1 = sa1;
47+ *r_sa2 = sa2;
48+ }
49+
50+ return actedge;
51+}
52+
53+/** \} */
54+
55 /* -------------------------------------------------------------------- */
56 /** \name Swap Area Operator
57 * \{ */
58@@ -1151,6 +1197,7 @@ static void SCREEN_OT_actionzone(wmOperatorType *ot)
59 * callbacks:
60 *
61 * invoke() gets called on shift+lmb drag in action-zone
62+ * exec() execute without any user interaction, based on properties
63 * call init(), add handler
64 *
65 * modal() accept modal events while doing it
66@@ -1241,6 +1288,23 @@ static int area_swap_modal(bContext *C, wmOperator *op, const wmEvent *event)
67 return OPERATOR_RUNNING_MODAL;
68 }
69
70+static int area_swap_exec(bContext *C, wmOperator *op)
71+{
72+ ScrArea *sa1, *sa2;
73+ int cursor[2];
74+
75+ RNA_int_get_array(op->ptr, "cursor", cursor);
76+ screen_area_edge_from_cursor(C, cursor, &sa1, &sa2);
77+
78+ if (sa1 == NULL || sa2 == NULL) {
79+ return OPERATOR_CANCELLED;
80+ }
81+
82+ ED_area_swapspace(C, sa1, sa2);
83+
84+ return OPERATOR_FINISHED;
85+}
86+
87 static void SCREEN_OT_area_swap(wmOperatorType *ot)
88 {
89 ot->name = "Swap Areas";
90@@ -1249,10 +1313,15 @@ static void SCREEN_OT_area_swap(wmOperatorType *ot)
91
92 ot->invoke = area_swap_invoke;
93 ot->modal = area_swap_modal;
94- ot->poll = ED_operator_areaactive;
95+ ot->exec = area_swap_exec;
96+ ot->poll = screen_active_editable;
97 ot->cancel = area_swap_cancel;
98
99 ot->flag = OPTYPE_BLOCKING;
100+
101+ /* rna */
102+ RNA_def_int_vector(
103+ ot->srna, "cursor", 2, NULL, INT_MIN, INT_MAX, "Cursor", "", INT_MIN, INT_MAX);
104 }
105
106 /** \} */
107@@ -3175,40 +3244,20 @@ static void area_join_draw_cb(const struct wmWindow *UNUSED(win), void *userdata
108
109 /* validate selection inside screen, set variables OK */
110 /* return 0: init failed */
111-/* XXX todo: find edge based on (x,y) and set other area? */
112-static int area_join_init(bContext *C, wmOperator *op)
113+static int area_join_init(bContext *C, wmOperator *op, ScrArea *sa1, ScrArea *sa2)
114 {
115- const wmWindow *win = CTX_wm_window(C);
116- bScreen *screen = CTX_wm_screen(C);
117- ScrArea *sa1, *sa2;
118- sAreaJoinData *jd = NULL;
119- int x1, y1;
120- int x2, y2;
121-
122- /* required properties, make negative to get return 0 if not set by caller */
123- x1 = RNA_int_get(op->ptr, "min_x");
124- y1 = RNA_int_get(op->ptr, "min_y");
125- x2 = RNA_int_get(op->ptr, "max_x");
126- y2 = RNA_int_get(op->ptr, "max_y");
127-
128- sa1 = BKE_screen_find_area_xy(screen, SPACE_TYPE_ANY, x1, y1);
129- if (sa1 == NULL) {
130- sa1 = BKE_screen_area_map_find_area_xy(&win->global_areas, SPACE_TYPE_ANY, x1, y1);
131- }
132- sa2 = BKE_screen_find_area_xy(screen, SPACE_TYPE_ANY, x2, y2);
133- if (sa2 == NULL) {
134- sa2 = BKE_screen_area_map_find_area_xy(&win->global_areas, SPACE_TYPE_ANY, x2, y2);
135- }
136- if ((sa1 && ED_area_is_global(sa1)) || (sa2 && ED_area_is_global(sa2))) {
137- BKE_report(
138- op->reports, RPT_ERROR, "Global areas (Top Bar, Status Bar) do not support joining");
139- return 0;
140+ if (sa1 == NULL || sa2 == NULL) {
141+ /* Get areas from cursor location if not specified. */
142+ int cursor[2];
143+ RNA_int_get_array(op->ptr, "cursor", cursor);
144+ screen_area_edge_from_cursor(C, cursor, &sa1, &sa2);
145 }
146- else if (sa1 == NULL || sa2 == NULL || sa1 == sa2) {
147+
148+ if (sa1 == NULL || sa2 == NULL) {
149 return 0;
150 }
151
152- jd = (sAreaJoinData *)MEM_callocN(sizeof(sAreaJoinData), "op_area_join");
153+ sAreaJoinData *jd = MEM_callocN(sizeof(sAreaJoinData), "op_area_join");
154
155 jd->sa1 = sa1;
156 jd->sa2 = sa2;
157@@ -3261,7 +3310,7 @@ static void area_join_exit(bContext *C, wmOperator *op)
158
159 static int area_join_exec(bContext *C, wmOperator *op)
160 {
161- if (!area_join_init(C, op)) {
162+ if (!area_join_init(C, op, NULL, NULL)) {
163 return OPERATOR_CANCELLED;
164 }
165
166@@ -3292,15 +3341,14 @@ static int area_join_invoke(bContext *C, wmOperator *op, const wmEvent *event)
167 return OPERATOR_PASS_THROUGH;
168 }
169
170- /* prepare operator state vars */
171- RNA_int_set(op->ptr, "min_x", sad->sa1->totrct.xmin);
172- RNA_int_set(op->ptr, "min_y", sad->sa1->totrct.ymin);
173- RNA_int_set(op->ptr, "max_x", sad->sa2->totrct.xmin);
174- RNA_int_set(op->ptr, "max_y", sad->sa2->totrct.ymin);
175+ if (!area_join_init(C, op, sad->sa1, sad->sa2)) {
176+ return OPERATOR_CANCELLED;
177+ }
178 }
179-
180- if (!area_join_init(C, op)) {
181- return OPERATOR_CANCELLED;
182+ else {
183+ if (!area_join_init(C, op, NULL, NULL)) {
184+ return OPERATOR_CANCELLED;
185+ }
186 }
187
188 /* add temp handler */
189@@ -3431,10 +3479,8 @@ static void SCREEN_OT_area_join(wmOperatorType *ot)
190 ot->flag = OPTYPE_BLOCKING | OPTYPE_INTERNAL;
191
192 /* rna */
193- RNA_def_int(ot->srna, "min_x", -100, INT_MIN, INT_MAX, "X 1", "", INT_MIN, INT_MAX);
194- RNA_def_int(ot->srna, "min_y", -100, INT_MIN, INT_MAX, "Y 1", "", INT_MIN, INT_MAX);
195- RNA_def_int(ot->srna, "max_x", -100, INT_MIN, INT_MAX, "X 2", "", INT_MIN, INT_MAX);
196- RNA_def_int(ot->srna, "max_y", -100, INT_MIN, INT_MAX, "Y 2", "", INT_MIN, INT_MAX);
197+ RNA_def_int_vector(
198+ ot->srna, "cursor", 2, NULL, INT_MIN, INT_MAX, "Cursor", "", INT_MIN, INT_MAX);
199 }
200
201 /** \} */
202@@ -3445,36 +3491,73 @@ static void SCREEN_OT_area_join(wmOperatorType *ot)
203
204 static int screen_area_options_invoke(bContext *C, wmOperator *op, const wmEvent *event)
205 {
206- const wmWindow *win = CTX_wm_window(C);
207- const bScreen *sc = CTX_wm_screen(C);
208 uiPopupMenu *pup;
209 uiLayout *layout;
210 PointerRNA ptr;
211- ScrEdge *actedge;
212- rcti window_rect;
213-
214- WM_window_rect_calc(win, &window_rect);
215- actedge = screen_geom_area_map_find_active_scredge(
216- AREAMAP_FROM_SCREEN(sc), &window_rect, event->x, event->y);
217+ ScrArea *sa1, *sa2;
218
219- if (actedge == NULL) {
220+ if (screen_area_edge_from_cursor(C, &event->x, &sa1, &sa2) == NULL) {
221 return OPERATOR_CANCELLED;
222 }
223
224 pup = UI_popup_menu_begin(C, WM_operatortype_name(op->type, op->ptr), ICON_NONE);
225 layout = UI_popup_menu_layout(pup);
226
227- uiItemFullO(
228- layout, "SCREEN_OT_area_split", NULL, ICON_NONE, NULL, WM_OP_INVOKE_DEFAULT, 0, &ptr);
229+ /* Vertical Split */
230+ uiItemFullO(layout,
231+ "SCREEN_OT_area_split",
232+ IFACE_("Vertical Split"),
233+ ICON_NONE,
234+ NULL,
235+ WM_OP_INVOKE_DEFAULT,
236+ 0,
237+ &ptr);
238+ /* store initial mouse cursor position */
239+ RNA_int_set_array(&ptr, "cursor", &event->x);
240+ RNA_enum_set(&ptr, "direction", 'v');
241+
242+ /* Horizontal Split */
243+
244+ uiItemFullO(layout,
245+ "SCREEN_OT_area_split",
246+ IFACE_("Horizontal Split"),
247+ ICON_NONE,
248+ NULL,
249+ WM_OP_INVOKE_DEFAULT,
250+ 0,
251+ &ptr);
252 /* store initial mouse cursor position */
253 RNA_int_set_array(&ptr, "cursor", &event->x);
254+ RNA_enum_set(&ptr, "direction", 'h');
255+
256+ /* Test if we can do a merge or swap. */
257+ if (sa1 && sa2) {
258+ uiItemS(layout);
259
260- uiItemFullO(layout, "SCREEN_OT_area_join", NULL, ICON_NONE, NULL, WM_OP_INVOKE_DEFAULT, 0, &ptr);
261- /* mouse cursor on edge, '4' can fail on wide edges... */
262- RNA_int_set(&ptr, "min_x", event->x + 4);
263- RNA_int_set(&ptr, "min_y", event->y + 4);
264- RNA_int_set(&ptr, "max_x", event->x - 4);
265- RNA_int_set(&ptr, "max_y", event->y - 4);
266+ /* Join Areas */
267+
268+ uiItemFullO(layout,
269+ "SCREEN_OT_area_join",
270+ IFACE_("Join Areas"),
271+ ICON_NONE,
272+ NULL,
273+ WM_OP_INVOKE_DEFAULT,
274+ 0,
275+ &ptr);
276+ RNA_int_set_array(&ptr, "cursor", &event->x);
277+
278+ /* Merge Areas */
279+
280+ uiItemFullO(layout,
281+ "SCREEN_OT_area_swap",
282+ IFACE_("Swap Areas"),
283+ ICON_NONE,
284+ NULL,
285+ WM_OP_EXEC_DEFAULT,
286+ 0,
287+ &ptr);
288+ RNA_int_set_array(&ptr, "cursor", &event->x);
289+ }
290
291 UI_popup_menu_end(C, pup);
292

@Brecht Van Lommel (brecht)

Hey, thanks for this. I realize it must have been frustrating but I really do appreciate it. This does clean it up a lot and I understand fully what you are intending here. Makes a lot of sense.

But I still don't want you to think I'm braindead. LOL

The thing I was avoiding, because I was mostly playing with screen_area_options_invoke(), was fully determining the areas to merge or split (as you do with that new function) but then just ignoring that result, and only sending the cursor position, which is then used to (again) figure out the areas to work on. It was that part of it: going through the trouble of finding the information we need but then just ignoring that in favor of sending less information than needed, that I thought would look like a bad approach when reviewed.

But I certainly like how much that function cleans things up and makes it all more readable. And that is far more important than running a function twice. So yours is just simply better.

Again, thanks for being so patient. Let me test is a bit more though and want to check for areas that are too narrow to further split.

Merged in awesome fixes and suggestions from @Brecht Van Lommel (brecht)

Fixed an initialization skip in area_join_modal()

Removed test for ability to join from screen_area_edge_from_cursor() so that can use it for swap, which can be done with areas that might not be join-able.

Note that testing for ability to join is purposefully not in area_join_init(). We don't want to cancel an interactive join if dragging to a "bad" area. Instead we allow it to continue but show a corrective "no" cursor so you can still move to another area.

But seems to work well, makes sense, and is quite fast to manage this way.

Thanks, looks good now.

This revision is now accepted and ready to land.Thu, Aug 22, 6:40 PM
This revision was automatically updated to reflect the committed changes.

I'm hazarding a guess that this is what's causing a crash when the operator is invoked with a cursor value outside of the action zone.

See https://developer.blender.org/T69122