Thank you very much Campbell for your changes. I was so excited about possibility to move the Add On to trunk that I missed those other mixed cases that you corrected. Sorry for that and once again - thank you.
With Respect Bartek Skorupa www.bartekskorupa.com On 27 mar 2013, at 19:34, Campbell Barton <ideasma...@gmail.com> wrote: > On Wed, Mar 27, 2013 at 10:49 PM, Bartek Skorupa (priv) > <bartekskor...@bartekskorupa.com> wrote: >> Final code clean up before moving to trunk: >> revision 4436. >> All suggested changes have been made. >> >> I'd like to ask for final permission before I commit. >> Thank you >> >> With Respect >> >> Bartek Skorupa >> >> www.bartekskorupa.com >> >> On 27 mar 2013, at 11:35, Bartek Skorupa (priv) >> <bartekskor...@bartekskorupa.com> wrote: >> >>> @Campbell >>> Thank you for cleaning up my code of node_efficiency_tools.py (revision >>> 4435) >>> I assume that now it's ready to be moved to trunk, is it? >>> Do I have your permission to do it? >>> >>> Thank you for your time. >>> >>> With Respect >>> Bartek Skorupa >>> >>> >>> Revision: 4435 >>> >>> http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-extensions&revision=4435 >>> Author: campbellbarton >>> Date: 2013-03-27 10:17:08 +0000 (Wed, 27 Mar 2013) >>> Log Message: >>> ----------- >>> minor code cleanup >>> >>> Modified Paths: >>> -------------- >>> contrib/py/scripts/addons/node_efficiency_tools.py >>> >>> >>> >>> >>> On 27 mar 2013, at 09:14, Bartek Skorupa (priv) >>> <bartekskor...@bartekskorupa.com> wrote: >>> >>>> 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 >>> >>> _______________________________________________ >>> 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 > > Thinks this is fine to move to trunk > > Command to move script... > svn mv > https://svn.blender.org/svnroot/bf-extensions/contrib/py/scripts/addons/node_efficiency_tools.py > https://svn.blender.org/svnroot/bf-extensions/trunk/py/scripts/addons/ > (Must add this info to our wiki docs!) > > -- > - Campbell > _______________________________________________ > 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