Fix #108763: temp_override to consider context's screen #114269

Andrej · 2023-10-30T13:22:47+01:00

Andrej commented

2023-10-30 13:22:47 +01:00

Previously it was checking the actually active screen - it was leading to the issue where it's no longer possible to override context.screen (#108763) which was possible with old context override method.

Issue mechanics - when operator is called with area and screen overriden it throws TypeError: Area not found in screen since it's area do not belong to the currently active screen

.screen override is required if you want to use some operator that's using area that's not available on the current screen. Most important case for me personally is bpy.ops.node.clipboard_copy because that's basically the only way to copy nodes from one shader tree to another.

Previously it was checking the actually active screen - it was leading to the issue where it's no longer possible to override `context.screen` (#108763) which was possible with old context override method. Issue mechanics - when operator is called with `area` and `screen` overriden it throws `TypeError: Area not found in screen` since it's `area` do not belong to the currently active `screen` `.screen` override is required if you want to use some operator that's using area that's not available on the current screen. Most important case for me personally is `bpy.ops.node.clipboard_copy` because that's basically the only way to copy nodes from one shader tree to another.

Andrej added 1 commit 2023-10-30 13:23:01 +01:00

f7e39daef4 temp_override to consider context's screen #108763

Previously it was checking the actually active screen - it was leading to issue where it's no longer possible to override `context.screen` ( #108763). With old context override method it was possible to override `context.screen`.

Iliya Katushenock changed title from ~~temp_override to consider context's screen #108763~~ to Fix #108763: temp_override to consider context's screen

2023-10-30 13:25:31 +01:00

Iliya Katushenock added the

Module

Python API

label 2023-10-30 13:25:50 +01:00

Andrej requested review from Campbell Barton 2023-10-30 16:37:46 +01:00

Campbell Barton commented

2023-11-06 11:38:14 +01:00

We should be able to support something like this but there is an aspect of this patch which I'm not so keen on.

I'm concerned that setting the screen, leaves the screen returned by WM_window_get_active_screen unchanged. Since the screen returned by the window is used quite a lot, I think it's error prone that Blender is left (even temporarily) in a state where only some functions will return the overridden screen.

Bigger picture, my aim with the temporary overrides was to ensure an inconsistent state is never allowed (so an active area is always part of the screen for e.g. so states that are likely to confuse lower level logic - which may crash, are never allowed).
This PR allows to set an active screen which isn't used by the current window which violates this principle.
Of course and argument can always be made that in this particular case it's OK... but I'm wary of allowing the windows screen and the contexts screen to get out of sync.

A solution could be to temporarily override the screen used by the active window. This also has some complications* but in general I think it's going to work predictably and cause the least amount of problems long term.

Before requesting changes to this PR it would be good to get a second opinion (maybe @mont29 ?).

* The main complication as far as I can see is when the overriding screen is already used by a window. Currently it's assumed each screen is only ever instanced once, so we need to check if it's acceptable to make a temporary exception to this rule. An alternative could be to set that other window using the requested screen active.

We should be able to support something like this but there is an aspect of this patch which I'm not so keen on. I'm concerned that setting the screen, leaves the screen returned by `WM_window_get_active_screen` unchanged. Since the screen returned by the window is used quite a lot, I think it's error prone that Blender is left (even temporarily) in a state where only some functions will return the overridden screen. Bigger picture, my aim with the temporary overrides was to ensure an inconsistent state is never allowed (so an active area is always part of the screen for e.g. so states that are likely to confuse lower level logic - which may crash, are never allowed). This PR allows to set an active screen which isn't used by the current window which violates this principle. Of course and argument can always be made that in this particular case it's OK... but I'm wary of allowing the windows screen and the contexts screen to get out of sync. A solution could be to temporarily override the screen used by the active window. This also has some complications\* but in general I think it's going to work predictably and cause the least amount of problems long term. Before requesting changes to this PR it would be good to get a second opinion (maybe @mont29 ?). ---- \* The main complication as far as I can see is when the overriding screen is already used by a window. Currently it's assumed each screen is only ever instanced once, so we need to check if it's acceptable to make a temporary exception to this rule. An alternative could be to set that other window using the requested screen active.

Campbell Barton requested review from Bastien Montagne 2023-11-06 11:48:19 +01:00

Bastien Montagne commented

2023-11-06 15:33:31 +01:00

I would indeed not allow a mismatch between the window and the screen in the context passed to operators and underlying code.

Overriding the active screen of the active window (or making another window active if given screen is already used by it) sounds like a reasonable idea to me.

I would indeed not allow a mismatch between the window and the screen in the context passed to operators and underlying code. Overriding the active screen of the active window (or making another window active if given screen is already used by it) sounds like a reasonable idea to me.

Andrej added 2 commits 2023-11-07 12:42:47 +01:00

e2ae3e5246 Revert "temp_override to consider context's screen #108763 "

This reverts commit f7e39daef4.

6d945cee49 Temporarily changing window's screen if it was overridden

