Fix: Interactive Docking modal operator re-run glitch #126379

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

Jonas Holzman commented

2024-08-15 22:49:15 +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.

Note

This has since been updated with a simpler solution using a patch based on the WINDEACTIVATE event by @Harley, see discussion below

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 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. > [!NOTE] > This has since been updated with a simpler solution using a patch based on the `WINDEACTIVATE` event by @Harley, see discussion below 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)

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

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

Compare

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

Consistent Hex address formatting c3b40e4d4b

Jonas Holzman requested review from Harley Acheson 2024-08-15 23:09:17 +02:00

Jonas Holzman referenced this pull request

2024-08-15 23:54:57 +02:00

Fix: UI: Editor window(s) title update when using interactive docking #126383

Jonas Holzman referenced this pull request

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

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

Harley Acheson commented

2024-08-20 19:56:58 +02:00

@Brainzman

I have no doubt there is an error here, but I've been unable to reproduce what you show in the capture. Are there reproducible steps to take that causes this?

@Brainzman I have no doubt there is an error here, but I've been unable to reproduce what you show in the capture. Are there reproducible steps to take that causes this?

Jonas Holzman commented

2024-09-07 14:08:22 +02:00

@Harley I can still reproduce the issue on the latest main, attached you'll find a .blend file you should be able to open with Load UI to get to the starting point of the following demo.

repro.blend

The problem happens when drag-and-dropping a window into another one to merge their editors. It happens every time if the window has just been created/teared off, and may or may not happen if multiple actions like selecting other windows or switching apps have happened since creating the second window (as seen in the demo), in which case, the window can just be teared off and immediately drag-and-dropped back in to reproduce the bug.

I don't know if the .blend file window placement will still hold on Windows, and the fact that I have multiple monitors plugged in could also mess things up a little. If that's the case, you can reproduce it manually by just opening two additional child windows like I did, window size/placement doesn't matter in my tests.

Note that the issue could also be macOS only, I'm curious to know if anyone else could reproduce it on macOS (perhaps @Alaska if you're interested?)

@Harley I can still reproduce the issue on the latest main, attached you'll find a .blend file you should be able to open with Load UI to get to the starting point of the following demo. <video src="/attachments/6c4eb097-55f7-4f32-9b01-9698478e81c2" title="Capture_2024-09-07_13.56.09-converted.mp4" controls></video> [repro.blend](/attachments/f06f5df1-c61c-4d3c-aaa3-022f761d61ab) The problem happens when drag-and-dropping a window into another one to merge their editors. It happens every time if the window has just been created/teared off, and may or may not happen if multiple actions like selecting other windows or switching apps have happened since creating the second window (as seen in the demo), in which case, the window can just be teared off and immediately drag-and-dropped back in to reproduce the bug. I don't know if the .blend file window placement will still hold on Windows, and the fact that I have multiple monitors plugged in could also mess things up a little. If that's the case, you can reproduce it manually by just opening two additional child windows like I did, window size/placement doesn't matter in my tests. Note that the issue could also be macOS only, I'm curious to know if anyone else could reproduce it on macOS (perhaps @Alaska if you're interested?)

repro.blend

540 KiB

Capture_2024-09-07_13.56.09-converted.mp4

12 MiB

Alaska commented

2024-09-07 17:36:51 +02:00

Note that the issue could also be macOS only, I'm curious to know if anyone else could reproduce it on macOS (perhaps @Alaska if you're interested?)

I can reproduce the issue here with latest main on my Mac and on Windows with the steps shown in your video with the file attached to your comment.

If it helps, tests were run on a 2560x1440 screen with 100% scaling in both cases.

> Note that the issue could also be macOS only, I'm curious to know if anyone else could reproduce it on macOS (perhaps @Alaska if you're interested?) I can reproduce the issue here with latest main on my Mac and on Windows with the steps shown in your video with the file attached to your comment. If it helps, tests were run on a 2560x1440 screen with 100% scaling in both cases.

❤️ 1

Harley Acheson commented

2024-09-07 23:58:02 +02:00

