WIP: UI Fix: Interactive Docking modal operator re-run glitch #126380

Jonas Holzman · 2024-08-15T22:51:26+02:00

Jonas Holzman commented

2024-08-15 22:51:26 +02:00

The following is a weird bug, with an even weirder and (temporary) fix.

Bug Description

The main interactive docking modal operator (SCREEN_OT_area_join) can sometimes be wrongly re-run by the Window Manager modal handler after finishing when opening/closing windows. This is due to the modal handler interpreting changes in the current context window as the operator being freed. (see the condition in wm_event_system.cc). This causes the operator to get in a broken/in-between state, where the cursor randomly changes and different areas get random docking highlights and hover behavior.

Demo

Fix

As the check happens in the modal handler, my original fix idea was to try to fake it, by re-overriding the original window back into context, see this diff:

diff --git a/source/blender/editors/screen/screen_ops.cc b/source/blender/editors/screen/screen_ops.cc
--- a/source/blender/editors/screen/screen_ops.cc
+++ b/source/blender/editors/screen/screen_ops.cc
@@ -4021,8 +4028,10 @@
+  wmWindow *modal_win = CTX_wm_window(C);
+
   /* execute the events */
   switch (event->type) {
 
     case MOUSEMOVE: {
       area_join_update_data(C, jd, event);
       area_join_dock_cb_window(jd, op);
       WM_cursor_set(jd->win1, area_join_cursor(jd, event));
       WM_event_add_notifier(C, NC_WINDOW, nullptr);
@@ -4133,8 +4141,21 @@
 
         const bool do_close_win = jd->close_win;
         wmWindow *close_win = jd->win1;
         area_join_exit(C, op);
         if (do_close_win) {
           wm_window_close(C, CTX_wm_manager(C), close_win);
         }
 
+        if (modal_win != close_win) {
+          /* This modal operator may change the active context window by creating new ones.
+           * However, the Window Manager modal handler excepts the active context window to
+           * not change. If it does, the handler will assume the operator has been freed,
+           * preventing its function from properly finishing, and calling its invoke function
+           * again.
+           * To circumvent this issue, set the active context window back to its original state
+           * if it wasn't closed.
+           *
+           * See wm_handler_operator_call() in wm_event_system.cc for more details.
+           */
+          CTX_wm_window_set(C, modal_win);
+        }

This however, does not work when windows are being closed for their areas to be merged into other screens, as seen towards the end of the demo, as they have been freed and naturally cause null derefs in window_set.

I thus went for the alternative (and very hacky approach) of tagging the operator via a fake op->customdata address, acting as a value, a hack possible since the modal handler doesn't clear it between runs. This let the modal function check if it's being wrongly run again, and finish gracefully. See the content of this PR.

This took me quite some time to debug and figure out. And this is certainly not a proper fix. It (maybe?) could be merged in to let user better test the experimental feature. But a proper fix would be to either :

Find a better (less hacky) workaround
Not opening/closing window in the operator modal function.
Improving the check on the Window Manager modal handler side, looking at the git history, this used to check if the entire window_manager had changed, instead of just the window (link), which would work in our case, but is no longer valid. Perhaps we could find a better, sort of in-between/less invasive check without breaking operators/addons that rely on this behavior? (cc @ideasman42)

The following is a weird bug, with an even weirder and (temporary) fix. ### Bug Description The main interactive docking modal operator (`SCREEN_OT_area_join`) can sometimes be wrongly re-run by the Window Manager modal handler after finishing when opening/closing windows. This is due to the modal handler interpreting changes in the current context window as the operator being freed. (see [the condition in wm_event_system.cc](https://projects.blender.org/blender/blender/src/commit/f6141b3fa7ebcb1ba9fe1cbe345f1f995ac691de/source/blender/windowmanager/intern/wm_event_system.cc#L2549)). This causes the operator to get in a broken/in-between state, where the cursor randomly changes and different areas get random docking highlights and hover behavior. #### Demo ![Capture_2024-08-15_22.21.08.gif](/attachments/c0175d1b-58ef-485b-957d-e8962221a335) --- ### Fix As the check happens in the modal handler, my original fix idea was to try to fake it, by re-overriding the original window back into context, see this diff: ``` diff --git a/source/blender/editors/screen/screen_ops.cc b/source/blender/editors/screen/screen_ops.cc --- a/source/blender/editors/screen/screen_ops.cc +++ b/source/blender/editors/screen/screen_ops.cc @@ -4021,8 +4028,10 @@ + wmWindow *modal_win = CTX_wm_window(C); + /* execute the events */ switch (event->type) { case MOUSEMOVE: { area_join_update_data(C, jd, event); area_join_dock_cb_window(jd, op); WM_cursor_set(jd->win1, area_join_cursor(jd, event)); WM_event_add_notifier(C, NC_WINDOW, nullptr); @@ -4133,8 +4141,21 @@ const bool do_close_win = jd->close_win; wmWindow *close_win = jd->win1; area_join_exit(C, op); if (do_close_win) { wm_window_close(C, CTX_wm_manager(C), close_win); } + if (modal_win != close_win) { + /* This modal operator may change the active context window by creating new ones. + * However, the Window Manager modal handler excepts the active context window to + * not change. If it does, the handler will assume the operator has been freed, + * preventing its function from properly finishing, and calling its invoke function + * again. + * To circumvent this issue, set the active context window back to its original state + * if it wasn't closed. + * + * See wm_handler_operator_call() in wm_event_system.cc for more details. + */ + CTX_wm_window_set(C, modal_win); + } ``` This however, does not work when windows are being closed for their areas to be merged into other screens, as seen towards the end of the demo, as they have been freed and naturally cause null derefs in `window_set`. I thus went for the alternative (and very hacky approach) of tagging the operator via a fake `op->customdata` address, acting as a value, a hack possible since the modal handler doesn't clear it between runs. This let the modal function check if it's being wrongly run again, and finish gracefully. See the content of this PR. This took me quite some time to debug and figure out. And this is certainly not a proper fix. It (maybe?) could be merged in to let user better test the experimental feature. But a proper fix would be to either : - Find a better (less hacky) workaround - Not opening/closing window in the operator modal function. - Improving the check on the Window Manager modal handler side, looking at the git history, this used to check if the entire window_manager had changed, instead of just the window ([link](https://projects.blender.org/blender/blender/commit/2939251a05c29b6462065394714a79195d067029)), which would work in our case, but is no longer valid. Perhaps we could find a better, sort of in-between/less invasive check without breaking operators/addons that rely on this behavior? (cc @ideasman42)

Capture_2024-08-15_22.21.08.gif

2.9 MiB

Jonas Holzman force-pushed fix-bad-docking-modal from 0c3abf0a9f to 84cda54113

2024-08-15 22:52:42 +02:00

Compare

Jonas Holzman requested review from Harley Acheson 2024-08-15 22:53:18 +02:00

Jonas Holzman changed title from ~~WIP Fix: Interactive Docking modal operator re-run glitch~~ to WIP: UI Fix: Interactive Docking modal operator re-run glitch

2024-08-15 22:57:07 +02:00

Jonas Holzman added 1 commit 2024-08-15 22:58:41 +02:00

Consistent Hex address formatting c3b40e4d4b

Jonas Holzman commented

2024-08-16 00:55:25 +02:00

Somehow, a Gitea bug seems to have duplicated this PR (seems to have happen on rename), closing in favor of the original: #126379

Jonas Holzman closed this pull request

2024-08-16 00:55:27 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: UI Fix: Interactive Docking modal operator re-run glitch #126380

Bug Description

Demo

Fix

Pull request closed