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) <[email protected]> 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) > <[email protected]> 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) >> <[email protected]> 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) >>> <[email protected]> wrote: >>> >>>> Thank you very much. >>>> >>>> Bartek Skorupa >>>> >>>> www.bartekskorupa.com >>>> >>>> On 26 mar 2013, at 17:53, CoDEmanX <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>>>>> 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<[email protected]> >>>>>>>>>> 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 >>>>>>>>>>>> [email protected] >>>>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> Bf-committers mailing list >>>>>>>>>>> [email protected] >>>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>>> _______________________________________________ >>>>>>>>>> Bf-committers mailing list >>>>>>>>>> [email protected] >>>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> Bf-committers mailing list >>>>>>>>> [email protected] >>>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> Bf-committers mailing list >>>>>>>> [email protected] >>>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> Bf-committers mailing list >>>>>>> [email protected] >>>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>> >>>>>> _______________________________________________ >>>>>> Bf-committers mailing list >>>>>> [email protected] >>>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>>>> >>>>> _______________________________________________ >>>>> Bf-committers mailing list >>>>> [email protected] >>>>> http://lists.blender.org/mailman/listinfo/bf-committers >>>> >>>> _______________________________________________ >>>> Bf-committers mailing list >>>> [email protected] >>>> http://lists.blender.org/mailman/listinfo/bf-committers >>> >>> _______________________________________________ >>> Bf-committers mailing list >>> [email protected] >>> http://lists.blender.org/mailman/listinfo/bf-committers >> >> _______________________________________________ >> Bf-committers mailing list >> [email protected] >> http://lists.blender.org/mailman/listinfo/bf-committers > > _______________________________________________ > Bf-committers mailing list > [email protected] > http://lists.blender.org/mailman/listinfo/bf-committers _______________________________________________ Bf-committers mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
