Frame node upgrade and general node editor tweaks

A number of new features for the node editor in general and the Frame node in particular.
For an detailed user-level description of new features see the following blogpost:

http://code.blender.org/index.php/2012/05/node-editing-tweaks/

Known issues:

* The custom ui handler for mouse cursor feedback on border resize overrides changes made by the generic ED_screen_set_subwinactive function (called from wm_event_do_handlers). This means that approaching an area border from within the node editor does not update the cursor to the "move" cursor correctly:

  http://www.pasteall.org/pic/31569

  (It works when moving the other way, from 3D view to node editor here). Advice on proper solution would be appreciated.

* The default selected/active theme colors for nodes (same as object theme colors) should be set to the "Text Highlight" theme color for consistency. Requires change to startup.blend, which blows up patch size too much for codereview app, so will have to be done before trunk merge.

[mont29, 2012-05-16 20:09:20]

Hi Lukas,

All in all, patch looks good to me (and Frame nodes much nicer this way!). Note
though it’s my first review, so I hope I didn’t missed anything important.

Apart from the few minor glitches noted in comments, only “problem” I can see is
style code (spaces around operators, among others), but anyway that whole area
has not yet been cleaned up, so don’t think is really important for now.

About the “resize window cursor bug”, I can propose kind of a hack (see also
comments): instead of always defining cursor to CURSOR_STD, re-use current one
(win->cursor) if it’s set to CURSOR_X/Y_MOVE… Not sure it’s a really nice
solution, but at least it works! ;)

Cheers,
Bastien

PS: about name “Frame”, why not simply “Block”, or “Box” ?

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
File source/blender/editors/space_node/node_draw.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_draw.c:805: uiSetRoundBox(UI_CNR_ALL); //
round all corners except lower right
Can remove the comment, now! ;)

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_draw.c:955: int
node_get_resize_cursor(int directions)
proposed hack: add a second param, int default_cursor, and return it when dir ==
0

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_draw.c:972: int cursor = CURSOR_STD;
proposed hack: init cursor like this:

int cursor = ELEM(win->cursor, CURSOR_X_MOVE, CURSOR_Y_MOVE) ? win->cursor :
CURSOR_STD;

…

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_draw.c:988: cursor =
node_get_resize_cursor(dir);
proposed hack: … and replace this line by:

