I will ask Campbell for review. If he reviews and say that it's ok - can I treat this as "You have a permission to move it to trunk"?
Thank you. Cheers Bartek Skorupa www.bartekskorupa.com On 26 mar 2013, at 17:01, Bastien Montagne <montagn...@wanadoo.fr> wrote: > Yes, imho it's good for addons/ dir. Would be cool if you could get > Campbell (ideasman_42) to review it too in the next days, else just move > it! :) > > Bastien > > On 26/03/2013 16:54, Bartek Skorupa (priv) wrote: >> @ Bastien >> Changed the code according to your guidelines. >> >> Is the script now ok to go to trunk? >> Thank you for your help. >> >> Cheers, >> Bartek Skorupa >> >> www.bartekskorupa.com >> >> On 25 mar 2013, at 17:33, Bartek Skorupa >> (priv)<bartekskor...@bartekskorupa.com> wrote: >> >>> Hey, >>> @CoDEmanX: >>> I don't quite get what you mean by: >>>> But please don't do this if the amount of props varies (prop1-propN)! >>> Could you elaborate, please? >>> >>> >>> Bartek Skorupa >>> >>> www.bartekskorupa.com >>> >>> On 25 mar 2013, at 17:19, CoDEmanX<codem...@gmx.de> wrote: >>> >>>> Proper formatting: >>>> >>>> props = layout.operator(NodesLinkActiveToSelected.bl_idname, >>>> text="Replace Links") >>>> >>>> props.prop1 = True >>>> props.prop2 = True >>>> props.prop3 = False >>>> >>>> But please don't do this if the amount of props varies (prop1-propN)! >>>> Use a CollectionProperty instead. I think i did this in the Game >>>> Property Visulizer addon to replace the evil eval(). >>>> >>>> >>>> Am 25.03.2013 16:08, schrieb Bartek Skorupa (priv): >>>>> Thank you for the explanation. >>>>>> props = layout.operator(NodesLinkActiveToSelected.bl_idname, >>>>>> text="Replace Links").prop1 = True >>>>>> props.prop2 = True >>>>>> props.prop3 = False >>>>> Oops… I didn't know you can do it :-) Really… >>>>> >>>>> Every day I learn something. >>>>> I will change all those properties accordingly. >>>>> >>>>> Thank you once again >>>>> >>>>> Regards >>>>> >>>>> Bartek Skorupa >>>>> >>>>> www.bartekskorupa.com >>>>> >>>>> On 25 mar 2013, at 16:02, Bastien Montagne<montagn...@wanadoo.fr> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> On 25/03/2013 15:51, Bartek Skorupa (priv) wrote: >>>>>>> Thank you for this quick review. >>>>>>> Yes you're right about my understanding of bl_label. I did mismatch >>>>>>> this and I can change it, np. >>>>>>> >>>>>>> About my 'option' properties: >>>>>>> In many cases I can change it to enums, but sometimes StringProperty is >>>>>>> used because I need to pass more than one property in one go. >>>>>>> Say I need to set 3 properties: >>>>>>> prop1 = True >>>>>>> prop2 = True >>>>>>> prop3 = False >>>>>>> >>>>>>> I wrap it in one StringProperty that gets the value of 'True True >>>>>>> False' and then in execute I use string split( ), and eval to get 3 >>>>>>> booleans. >>>>>>> >>>>>>> Please take a look at line 1325 for example: >>>>>>> layout.operator(NodesLinkActiveToSelected.bl_idname, text="Replace >>>>>>> Links").option = 'True True False' >>>>>>> >>>>>>> option then is passed to execute of NodesLinkActiveToSelected and split. >>>>>>> see lines: 795 to 798: >>>>>>> option_split = self.option.split( ) >>>>>>> replace = eval(option_split[0]) >>>>>>> use_node_name = eval(option_split[1]) >>>>>>> use_outputs_names = eval(option_split[2]) >>>>>>> >>>>>>> This way I get 3 variables out of one property 'option' >>>>>>> >>>>>>> Is there a better way of doing it? >>>>>> Yes there is! :) >>>>>> >>>>>> In such case, you should do that: >>>>>> >>>>>> props = layout.operator(NodesLinkActiveToSelected.bl_idname, >>>>>> text="Replace Links").prop1 = True >>>>>> props.prop2 = True >>>>>> props.prop3 = False >>>>>> >>>>>>> Another question: >>>>>>> Could you please explain me why using StringProperty instead of proper >>>>>>> EnumProperty is wrong? >>>>>>> Is it a convention, speed issues or anything else? >>>>>> First of all, it is a convention. True/false options should be booleans, >>>>>> options with a well defined set of choices should be enums, etc. >>>>>> >>>>>> This also helps on documentation level (as each enum items has its own >>>>>> label/description, you do not need comments in code, and doc can be >>>>>> retrieved easily by multiple ways). >>>>>> >>>>>> And there is also an UI interest, as using enums you can get a nice >>>>>> drop-down list as a control, or you can even auto-generate a menu where >>>>>> each entry will execute the operator with one of the options of the >>>>>> enum, … >>>>>> >>>>>> By the way, note that if you have a set of related booleans options that >>>>>> are not mutually exclusive, you can use an enum with 'ENUM_FLAG' option >>>>>> (in that case you are just limited to at most 32 different flags - the >>>>>> length of a classical integer ;) ). >>>>>> >>>>>> Kind regards, >>>>>> Bastien >>>>>> >>>>>>> Thank you in advance >>>>>>> >>>>>>> Regards >>>>>>> >>>>>>> Bartek Skorupa >>>>>>> >>>>>>> www.bartekskorupa.com >>>>>>> >>>>>>> On 25 mar 2013, at 15:24, Bastien Montagne<montagn...@wanadoo.fr> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi Bartek, >>>>>>>> >>>>>>>> Just did a (very quick) skim review of your addon, and I agree that >>>>>>>> there are valuable features in it, worth moving it to trunk. However, I >>>>>>>> noted at least two points that imho should be addressed before the >>>>>>>> move: >>>>>>>> >>>>>>>> First, you seems to mismatch labels and tips of operators! E.g. instead >>>>>>>> of this line: >>>>>>>> >>>>>>>> bl_label = "Copy Settings of Active Node to Selected Nodes" >>>>>>>> >>>>>>>> I would rather see those: >>>>>>>> >>>>>>>> bl_label = "Copy Node Settings" >>>>>>>> bl_description = "Copy the settings of the active node to selected >>>>>>>> ones" >>>>>>>> >>>>>>>> The second point is, I think, more important. You should replace your >>>>>>>> “multipurpose” "option" StringProperty by relevant properties. E.g. >>>>>>>> >>>>>>>> # option: 'from active', 'from node', 'from socket' >>>>>>>> option = StringProperty() >>>>>>>> >>>>>>>> Should be replaced by: >>>>>>>> >>>>>>>> source = EnumProperty(name="Source", description="A relevant >>>>>>>> description…", default='FROM_ACTIVE', >>>>>>>> items=(('FROM_ACTIVE', "From Active", "A >>>>>>>> relevant description…", 'ICON_NONE', 1), >>>>>>>> ('FROM_NODE', "From Node", "A relevant >>>>>>>> description…", 'ICON_NONE', 2), >>>>>>>> ('FROM_SOCKET', "From Socket", "A >>>>>>>> relevant description…", 'ICON_NONE', 3), >>>>>>>> ) >>>>>>>> ) >>>>>>>> >>>>>>>> (with relevant changes in other parts of the code). >>>>>>>> >>>>>>>> Best regards, >>>>>>>> Bastien >>>>>>>> >>>>>>>> On 25/03/2013 14:00, Bartek Skorupa (priv) wrote: >>>>>>>>> Hey, >>>>>>>>> >>>>>>>>> After recent commits of node_efficiency_tools.py I as an author call >>>>>>>>> this add-on ready. >>>>>>>>> All of the features that I had in mind have been included, documented >>>>>>>>> on wiki and in video tutorial. >>>>>>>>> Recent API changes have taken into account. >>>>>>>>> It certainly will develop further, but at this stage it's IMHO ready >>>>>>>>> to go trunk. >>>>>>>>> >>>>>>>>> I'd like to ask for reviewing the code and hopefully permission to >>>>>>>>> move this add-on to trunk. >>>>>>>>> https://svn.blender.org/svnroot/bf-extensions/contrib/py/scripts/addons/node_efficiency_tools.py >>>>>>>>> >>>>>>>>> Here's the wiki page: >>>>>>>>> http://wiki.blender.org/index.php/Extensions:2.6/Py/Scripts/Nodes/Nodes_Efficiency_Tools >>>>>>>>> >>>>>>>>> Tracker: >>>>>>>>> http://projects.blender.org/tracker/?func=detail&atid=468&aid=33543&group_id=153 >>>>>>>>> >>>>>>>>> Thread on blenderartists: >>>>>>>>> http://blenderartists.org/forum/showthread.php?274755-ADDON-Nodes-Efficiency-Tools >>>>>>>>> >>>>>>>>> >>>>>>>>> Many users appreciate this add-on and wish to have it in official >>>>>>>>> Blender releases. >>>>>>>>> I declare to maintain the code. >>>>>>>>> >>>>>>>>> Please let me know if you feel that it's worth including or not. >>>>>>>>> >>>>>>>>> >>>>>>>>> With Respect, >>>>>>>>> >>>>>>>>> Bartek Skorupa >>>>>>>>> >>>>>>>>> www.bartekskorupa.com >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> Bf-committers mailing list >>>>>>>>> Bf-committers@blender.org >>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Bf-committers mailing list >>>>>>>> Bf-committers@blender.org >>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>> _______________________________________________ >>>>>>> Bf-committers mailing list >>>>>>> Bf-committers@blender.org >>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>> >>>>>> _______________________________________________ >>>>>> Bf-committers mailing list >>>>>> Bf-committers@blender.org >>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>> _______________________________________________ >>>>> Bf-committers mailing list >>>>> Bf-committers@blender.org >>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>> >>>> _______________________________________________ >>>> Bf-committers mailing list >>>> Bf-committers@blender.org >>>> http://lists.blender.org/mailman/listinfo/bf-committers >>> _______________________________________________ >>> Bf-committers mailing list >>> Bf-committers@blender.org >>> http://lists.blender.org/mailman/listinfo/bf-committers >> _______________________________________________ >> Bf-committers mailing list >> Bf-committers@blender.org >> http://lists.blender.org/mailman/listinfo/bf-committers >> > _______________________________________________ > Bf-committers mailing list > Bf-committers@blender.org > http://lists.blender.org/mailman/listinfo/bf-committers _______________________________________________ Bf-committers mailing list Bf-committers@blender.org http://lists.blender.org/mailman/listinfo/bf-committers