[Bf-codereview] Frame node upgrade and general node editor tweaks (issue 6200062)

[lukas.toenne at googlemail.com]

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_node/node_draw.c
File source/blender/editors/space_node/node_draw.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_node/node_draw.c#newcode805
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_node/node_draw.c#newcode955
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_node/node_draw.c#newcode972
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_node/node_ops.c
File source/blender/editors/space_node/node_ops.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_node/node_ops.c#newcode127
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_node/node_ops.c#newcode134
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_node/node_ops.c#newcode165
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_node/space_node.c
File source/blender/editors/space_node/space_node.c (right):

http://codereview.appspot.com/6200062/diff/1/source/blender/editors/space_node/space_node.c#newcode360
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.

http://codereview.appspot.com/6200062/