Clip editor: toolbar and sidebar allowed to open in graph and dopesheet mode #70522

New Issue

Alessio Monti di Sopra · 2019-10-04T16:01:07+02:00

2019-10-04 16:01:07 +02:00

System Information
Operating system: Linux-4.19.69-1-MANJARO-x86_64-with-arch-Manjaro-Linux 64 Bits
Graphics card: GeForce GT 740M/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 418.88

Blender Version
Broken: version: 2.80 (sub 75), branch: master, commit date: 2019-07-29 14:47, hash: f6cb5f5449

Short description of error

In the Clip editor, toolbar and sidebar are allowed to open also when in Graph or Dopesheet sub-mode, they open occupying the entire editor area and keep that size when going back to Clip mode.
@ideasman42 not sure, but I think this was caused by 6aef124e7d

**System Information** Operating system: Linux-4.19.69-1-MANJARO-x86_64-with-arch-Manjaro-Linux 64 Bits Graphics card: GeForce GT 740M/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 418.88 **Blender Version** Broken: version: 2.80 (sub 75), branch: master, commit date: 2019-07-29 14:47, hash: `f6cb5f5449` **Short description of error** In the Clip editor, toolbar and sidebar are allowed to open also when in Graph or Dopesheet sub-mode, they open occupying the entire editor area and keep that size when going back to Clip mode. @ideasman42 not sure, but I think this was caused by 6aef124e7d

Alessio Monti di Sopra commented

2019-10-04 16:01:07 +02:00

Added subscribers: @ideasman42, @a.monti

Germano Cavalcante commented

2019-10-07 16:10:15 +02:00

Added subscriber: @mano-wii

Julian Eisel was assigned by Germano Cavalcante

2019-10-07 16:10:15 +02:00

Germano Cavalcante commented

2019-10-07 16:10:15 +02:00

I can confirm.
It seems that Sidebar (shortcut N) totally breaks the interface of Movie Clip Editor.
Not sure if the sidebar is empty or broken.

Cc @ideasman42

I can confirm. It seems that Sidebar (shortcut N) totally breaks the interface of Movie Clip Editor. Not sure if the sidebar is empty or broken. Cc @ideasman42

ChristopherAnderssarian commented

2019-10-07 16:21:45 +02:00

Added subscriber: @ChristopherAnderssarian

Julian Eisel commented

2019-10-07 18:59:11 +02:00

This indeed seems to be an issue from 6aef124e7d. For the clip editor, the regions are supposed to be "unavailable" by hiding them and setting the region alignment to NONE.

This should be a reasonable fix:

diff --git a/source/blender/makesrna/intern/rna_space.c b/source/blender/makesrna/intern/rna_space.c
index 52aca9bdea5..187ec958cc0 100644
--- a/source/blender/makesrna/intern/rna_space.c
+++ b/source/blender/makesrna/intern/rna_space.c
@@ -589,7 +589,7 @@ static void rna_Space_bool_from_region_flag_set_by_type(PointerRNA *ptr,
 {
   ScrArea *sa = rna_area_from_space(ptr);
   ARegion *ar = BKE_area_find_region_type(sa, region_type);
-  if (ar) {
+  if (ar && ar->alignment != RGN_ALIGN_NONE) {
     SET_FLAG_FROM_TEST(ar->flag, value, region_flag);
   }
   ED_region_tag_redraw(ar);

@ideasman42 I do wonder though, why did you go through all the hoops needed to get region toggling to work through RNA properties? In fact, I added a general region toggle operator recently, which allows passing the region type as OP property, see SCREEN_OT_region_toggle. Couldn't we use that and keep things simple in RNA? Otherwise we have to add these RNA properties for every region type that may be toggleable.

This indeed seems to be an issue from 6aef124e7d. For the clip editor, the regions are supposed to be "unavailable" by hiding them and setting the region alignment to `NONE`. This should be a reasonable fix: ``` diff --git a/source/blender/makesrna/intern/rna_space.c b/source/blender/makesrna/intern/rna_space.c index 52aca9bdea5..187ec958cc0 100644 --- a/source/blender/makesrna/intern/rna_space.c +++ b/source/blender/makesrna/intern/rna_space.c @@ -589,7 +589,7 @@ static void rna_Space_bool_from_region_flag_set_by_type(PointerRNA *ptr, { ScrArea *sa = rna_area_from_space(ptr); ARegion *ar = BKE_area_find_region_type(sa, region_type); - if (ar) { + if (ar && ar->alignment != RGN_ALIGN_NONE) { SET_FLAG_FROM_TEST(ar->flag, value, region_flag); } ED_region_tag_redraw(ar); ``` @ideasman42 I do wonder though, why did you go through all the hoops needed to get region toggling to work through RNA properties? In fact, I added a general region toggle operator recently, which allows passing the region type as OP property, see `SCREEN_OT_region_toggle`. Couldn't we use that and keep things simple in RNA? Otherwise we have to add these RNA properties for every region type that may be toggleable.

blender-admin commented

2019-10-08 10:29:27 +02:00

This issue was referenced by 95749f5d54

This issue was referenced by 95749f5d548c9cd304b2f8399c791c98e431a19e

Julian Eisel commented

2019-10-08 10:35:59 +02:00

Changed status from 'Open' to: 'Resolved'

Julian Eisel closed this issue

2019-10-08 10:35:59 +02:00

Julian Eisel commented

2019-10-08 10:45:39 +02:00

Committed my fix now, altough my question still persists :)

Germano Cavalcante commented

2019-10-08 14:35:19 +02:00

Let me get this straight.
The idea is to remove properties that indicate if the region of an editor is visible, and use a generic operator to hide or display them.
On the one hand this can diminish the python programmer's freedom to know if a region is visible or not.
But if I'm not mistaken, there are other ways to know if a region is visible in python.

If this is the case, in my opinion it is better to use an operator.

Let me get this straight. The idea is to remove properties that indicate if the region of an editor is visible, and use a generic operator to hide or display them. On the one hand this can diminish the python programmer's freedom to know if a region is visible or not. But if I'm not mistaken, there are other ways to know if a region is visible in python. If this is the case, in my opinion it is better to use an operator.

Julian Eisel commented

2019-10-08 14:49:34 +02:00

For .py access convenience, we can also add a Space.is_region_visible('TOOL_HEADER').

I just find it odd that we are using RNA properties to execute an action, when we can easily do it with an operator. Even if we had individual readonly Space.is_region_foo_visible checks, that would be fine, but mixing up purposes of RNA properties and operators, meeh, I'd rather avoid that.

For .py access convenience, we can also add a `Space.is_region_visible('TOOL_HEADER')`. I just find it odd that we are using RNA properties to execute an action, when we can easily do it with an operator. Even if we had individual readonly `Space.is_region_foo_visible` checks, that would be fine, but mixing up purposes of RNA properties and operators, meeh, I'd rather avoid that.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Clip editor: toolbar and sidebar allowed to open in graph and dopesheet mode #70522