Andrej commented

2023-11-07 12:44:17 +01:00

Found another issue with the first approach - getting context items like context.active_object (context doesn't have attribute active_object...) was resulting in errors since ctx_data_get is using screen->context to get some context items and screen is not active, then screen->context is null, resulting in those errors.

Added changes. I've used similar approach with ContextStore ctx_temp - if the window's screen doesn't match the screen from the temp_override, then it switches the screen and switches it back on bpy_rna_context_temp_override_exit. It's also considering that if the temp_override.window's screen is matching temp_override.screen already then there is no need to switch it back later since windows screens basically were untouched.
Initially I thought I could just change active screen with something like WM_window_set_active_screen but since screens are connected to their workspaces I've switched the workspaces.

I've run some tests I've had issues with before:

tests code in python

import bpy


def get_shader_editor_context():
    for screen in bpy.data.screens:
        for area in screen.areas:
            if area.type == "NODE_EDITOR":
                for space in area.spaces:
                    if space.tree_type == "ShaderNodeTree":
                        context_override = {
                            "area": area,
                            "screen": screen,
                        }
                        return context_override


def test_1():
    # basically smoke test
    temp_override = get_shader_editor_context()
    with bpy.context.temp_override(**temp_override):
        print("during test_1")
    print("test_1 finished")


def test_2():
    def get_console_context():
        screen = bpy.data.screens["Scripting"]
        area = next(area for area in screen.areas if area.type == "CONSOLE")
        region = next(region for region in area.regions if region.type == "WINDOW")
        context_override = {"area": area, "screen": screen, "region": region}
        return context_override

    context = get_console_context()
    with bpy.context.temp_override(**context):
        bpy.ops.console.scrollback_append(text="test_temp_override", type="ERROR")
    print("test 2 finished")


def test_3():
    temp_override = get_shader_editor_context()
    material = bpy.context.active_object.active_material

    # change current material and make other window's shader editor is updated
    with bpy.context.temp_override(**temp_override):
        # select all nodes and copy them to clipboard
        for node in material.node_tree.nodes:
            node.select = True
        bpy.ops.node.clipboard_copy()

        # remove all nodes from the current material
        for n in material.node_tree.nodes[:]:
            material.node_tree.nodes.remove(n)
        print(f"Nodes removed: {len(material.node_tree.nodes)}")

        bpy.ops.node.clipboard_paste(offset=(0, 0))

        print(f"Nodes pasted: {len(material.node_tree.nodes)}")

    print("test 3 finished")

# supposed to be started from text editor in any tabs besides Scripting/Shading
test_1() # smoke test, should work without errors
test_2() # should print text in python console in Scripting
test_3() # should print Nodes removed: 0. Nodes pasted: 2

Found another issue with the first approach - getting context items like `context.active_object` (`context` doesn't have attribute `active_object`...) was resulting in errors since `ctx_data_get` is using `screen->context` to get some context items and `screen` is not active, then `screen->context` is null, resulting in those errors. Added changes. I've used similar approach with `ContextStore ctx_temp` - if the window's screen doesn't match the screen from the `temp_override`, then it switches the screen and switches it back on `bpy_rna_context_temp_override_exit`. It's also considering that if the temp_override.window's screen is matching temp_override.screen already then there is no need to switch it back later since windows screens basically were untouched. Initially I thought I could just change active screen with something like `WM_window_set_active_screen` but since screens are connected to their workspaces I've switched the workspaces. I've run some tests I've had issues with before: <details> <summary>tests code in python</summary> ```python import bpy def get_shader_editor_context(): for screen in bpy.data.screens: for area in screen.areas: if area.type == "NODE_EDITOR": for space in area.spaces: if space.tree_type == "ShaderNodeTree": context_override = { "area": area, "screen": screen, } return context_override def test_1(): # basically smoke test temp_override = get_shader_editor_context() with bpy.context.temp_override(**temp_override): print("during test_1") print("test_1 finished") def test_2(): def get_console_context(): screen = bpy.data.screens["Scripting"] area = next(area for area in screen.areas if area.type == "CONSOLE") region = next(region for region in area.regions if region.type == "WINDOW") context_override = {"area": area, "screen": screen, "region": region} return context_override context = get_console_context() with bpy.context.temp_override(**context): bpy.ops.console.scrollback_append(text="test_temp_override", type="ERROR") print("test 2 finished") def test_3(): temp_override = get_shader_editor_context() material = bpy.context.active_object.active_material # change current material and make other window's shader editor is updated with bpy.context.temp_override(**temp_override): # select all nodes and copy them to clipboard for node in material.node_tree.nodes: node.select = True bpy.ops.node.clipboard_copy() # remove all nodes from the current material for n in material.node_tree.nodes[:]: material.node_tree.nodes.remove(n) print(f"Nodes removed: {len(material.node_tree.nodes)}") bpy.ops.node.clipboard_paste(offset=(0, 0)) print(f"Nodes pasted: {len(material.node_tree.nodes)}") print("test 3 finished") # supposed to be started from text editor in any tabs besides Scripting/Shading test_1() # smoke test, should work without errors test_2() # should print text in python console in Scripting test_3() # should print Nodes removed: 0. Nodes pasted: 2 ``` </details>

Campbell Barton requested changes 2023-11-10 03:47:33 +01:00

Campbell Barton left a comment

Not full review, this seems the right direction though.

New screen logic should be added between window & area logic so the order is always: window -> screen -> area -> region.

Not full review, this seems the right direction though. New screen logic should be added between `window` & `area` logic so the order is always: `window -> screen -> area -> region`.

source/blender/python/intern/bpy_rna_context.cc Outdated

						
				@ -83,3 +86,3 @@

				  self->ctx_init.region_is_set = (self->ctx_init.region != region);

				  bScreen *screen = win ? WM_window_get_active_screen(win) : nullptr;

				  // overridding screen is a bit different since

Campbell Barton commented

2023-11-10 03:43:34 +01:00

*picky* use C-style comments, full sentences.

\*picky\* use C-style comments, full sentences.

Andrej commented

2023-11-10 10:48:46 +01:00

Fixed.

source/blender/python/intern/bpy_rna_context.cc Outdated

						
				@ -391,2 +423,4 @@

				    ctx_temp.region_is_set = true;

				  }

				  if (params.screen.ptr != nullptr) {

				    ctx_temp.screen = static_cast<bScreen *>(params.screen.ptr->data);

Campbell Barton commented

2023-11-10 03:43:18 +01:00

Temporary screens screen->temp != 0 screens should fail with an exception. Perhaps they could be supported but for this patch it's simplest not to.

Temporary screens `screen->temp != 0` screens should fail with an exception. Perhaps they could be supported but for this patch it's simplest not to.

Andrej commented

2023-11-10 10:48:37 +01:00

Added an exception.

Andrej added 2 commits 2023-11-10 10:35:08 +01:00

1198a627e4 temp_override screen - exception on overriding with temp screen

2b858f84c7 temp_override screen - comments formatting and logic rearranged

- reformatted comments
- rearranged part of screen logic

Andrej commented

2023-11-10 11:01:50 +01:00

New screen logic should be added between window & area logic so the order is always: window -> screen -> area -> region.

Rearranged the code a bit but self->ctx_init.screen = WM_window_get_active_screen(win); is still not part of other self->ctx_init.___ = ___ block since it's need to know what win is going to be used.:

If window and screen are overridden then we're possibly changing workspace for some other window, so we'll need to store the original screen for that particular window in self->ctx_init.screen, therefore we need to know the exact used window first.

> New screen logic should be added between `window` & `area` logic so the order is always: `window -> screen -> area -> region`. Rearranged the code a bit but `self->ctx_init.screen = WM_window_get_active_screen(win);` is still not part of other `self->ctx_init.___ = ___` block since it's need to know what `win` is going to be used.: If window and screen are overridden then we're possibly changing workspace for some other window, so we'll need to store the original screen for that particular window in `self->ctx_init.screen`, therefore we need to know the exact used window first.

Andrej requested review from Campbell Barton 2023-11-10 11:01:56 +01:00

Andrej added 1 commit 2023-11-10 11:11:20 +01:00

d0677648c1 temp_override screen - rearrange logic

Andrej commented

2023-11-23 09:50:09 +01:00

@ideasman42 anything to change or looks good?

Campbell Barton commented

2023-12-06 10:48:37 +01:00

Committed 6af92e1360 with some edits.

Split out screen spesific checks into their own blocks.
Added additional checks for invalid cases.

Committed 6af92e13607bde4cdaccba0f280e7a5219725cbf with some edits. - Split out screen spesific checks into their own blocks. - Added additional checks for invalid cases.

Campbell Barton closed this pull request

2023-12-06 10:48:42 +01:00

Campbell Barton referenced this issue from a commit

2023-12-06 10:50:06 +01:00

PyAPI: support overriding the "screen" with Context.temp_override()

Andrej commented

2023-12-08 10:04:56 +01:00

@ideasman42 hi! thanks for merging this!

Any ideas of good place in tests/python to add some tests for context.temp_override?

And I think I've found a bug - it occurs if we override both window and screen and overridden window's screen is never switched back. Reported it separately - #115937

@ideasman42 hi! thanks for merging this! Any ideas of good place in [tests/python](https://projects.blender.org/blender/blender/src/branch/main/tests/python) to add some tests for `context.temp_override`? And I think I've found a bug - it occurs if we override both window and screen and overridden window's screen is never switched back. Reported it separately - #115937

Dalai Felinto commented

2023-12-08 11:22:24 +01:00

@Andrej730 better to report the bug as a new Bug Report (while mentioning this pull request #114269), so it can be better processed

👍 1

Andrej referenced this pull request

2023-12-08 12:42:36 +01:00

Regression: Context override causes pernament changes #115937