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

[jason at monsterjavaguns.com]

The add-on has changed a little bit since this review (contributions
from Pablo Vazquez and Bassam Kurdali), but all of the comments have
been addressed and committed to SVN.

There's a slight issue with Pablo's update (adds support for launching a
second Blender instance) in that it allows for nested links whereas the
single-instance option does not. I'm not sure about the best way to
handle this. I don't think that the linked file knows whether or not
it's been opened in a new instance of Blender or not.


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

https://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)}:
On 2012/10/30 02:44:27, ideasman42 wrote:
> no need to check bpy.path.abspath() here, filepaths are always
absolute.

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

Done.

https://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode48
object_edit_linked.py:48: bpy.data.objects[ob_name].select = True
On 2012/10/30 02:44:27, ideasman42 wrote:
> 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.

Added a comment. As a future revision, I can set the correct scene for
the linked object/group.

https://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)
On 2012/10/30 02:44:27, ideasman42 wrote:
> style: for blender addons we split these up into multiple lines.

Done.

https://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)
On 2012/10/30 02:44:27, ideasman42 wrote:
> 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.

Without auto-save, losing work is exactly what happens. This is why it's
enabled by default. Since this initial code review Pablo Vazquez also
added the ability to launch a new instance of Blender as an additional
option. That works as you describe, but the single instance is still the
default behavior.

https://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])
On 2012/10/30 02:44:27, ideasman42 wrote:
> 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}

Done.

https://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)
On 2012/10/30 02:44:27, ideasman42 wrote:
> style: split multi-line as before

Done.

https://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode109
object_edit_linked.py:109: # Probably the wrong context to check for
here...
On 2012/10/30 02:44:27, ideasman42 wrote:
> poll probably doesnt make sense here, shouldnt poll() check if
> (settings["original_file"] != "") ?

Done.

https://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode137
object_edit_linked.py:137: kmi_edit.active = True
On 2012/10/30 02:44:27, ideasman42 wrote:
> 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.

I wasn't aware that keymaps could be easily overloaded this way (also, I
had a really poor poll function for the Edit Linked operator. It now
works correctly. Done.

https://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
On 2012/10/30 02:44:27, ideasman42 wrote:
> 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.

Done.

https://codereview.appspot.com/6815051/diff/1/object_edit_linked.py#oldcode178
object_edit_linked.py:178: kc =
bpy.context.window_manager.keyconfigs.addon
On 2012/10/30 02:44:27, ideasman42 wrote:
> 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

Done.

https://codereview.appspot.com/6815051/