Review Request 110910: Refactored context menu creation code out of KoResourceItemChooser and into its own QMenu class

[Friedrich W. H. Kossebau]

> On June 11, 2013, 4:44 p.m., Friedrich W. H. Kossebau wrote:
> > libs/widgets/KoResourceItemChooserContextMenu.h, lines 72-75
> > <http://git.reviewboard.kde.org/r/110910/diff/1/?file=149238#file149238line72>
> >
> >     Why not setX here? Would be the usual pattern in Qt/KDE/Calligra.
> >     So e.g.
> >     void setResource(KoResource *resource);
> 
> Sascha Suelzer wrote:
>     Ah, yes. I'll do that again, then. I figured that the get and set bits might be not needed and that the argument/no arguments difference would suffice as marking a getter or setter.

In Qt/KDE/Calligra code the pattern has developed to name setters of a property "something" "setSomething(...)" and getters "something()", so while your idea would suffice it breaks consistency :)


> On June 11, 2013, 4:44 p.m., Friedrich W. H. Kossebau wrote:
> > libs/widgets/KoResourceItemChooser.cpp, line 802
> > <http://git.reviewboard.kde.org/r/110910/diff/1/?file=149237#file149237line802>
> >
> >     Why not continue to create the menu object on the stack? It is a blocking menu anyway and will be deleted on exiting this method, so no need to create it now on the heap.
> 
> Sascha Suelzer wrote:
>     Well, that decision was based on ignorance on my part. Putting it on the stack was my first thought, yeah, but I was not sure if that was the way to go, since the vast majority of UI code seems to be done with pointers.

Indeed most is done with pointers, but mainly because the memory management of the UI classes is solved by the QWidget/QObject parentship thingie, so these objects need to be on the heap. But the toplevel parent-less objects/widgets, like menus or dialogs, which we have to delete ourselves, those are usually placed on the stack.


> On June 11, 2013, 4:44 p.m., Friedrich W. H. Kossebau wrote:
> > libs/widgets/KoResourceItemChooser.cpp, line 937
> > <http://git.reviewboard.kde.org/r/110910/diff/1/?file=149237#file149237line937>
> >
> >     Better add a comment why starting with 1 and not 0.
> >     E.g. I am wondering why, and so might many other readers of this code :)
> 
> Sascha Suelzer wrote:
>     Well, it's done this way because the first (zero) index of the TagBox is the Unfiltered/All Presets view and thus isn't really a tag, but at this point, getting combo box items is the only way to have empty tags open for assignment for the context menu. Querying the tag object directly wouldn't return these empty tags since the tag object only tracks tags with content.
>     I see the confusion though, and adding an extra list to track tags instead of querying the combo box was one of my first ideas, but I didn't go through with it since the current approach was quicker (plus I wanted (and still will ;) ) to refactor all that TagBox stuff out into its own widget, which would have done this properly), but it's rightfully biting my behind now ;). I'll fix it.

Just the comment is fine IMHO, because then it is explained why the non-usual, magic 1 is used, so no need to investigate if that is a type or something :)


> On June 11, 2013, 4:44 p.m., Friedrich W. H. Kossebau wrote:
> > libs/widgets/KoResourceItemChooser.cpp, lines 857-858
> > <http://git.reviewboard.kde.org/r/110910/diff/1/?file=149237#file149237line857>
> >
> >     New stuff to learn/discover: one can also connect signals with each other, thus no need to go via a slot, just to emit another signal. So all these three slots might not be needed, instead you can just three times do like this: 
> >     connect(menu, SIGNAL(addTagToResource(KoResource*,QString)),
> >             this, SIGNAL(addTagToResource(KoResource*,QString)));
> >     
> >     See http://qt-project.org/doc/qt-4.8/qobject.html#connect, grep for "A signal can also be connected to another signal"
> 
> Sascha Suelzer wrote:
>     That's a pretty cool thing to know 8) and I think I can use this knowledge to reduce some needless relaying in the resource server adapter.
>     Does it really apply to this particular case, though? Since the SLOT KoResourceItemChooser::contextAddTagToResource(KoResource*,QString) isn't just a relay (at least not completely), it's the actual final destination.
>     
>     It does also trigger the synchronization of all the resource item choosers of the same resource type, though.

Ah, sorry, mixed things up and thought this code was part of the menu, alright.


- Friedrich W. H.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110910/#review34156
-----------------------------------------------------------


On June 11, 2013, 7:29 p.m., Sascha Suelzer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110910/
> -----------------------------------------------------------
> 
> (Updated June 11, 2013, 7:29 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> Refactored context menu creation code out of KoResourceItemChooser and into its own QMenu class, any style feedback is welcome.
> Also a tiny amount of whitespace cleanup, but only for the files affected by the code changes.
> 
> 
> Diffs
> -----
> 
>   libs/widgets/KoResourceItemChooser.h 29fb09a 
>   libs/widgets/KoResourceItemChooser.cpp 4ae16ad 
>   libs/widgets/KoResourceItemChooserContextMenu.h a33a132 
>   libs/widgets/KoResourceItemChooserContextMenu.cpp 6af2fe1 
> 
> Diff: http://git.reviewboard.kde.org/r/110910/diff/
> 
> 
> Testing
> -------
> 
> Tested only for Krita, everything seems to work as it should.
> 
> 
> Thanks,
> 
> Sascha Suelzer
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130611/f1fa9973/attachment.htm>