Oh, I can (finally) recreate this by using this blend. Thanks!

Harley Acheson commented

2024-09-11 02:28:08 +02:00

@Brainzman

I'm sure this is separate issue, but I'd love for you to confirm that you see the same thing....

When I launch that "repro.blend" on my computer I get three windows, a main window on my main monitor containing the usual default editors. On my second monitor (to the right) there are two windows, one an Image Editor and the other a Timeline.

It is the Timeline that is the most interesting. Its left edge is at that monitor's left edge, actually just one pixel to the left of it. If I drag to that one from either other window it seems at first that it will only dock to the entire window. But that isn't really it. The tooltip info normally shown at the mouse cursor is not shown. That has to be shown somewhere so it is probably out of window bounds.

If I move that window even slightly it works correctly. While at the edge and it not working, the value for x in area_docking_target has a value that is not in that monitor. I drag into the very left part of where I would normally get "10" or so, I get 1400 or so - a value that would be inside the left monitor,

But I do not think this is related to the calculations in area_docking_target. So far I think "win2" is not incorrect. But still investigating...

@Brainzman I'm sure this is separate issue, but I'd love for you to confirm that you see the same thing.... When I launch that "repro.blend" on my computer I get three windows, a main window on my main monitor containing the usual default editors. On my second monitor (to the right) there are two windows, one an Image Editor and the other a Timeline. It is the Timeline that is the most interesting. Its left edge is at that monitor's left edge, actually just one pixel to the left of it. If I drag to that one from either other window it seems at first that it will only dock to the entire window. But that isn't really it. The tooltip info normally shown at the mouse cursor is not shown. That has to be shown _somewhere_ so it is probably out of window bounds. If I move that window even slightly it works correctly. While at the edge and it not working, the value for `x` in `area_docking_target` has a value that is not in that monitor. I drag into the very left part of where I would normally get "10" or so, I get 1400 or so - a value that would be inside the left monitor, But I do not think this is related to the calculations in `area_docking_target`. So far I think "win2" is not incorrect. But still investigating...

Harley Acheson commented

2024-09-16 22:52:00 +02:00

@Brainzman

Okay, that sample blend of yours was really interesting...

The "posx" and "posy" of the window containing Timeline is not correct. If you move or resize that window then Blender asks ghost and then corrects that window's position and size using wm_window_update_size_position.

When creating a new window that same function is called. But when restoring windows from a blend then that function is not called. I'm assuming that this window might have some other position but it placed where it is based on some limitation and those win members are not set right.

I can get that sample blend to work correctly with the following changes:

diff --git a/source/blender/windowmanager/intern/wm_window.cc b/source/blender/windowmanager/intern/wm_window.cc
index 82b6b16df08..0ab6a880dde 100644
--- a/source/blender/windowmanager/intern/wm_window.cc
+++ b/source/blender/windowmanager/intern/wm_window.cc
@@ -716,6 +716,8 @@ static void wm_window_ensure_eventstate(wmWindow *win)
   wm_window_update_eventstate(win);
 }
 
