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

[montagne29 at wanadoo.fr]

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_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
Can remove the comment, now! ;)

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)
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_node/node_draw.c#newcode972
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_node/node_draw.c#newcode988
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_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",
Not sure Attach is best… perhaps Move & Attach?

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",
Label should be Detach (or Detach & Move ?)

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);
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_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))
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/rna_userdef.c
File source/blender/makesrna/intern/rna_userdef.c (right):

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

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