object_edit_linked addon review

[ideasman42, 2012-10-30 02:44:27]

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

[Fweeb, 2013-06-24 17:46:04]

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.