Note about "Select Parent/Children": This operator allows to select 'FRAME' that wraps selected nodes or select all of the nodes that are "children" of selected 'FRAME'. It's done using '[' and ']' keys. Campbell said that it could be implemented on a higher level, but before it happens, I'd leave it in my code. When this option is implemented I'll immediately remove this class from node_efficiency_tools.py
Bartek Skorupa www.bartekskorupa.com On 27 mar 2013, at 09:06, Bartek Skorupa (priv) <bartekskor...@bartekskorupa.com> wrote: > Thank you Campbell, Bastien and CoDEmanX for reviewing my code. > I have implemented all of your suggestions. > All of the issues pointed in this review: > https://codereview.appspot.com/7651047/diff/1/node_efficiency_tools.py#newcode103 > have been addressed. > The review has been done using not the latest version of the script, so some > of the suggestions have been implemented earlier. > The script with all of those changes is version 2..1.4 and has been committed > as revision: 4434. > > Is it now ok to move it to trunk? > > Thank you. > > With Respect > Bartek Skorupa > > www.bartekskorupa.com > > On 26 mar 2013, at 17:55, Bartek Skorupa (priv) > <bartekskor...@bartekskorupa.com> wrote: > >> Thank you very much. >> >> Bartek Skorupa >> >> www.bartekskorupa.com >> >> On 26 mar 2013, at 17:53, CoDEmanX <codem...@gmx.de> wrote: >> >>> if the number of properties you pass to an operator differ, e.g. one >>> prop per Custom Property of the active object, then you shouldn't do >>> something like >>> >>> prop1 = ... >>> prop2 = ... >>> ... >>> prop20 = ... >>> >>> to allow for up to 20 properties be passed to the operator. Why? Cause >>> you don't need 20 properties if you mostly need to pass just one or two, >>> and there may be situations in which you need more than 20. So a fixed >>> amount is really bad. >>> >>> Instead, you can use a CollectionProperty and add an item for every >>> element you need to pass. >>> >>> Here's an example: >>> >>> http://www.pasteall.org/40829/python >>> >>> >>> If ideasman is fine with your code, then it's ok to move to trunk! >>> >>> >>> Am 25.03.2013 17:33, schrieb Bartek Skorupa (priv): >>>> 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 _______________________________________________ Bf-committers mailing list Bf-committers@blender.org http://lists.blender.org/mailman/listinfo/bf-committers