[Bf-codereview] object_edit_linked addon review (issue 6815051)

[ideasman42 at gmail.com]

Reviewers: jason_handturkeystudios.com, ideasman42,  
bf-codereview_blender.org,

Message:
initial review of Jason van Gumster's library linking addon.


http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py
File object_edit_linked.py (left):

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode44
object_edit_linked.py:44: if settings["linked_file"] in
{bpy.data.filepath, bpy.path.abspath(bpy.data.filepath)}:
no need to check bpy.path.abspath() here, filepaths are always absolute.

Should however not compare strings likle this, use os.path.samefile().

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode48
object_edit_linked.py:48: bpy.data.objects[ob_name].select = True
The object may not be in the active scene, maybe its OK to assume it is,
but take care here.

In this case selecting will just do nothing.
Same with setting active - below.

At least worth commenting here I think.

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode63
object_edit_linked.py:63: autosave = bpy.props.BoolProperty(name =
"Autosave", description = "Automatically save the current file before
opening the linked library", default = True)
style: for blender addons we split these up into multiple lines.

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode63
object_edit_linked.py:63: autosave = bpy.props.BoolProperty(name =
"Autosave", description = "Automatically save the current file before
opening the linked library", default = True)
If you don't auto-save, what happens? - loose your work? ... realize
this is tricky, but have to take care the user doesn't leave this off by
accident.

This is one reason my addon that did this (which was far more
primitive), didnt attempt to manage the save/reload stuff, It just
opened the library in another blend and let the user do the save/reload.

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode75
object_edit_linked.py:75: settings["linked_objects"].extend([ob.name for
ob in target.dupli_group.objects])
Its common to have an object show up multiple times in a dupli-group.

Using a set will de-duplicate for you,
[ob.name for ob in target.dupli_group.objects]

can be a set...
{ob.name for ob in target.dupli_group.objects}

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode105
object_edit_linked.py:105: autosave = bpy.props.BoolProperty(name =
"Autosave", description = "Automatically save the current file before
opening original file", default = True)
style: split multi-line as before

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode109
object_edit_linked.py:109: # Probably the wrong context to check for
here...
poll probably doesnt make sense here, shouldnt poll() check if
(settings["original_file"] != "") ?

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode137
object_edit_linked.py:137: kmi_edit.active = True
enable/disable keymaps from within a draw function is not good, What
happens if the panel happens to be hidden and not draw?

Best not enable disable during the duration of the addons operation,
instead just always keep enabled, and have the operators they call fail
to poll() on the wrong context.

- This is what is done in the rest of blender.

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode139
object_edit_linked.py:139:
self.layout.operator("object.edit_linked").autosave =
context.scene.edit_linked_autosave
normally 'scene' is assigned a variable, instead of referecing
'context.scene' multiple times, these lookups take time and its nice to
avoid too many, especially for draw functiuons.

http://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode178
object_edit_linked.py:178: kc =
bpy.context.window_manager.keyconfigs.addon
if the new 'km' is stored on registering, it can simply be removed,
without removing its items, See keymap example (at the bottom):
https://github.com/ideasman42/blender_addon_tutorial/blob/master/readme.rst



Please review this at http://codereview.appspot.com/6815051/

Affected files:
   M     object_edit_linked.py


Index: object_edit_linked.py
===================================================================
--- object_edit_linked.py	(revision 3901)
+++ object_edit_linked.py	(working copy)
@@ -183,3 +183,4 @@

  if __name__ == "__main__":
      register()
+