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

[Pau Arumí Albó]

El ds 26 de 04 del 2008 a les 06:18 -0300, en/na Natanael Olaiz va
escriure:
> Removed a processboxTooltip unused argument (index).
> 
> El 04/26/2008 05:41 AM, Natanael Olaiz escribió:
> > Hi!
> >
> > Starting from Pau suggested exercises, this is what I did:

> > As always, suggestions and critics are welcome. I putted some comments 
> > with questions in some places.

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.

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)

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?

> >    - 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.

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.


> +               CLAM::ProcessingFactory & factory =
> CLAM::ProcessingFactory::GetInstance();
> +               const char* nombreclase =

Yep, se te ha colado a spanish name :-)

>  ((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...)


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(...)



> >    - Added an auto-decimal-configuration in the spinbox of 
> > ControlSenderWidget

Nice.  I'd wait a new patch with spaces->tabs.

That's all for now, looking forward for your next revision.


Pau