+static bool wm_window_update_size_position(wmWindow *win);
+
 /* Belongs to below. */
 static void wm_window_ghostwindow_add(wmWindowManager *wm,
                                       const char *title,
@@ -779,17 +781,12 @@ static void wm_window_ghostwindow_add(wmWindowManager *wm,
     wm_window_ensure_eventstate(win);
 
     /* Store actual window size in blender window. */
-    GHOST_RectangleHandle bounds = GHOST_GetClientBounds(
-        static_cast<GHOST_WindowHandle>(win->ghostwin));
-
     /* WIN32: gives undefined window size when minimized. */
     if (GHOST_GetWindowState(static_cast<GHOST_WindowHandle>(win->ghostwin)) !=
         GHOST_kWindowStateMinimized)
     {
-      win->sizex = GHOST_GetWidthRectangle(bounds);
-      win->sizey = GHOST_GetHeightRectangle(bounds);
+      wm_window_update_size_position(win);
     }
-    GHOST_DisposeRectangle(bounds);
 
 #ifndef __APPLE__
     /* Set the state here, so minimized state comes up correct on windows. */

Do you mind giving that a test before I make a PR?

@Brainzman Okay, that sample blend of yours was really interesting... The "posx" and "posy" of the window containing Timeline is not correct. If you move or resize that window then Blender asks ghost and then corrects that window's position and size using `wm_window_update_size_position`. When creating a new window that same function is called. But when restoring windows from a blend then that function is not called. I'm assuming that this window might have some other position but it placed where it is based on some limitation and those win members are not set right. I can get that sample blend to work correctly with the following changes: ```Diff diff --git a/source/blender/windowmanager/intern/wm_window.cc b/source/blender/windowmanager/intern/wm_window.cc index 82b6b16df08..0ab6a880dde 100644 --- a/source/blender/windowmanager/intern/wm_window.cc +++ b/source/blender/windowmanager/intern/wm_window.cc @@ -716,6 +716,8 @@ static void wm_window_ensure_eventstate(wmWindow *win) wm_window_update_eventstate(win); } +static bool wm_window_update_size_position(wmWindow *win); + /* Belongs to below. */ static void wm_window_ghostwindow_add(wmWindowManager *wm, const char *title, @@ -779,17 +781,12 @@ static void wm_window_ghostwindow_add(wmWindowManager *wm, wm_window_ensure_eventstate(win); /* Store actual window size in blender window. */ - GHOST_RectangleHandle bounds = GHOST_GetClientBounds( - static_cast<GHOST_WindowHandle>(win->ghostwin)); - /* WIN32: gives undefined window size when minimized. */ if (GHOST_GetWindowState(static_cast<GHOST_WindowHandle>(win->ghostwin)) != GHOST_kWindowStateMinimized) { - win->sizex = GHOST_GetWidthRectangle(bounds); - win->sizey = GHOST_GetHeightRectangle(bounds); + wm_window_update_size_position(win); } - GHOST_DisposeRectangle(bounds); #ifndef __APPLE__ /* Set the state here, so minimized state comes up correct on windows. */ ``` Do you mind giving that a test before I make a PR?

Jonas Holzman commented

2024-09-16 23:37:57 +02:00

@Harley Ah! Funnily enough I was just about to comment on not being able to reproduce the bug, even after debugging the posx/posy and sizex/sizey values in wm_window_ghostwindow_ensure, but suspecting the problem was close to this area.

I'm not completely able to fully dive into this right now, but a rough possible cause from intution, is that similarly to the previous docking bug we had on macOS, we're missing a conversion from native pixel/retina coordinates (so basically *2 or /2) which could make the relative window coordinate double or halve. I would need some to do some additional experiment to confirm this theory, but as I'm not able to reproduce this bug at all on the macOS machine I created the file one, I would guess that it's at least roughly related to this area (unimplement macOS window coordinates shenanigans). I'll try to open the blend on my Linux workstation tomorrow to see how it reacts, and depending on this I'll see if I can properly debug my way to the source of the problem.

Overall, while this is definitely one weird edge case, that's also definitely the kind of problems that we would need to fix by overhauling macOS window coordinates and adding proper multi-monitor support as we previously dicussed. Potentially, we could also eliminate an entire possible class of bug if we properly abstract the fact that macOS (and possibly other future GHOST backends) uses a different, native x2 pixel size coordinate system instead of having to manually insert getNativePixelSize in hard to predict places. This is definitely something that's on my todo and I hope to be able to concentrate on in the upcoming months. We even got some related user bug reports since then (see #126410 or #127038)

@Harley Ah! Funnily enough I was just about to comment on not being able to reproduce the bug, even after debugging the posx/posy and sizex/sizey values in `wm_window_ghostwindow_ensure`, but suspecting the problem was close to this area. I'm not completely able to fully dive into this right now, but a rough possible cause from intution, is that similarly to the previous docking bug we had on macOS, we're missing a conversion from native pixel/retina coordinates (so basically *2 or /2) which could make the relative window coordinate double or halve. I would need some to do some additional experiment to confirm this theory, but as I'm not able to reproduce this bug at all on the macOS machine I created the file one, I would *guess* that it's at least roughly related to this area (unimplement macOS window coordinates shenanigans). I'll try to open the blend on my Linux workstation tomorrow to see how it reacts, and depending on this I'll see if I can properly debug my way to the source of the problem. Overall, while this is definitely one weird edge case, that's also definitely the kind of problems that we would need to fix by overhauling macOS window coordinates and adding proper multi-monitor support as we previously dicussed. Potentially, we could also eliminate an entire possible class of bug if we properly abstract the fact that macOS (and possibly other future GHOST backends) uses a different, native x2 pixel size coordinate system instead of having to manually insert `getNativePixelSize` in hard to predict places. This is definitely something that's on my todo and I hope to be able to concentrate on in the upcoming months. We even got some related user bug reports since then (see #126410 or #127038)

Jonas Holzman commented

2024-09-16 23:53:27 +02:00

@Harley And regarding the patch you included, I can confirm that it run fines, but doesn't affect the layout restoration on macOS (possibly because the saved coordinates are already correct at this native pixel size like I suggested above).

However, as I said above, there might be a less work-aroundy way of fixing this issue (especially, since this seems more like macOS saving bogus coordinate than the WM doing something wrong upon loading the UI), I'm making quite a lot of assumptions here, which might be completely wrong since I can't really cross-check anything right now, but I'll try to look into the problem from a more macOS specific window coordinates point of view asap.

@Harley And regarding the patch you included, I can confirm that it run fines, but doesn't affect the layout restoration on macOS (possibly because the saved coordinates are already correct at this native pixel size like I suggested above). However, as I said above, there *might* be a less work-aroundy way of fixing this issue (especially, since this *seems* more like macOS saving bogus coordinate than the WM doing something wrong upon loading the UI), I'm making quite a lot of assumptions here, which might be completely wrong since I can't really cross-check anything right now, but I'll try to look into the problem from a more macOS specific window coordinates point of view asap.

Harley Acheson commented

2024-09-17 00:09:14 +02:00

The odd thing here is that is does just seem like a bug. wm_window_ghostwindow_add currently does call GHOST_GetWindowState but then only updates the win's sizex and sizey. Seems a bit odd to do that and not also update posx and posy. wm_window_update_size_position does both, but this isn't called when loading windows from a blend.

not being able to reproduce the bug

It was a pain to find it. In a nutshell I had set a breakpoint near the bottom of area_join_cursor (currently line 3900) where it returns a "move_cursor". With your blend file this is hit the moment you drag into Timeline from Image Editor. And here I noticed that jd->win2 was the correct window, but the positions of it were wrong. win->posx = 449 while win->posy = 142, which are completely unrelated to the position of that window. And doing anything with it instantly fixed those values. And of course with those values wrong the calculations for where in that window you are dragging is also bogus.

The odd thing here is that is does just seem like a bug. `wm_window_ghostwindow_add` currently does call `GHOST_GetWindowState` but then only updates the win's sizex and sizey. Seems a bit odd to do that and not also update posx and posy. `wm_window_update_size_position` does both, but this isn't called when loading windows from a blend. > not being able to reproduce the bug It was a pain to find it. In a nutshell I had set a breakpoint near the bottom of `area_join_cursor` (currently line 3900) where it returns a "move_cursor". With your blend file this is hit the moment you drag into Timeline from Image Editor. And here I noticed that jd->win2 was the correct window, but the positions of it were wrong. win->posx = 449 while win->posy = 142, which are completely unrelated to the position of that window. And doing anything with it instantly fixed those values. And of course with those values wrong the calculations for where in that window you are dragging is also bogus.

Harley Acheson commented

2024-09-17 21:28:13 +02:00

@Brainzman

For this specific issue, of the operator being re-run after a window is closed, I think this might be all we need:

diff --git a/source/blender/editors/screen/screen_ops.cc b/source/blender/editors/screen/screen_ops.cc
index 0280bbb6f62..0e12b11e457 100644
--- a/source/blender/editors/screen/screen_ops.cc
+++ b/source/blender/editors/screen/screen_ops.cc
@@ -4155,6 +4155,11 @@ static void area_join_cancel(bContext *C, wmOperator *op)
 /* modal callback while selecting area (space) that will be removed */
 static int area_join_modal(bContext *C, wmOperator *op, const wmEvent *event)
 {
+  if (event->type == WINDEACTIVATE) {
+    /* This operator can close windows, which can cause it to be re-run. */
+    return OPERATOR_FINISHED;
+  }
+
   if (op->customdata == nullptr) {
     if (!area_join_init(C, op, nullptr, nullptr)) {
       return OPERATOR_CANCELLED;

@Brainzman For this specific issue, of the operator being re-run after a window is closed, I _think_ this might be all we need: ```Diff diff --git a/source/blender/editors/screen/screen_ops.cc b/source/blender/editors/screen/screen_ops.cc index 0280bbb6f62..0e12b11e457 100644 --- a/source/blender/editors/screen/screen_ops.cc +++ b/source/blender/editors/screen/screen_ops.cc @@ -4155,6 +4155,11 @@ static void area_join_cancel(bContext *C, wmOperator *op) /* modal callback while selecting area (space) that will be removed */ static int area_join_modal(bContext *C, wmOperator *op, const wmEvent *event) { + if (event->type == WINDEACTIVATE) { + /* This operator can close windows, which can cause it to be re-run. */ + return OPERATOR_FINISHED; + } + if (op->customdata == nullptr) { if (!area_join_init(C, op, nullptr, nullptr)) { return OPERATOR_CANCELLED; ```

Jonas Holzman force-pushed fix-bad-docking-modal from cee80562d6 to db9a24d64d

2024-09-18 23:33:13 +02:00

Compare

Jonas Holzman commented

2024-09-18 23:39:10 +02:00

@Harley

The odd thing here is that is does just seem like a bug. wm_window_ghostwindow_add currently does call GHOST_GetWindowState but then only updates the win's sizex and sizey. Seems a bit odd to do that and not also update posx and posy. wm_window_update_size_position does both, but this isn't called when loading windows from a blend.

Taking a better look at the overall window restore logic your patch seems to actually perfectly address the issue, as unlike I originally thought above, window size/position adjustments are supposed to be performed on load, that's a very nice catch, and I would guess this fixes a more general issue of blend file not properly restoring when saved/restored at different native pixel sizes.

For this specific issue, of the operator being re-run after a window is closed, I think this might be all we need:

Ooh that's indeed a nicer solution than the hack I propose in this PR. However, I think it would still be nice to keep the operator as is, and try to instead fix the problem in the window manager. Like I said in our last meeting, this operator is not technically doing anything wrong, and since this is behavior that could theoretically be implemented by a user-defined Python operator, or a future screen/WM C++ modal operator for that matter, we also have a pretty bad, and veryy confusing general modal operator bug on our hands.

As such, and as I suggested as one of the possible solutions of this PR, the "proper" fix would then be to improve the condition in wm_event_system.cc by adapting it to actually check if the loaded blend file changes (as the comment above the condition suggests). I'll try to write up a solution for this soon, and I'd also first need to reproduce the reason why this workaround is in place in the first place (probably via the addon suggested in the comment). Also CC @ideasman42 I'd be interested to get your view on this.

Also, thinking twice about this, if we're in a rush to fix this issue for the area docking release, we might just as well merge this PR with your patch as a temporary fix, and make another PR that properly fixes this issue on the WM side later. I updated this PR with your patch in that sense.

@Harley > The odd thing here is that is does just seem like a bug. wm_window_ghostwindow_add currently does call GHOST_GetWindowState but then only updates the win's sizex and sizey. Seems a bit odd to do that and not also update posx and posy. wm_window_update_size_position does both, but this isn't called when loading windows from a blend. Taking a better look at the overall window restore logic your patch seems to actually perfectly address the issue, as unlike I originally thought above, window size/position adjustments are supposed to be performed on load, that's a very nice catch, and I would guess this fixes a more general issue of blend file not properly restoring when saved/restored at different native pixel sizes. > For this specific issue, of the operator being re-run after a window is closed, I think this might be all we need: Ooh that's indeed a nicer solution than the hack I propose in this PR. However, I think it would still be nice to keep the operator as is, and try to instead fix the problem in the window manager. Like I said in our last meeting, this operator is not technically doing anything wrong, and since this is behavior that could *theoretically* be implemented by a user-defined Python operator, or a future screen/WM C++ modal operator for that matter, we also have a pretty bad, and veryy confusing general modal operator bug on our hands. As such, and as I suggested as one of the possible solutions of this PR, the "proper" fix would then be to improve 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) by adapting it to actually check if the loaded blend file changes (as the comment above the condition suggests). I'll try to write up a solution for this soon, and I'd also first need to reproduce the reason why this workaround is in place in the first place (probably via the addon suggested in the comment). Also CC @ideasman42 I'd be interested to get your view on this. Also, thinking twice about this, if we're in a rush to fix this issue for the area docking release, we might just as well merge this PR with your patch as a temporary fix, and make another PR that properly fixes this issue on the WM side later. I updated this PR with your patch in that sense.

Harley Acheson commented

2024-09-19 00:07:37 +02:00

@Brainzman

Taking a better look at the overall window restore logic your patch seems to actually perfectly address the issue, as unlike I originally thought above, window size/position adjustments are supposed to be performed on load, that's a very nice catch, and I would guess this fixes a more general issue of blend file not properly restoring when saved/restored at different native pixel sizes.

It looks harmless at least so will probably do a bit more testing and do that soon.

I think it would still be nice to keep the operator as is, and try to instead fix the problem in the window manager.

I've just had no luck trying to fix this in that behavior in wm_event_system.cc.

we might just as well merge this PR with your patch as a temporary fix, and make another PR that properly fixes this issue on the WM side later. I updated this PR with your patch in that sense.

That makes sense. This check for WINDEACTIVATE is pretty harmless and should coexist (if it has to for a while) with a change to wm_event_system.cc.

Thanks so much for all your help with this.

@Brainzman > Taking a better look at the overall window restore logic your patch seems to actually perfectly address the issue, as unlike I originally thought above, window size/position adjustments are supposed to be performed on load, that's a very nice catch, and I would guess this fixes a more general issue of blend file not properly restoring when saved/restored at different native pixel sizes. It looks harmless at least so will probably do a bit more testing and do that soon. > I think it would still be nice to keep the operator as is, and try to instead fix the problem in the window manager. I've just had no luck trying to fix this in that behavior in wm_event_system.cc. > we might just as well merge this PR with your patch as a temporary fix, and make another PR that properly fixes this issue on the WM side later. I updated this PR with your patch in that sense. That makes sense. This check for WINDEACTIVATE is pretty harmless and should coexist (if it has to for a while) with a change to wm_event_system.cc. Thanks so much for all your help with this.

👍 1

Harley Acheson added 1 commit 2024-09-19 00:21:39 +02:00

Merge branch 'main' of projects.blender.org:blender/blender into pr126379

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

2221b6c743

Harley Acheson commented

2024-09-19 00:22:42 +02:00

@blender-bot build

Harley Acheson added this to the User Interface project 2024-09-19 00:24:06 +02:00

Harley Acheson changed title from ~~WIP Fix: Interactive Docking modal operator re-run glitch~~ to Fix: Interactive Docking modal operator re-run glitch

2024-09-19 00:38:22 +02:00

Harley Acheson merged commit aa5385f648 into main

2024-09-19 00:47:54 +02:00

Harley Acheson referenced this issue from a commit

2024-09-19 00:47:54 +02:00

Fix: Interactive Docking modal operator re-run glitch

Lukas Tönne referenced this issue from a commit

2024-09-23 08:17:12 +02:00

Fix: Interactive Docking modal operator re-run glitch

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

Fix: Interactive Docking modal operator re-run glitch #126379

Bug Description

Demo

Fix