[Clam-devel] Re: Patch: add duplicate context menu option for processing boxes

[Natanael Olaiz]

Sorry for the late reply. I was testing Blender, thinking about the new 
plan, and learning a lot. :)

El 04/29/2008 02:13 PM, Pau Arumí Albó escribió:
> Hi Natanael,
> is great to see new patches and features coming in! 
> As Hernan said, let's keep different concerns in different patches. It's
> easy to review/discuss separately and do separate commits also.
>
> So, please prepare three new patches (having updated to last revision --
> beware of conflicts when updating). NetworkCanvas.hxx is source of two
> patches, so you'll need to have two copies. Actually i use to work with
> several sandboxes (=local copy of the repository) to keep different
> local changes.
>   
Here I attached two: one with a cleaner "processBoxToolTip(...)" and a 
minor correction in a comment of my previous patch, and the other is the 
"auto decimal spinbox configuration". About this last, I think is not 
really useful right now with the limitation in the configuration widget 
spinbox, and the automatic creation limitation in NetworkCanvas ( 
steps=max((upper-lower)/200, 0.01) ) but I though that in the future 
could be useful...

> A tip for people learning patches with subversion: new files (also svg
> icons) should be part of the patch. For that, just do $ svn add
> <mynewfile>. Then, a svn diff will also contain the (whole) new file.
>
> Also, check the indentation style: in CLAM we use only tabs not spaces.
> (i missed to revise this in the last patch -- i've seen the corrective
> commit from David)
>   
It wasn't my intention to use spaces. I don't know if it was because a 
mistake or the kdevelop did something weired. What about these new patches?

> Now some comments on your changes:
>
>   
>>  - Added a duplicate context option for processing boxes (I attached 
>>     
>>> an ugly test icon :) )
>>>       
>
> I've tested it and it's very interesting. However let's keep this in the
> fridge for a while. I'd like to have some discussion about what kind of
> copy-paste feature we'd like. My first thought is that a processing copy
> should be applicable to the current selection (maybe with multiple
> processings). Then it wouldn't make sense to have two features that do
> the same in different ways.  
> What do people think?
>   

I agree.

>   
>>>    - Added a tooltip when moving over the name of processing box (is a 
>>> little bit annoying)
>>>       
>
> I like it (i don't know if i'm missing the annoying part). I find it
> very useful.
>   
I though it could be annoying when is too close with the "bodyRegion" 
(some objects by default have a small "body").

> Another related feature i'd like to see is this tooltip when moving over
> the processing-tree (the left pane). 
>
> @@ -1002,6 +1001,21 @@
>   
>>                         .arg(typeString)
>>                         ;
>>         }
>> +
>> +       QString processboxTooltip(void * processing) const
>> +       { // TODO: put a global configuration tag to enable/disable
>>  this tooltip
>>     
>
> I suggest you to remove this TODO, we don't have configuration
> parameters in network-editor anyway.
>   
Done.

>
>   
>> +               CLAM::ProcessingFactory & factory =
>> CLAM::ProcessingFactory::GetInstance();
>> +               const char* nombreclase =
>>     
>
> Yep, se te ha colado a spanish name :-)
>
>   
oops. :)

>>  ((CLAM::Processing*)processing)->GetClassName();
>> +               std::string stringtemp =
>> factory.GetValueFromAttribute(nombreclase,"description");
>> +               const char* desc = stringtemp.c_str();
>> +               const char* category =
>> factory.GetValueFromAttribute(nombreclase,"category").c_str();
>>     
>
> a CLAM-naming-style tip: avoid using variables with "temp". In this case
> i'd avoid the string var by enlarging the getter with  .c_str().
> Also prefer complete names: "description" than "desc" 
>
> (I think we desperately need a wiki page with naming conventions...)
>   
Done.

> With the minor fixes, this is commitable. But i'd like a next step that
> removes the hardcoded attributes "category", "description" and gets all
> metadata instead -- using Factory::GetPairsFromKey(...)
>   
OK. I'll do it.

>
>
>   
>>>    - Added an auto-decimal-configuration in the spinbox of 
>>> ControlSenderWidget
>>>       
>
> Nice.  I'd wait a new patch with spaces->tabs.
>   
Are OK now?


Im testing in another directory your suggestion about adding also a 
processingTree info tooltip. Actually I'm printing it on std::cout, but 
I still need to print real tooltips...


Best regards,
Natanael.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autoDecimalSpinbox_in_ControlSenderWidget.patch
Type: text/x-patch
Size: 672 bytes
Desc: not available
URL: <http://lists.clam-project.org/pipermail/clam-devel-clam-project.org/attachments/20080506/76a272df/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: processBoxToolTip.patch
Type: text/x-patch
Size: 2540 bytes
Desc: not available
URL: <http://lists.clam-project.org/pipermail/clam-devel-clam-project.org/attachments/20080506/76a272df/attachment-0007.bin>