cursor = node_get_resize_cursor(dir, cursor);

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
File source/blender/editors/space_node/node_ops.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_ops.c:127: ot =
WM_operatortype_append_macro("NODE_OT_translate_attach", "Attach",
Not sure Attach is best… perhaps Move & Attach?

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_ops.c:134: ot =
WM_operatortype_append_macro("NODE_OT_detach_translate_attach", "Attach",
Label should be Detach (or Detach & Move ?)

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_ops.c:165: RNA_boolean_set(mot->ptr,
"release_confirm", TRUE);
This does not work (NODE_OT_translate_attach is itself a macro, and has no
"release_confirm" property).

Anyway, don’t think it’s useful, as it’s already set in translate_attach…

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
File source/blender/editors/space_node/space_node.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/space_node.c:360: static void
node_ui_handler_remove(bContext *UNUSED(C), void *UNUSED(userdata))
Don’t think you need this, seems you can just pass NULL to
WM_event_add_ui_handler.

http://codereview.appspot.com/6200062/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_userdef.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_userdef.c:1527: 
Selected Node would be nicer :p

[lukas.toenne, 2012-05-19 07:18:00]

Last comment failed due to chunk mismatch with trunk:

"Selected Node would be nicer :p"

Don't think so, "Selected Node" implies there is only one (like active node),
but it's really just a node state. I tried to follow the existing naming scheme
here, think it's ok ...

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
File source/blender/editors/space_node/node_draw.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_draw.c:805: uiSetRoundBox(UI_CNR_ALL); //
round all corners except lower right
On 2012/05/16 20:09:20, mont29 wrote:
> Can remove the comment, now! ;)

Done.

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_draw.c:955: int
node_get_resize_cursor(int directions)
On 2012/05/16 20:09:20, mont29 wrote:
> proposed hack: add a second param, int default_cursor, and return it when dir
==
> 0

Hmm, yeah, this would avoid overriding the cursor *if not hitting a node
border*. If the node border is hit, it would still override a potential resize
cursor from the window system.

A proper solution would be to do the window system even handling and cursor
update *after* the internal handlers, so  any changes made by the node editor
can be undone by the window border resize handler if necessary. I'll see if this
works and come back to it.

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_draw.c:972: int cursor = CURSOR_STD;
On 2012/05/16 20:09:20, mont29 wrote:
> proposed hack: init cursor like this:
> 
> int cursor = ELEM(win->cursor, CURSOR_X_MOVE, CURSOR_Y_MOVE) ? win->cursor :
> CURSOR_STD;
> 
> …

See above. If the "real" solution doesn't work this would still be a hacky
option.

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
File source/blender/editors/space_node/node_ops.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_ops.c:127: ot =
WM_operatortype_append_macro("NODE_OT_translate_attach", "Attach",
On 2012/05/16 20:09:20, mont29 wrote:
> Not sure Attach is best… perhaps Move & Attach?

Done.

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_ops.c:134: ot =
WM_operatortype_append_macro("NODE_OT_detach_translate_attach", "Attach",
On 2012/05/16 20:09:20, mont29 wrote:
> Label should be Detach (or Detach & Move ?)

Done.

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/node_ops.c:165: RNA_boolean_set(mot->ptr,
"release_confirm", TRUE);
On 2012/05/16 20:09:20, mont29 wrote:
> This does not work (NODE_OT_translate_attach is itself a macro, and has no
> "release_confirm" property).
> 
> Anyway, don’t think it’s useful, as it’s already set in translate_attach…

Done. Was a leftover when this was still a regular operator.

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
File source/blender/editors/space_node/space_node.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_nod...
source/blender/editors/space_node/space_node.c:360: static void
node_ui_handler_remove(bContext *UNUSED(C), void *UNUSED(userdata))
On 2012/05/16 20:09:20, mont29 wrote:
> Don’t think you need this, seems you can just pass NULL to
> WM_event_add_ui_handler.

Done.

[lukas.toenne, 2012-05-19 07:24:18]

Aforementioned minor fixes

[lukas.toenne, 2012-05-19 07:37:42]

Fix for mouse cursor problem

[lukas.toenne, 2012-05-19 07:41:41]

http://codereview.appspot.com/6200062/diff/4004/source/blender/windowmanager/...
File source/blender/windowmanager/intern/wm_event_system.c (right):

http://codereview.appspot.com/6200062/diff/4004/source/blender/windowmanager/...
source/blender/windowmanager/intern/wm_event_system.c:2079:
ED_screen_set_subwinactive(C, event);
Moving this function behind the screen area handlers allows it to override any
custom mouse cursors set by those handlers.

This might possibly have side effects, but seems to work nicely at first glance.

[lukas.toenne, 2012-05-19 07:53:09]

On 2012/05/19 07:41:41, lukas.toenne wrote:
>
http://codereview.appspot.com/6200062/diff/4004/source/blender/windowmanager/...
> File source/blender/windowmanager/intern/wm_event_system.c (right):
> 
>
http://codereview.appspot.com/6200062/diff/4004/source/blender/windowmanager/...
> source/blender/windowmanager/intern/wm_event_system.c:2079:
> ED_screen_set_subwinactive(C, event);
> Moving this function behind the screen area handlers allows it to override any
> custom mouse cursors set by those handlers.
> 
> This might possibly have side effects, but seems to work nicely at first
glance.

Forget about this one, it's not a good idea. The ED_screen_set_subwinactive does
a couple of things that are probably needed by screen area handlers. Actually
setting the mouse cursor might be moved behind those handlers however.

[lukas.toenne, 2012-05-20 09:44:22]

Proper mouse cursor fix by moving area border cursor setting after area handlers

[lukas.toenne, 2012-05-22 07:51:28]

Yet another way of setting the node resize cursor

[lukas.toenne, 2012-05-22 07:58:52]

Using a space area event handler is probably not a good way to update the mouse
cursor, so i came up with a different solution.

The are typeinfo now has an additional flag event_cursor, when set this causes
the cursor callback to be called on every mouse move, instead of just once when
entering the area. This avoids problems of overriding the default screen cursor
updates.

The problem with flickering cursor still persists. I've tracked it down to GHOST
level: using standard CURSOR_* types instead of custom BC_* cursors seems to
work nicely. Not sure what the reason is, it seems the custom bitmap cursors
shift their position slightly on mouse moves and intermediately set the standard
cursor or so ...