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

[Sascha Suelzer]

> 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 :)

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.


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

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.


> 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);

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. 


> On June 11, 2013, 4:44 p.m., Friedrich W. H. Kossebau wrote:
> > libs/widgets/KoResourceItemChooserContextMenu.h, lines 88-90
> > <http://git.reviewboard.kde.org/r/110910/diff/1/?file=149238#file149238line88>
> >
> >     Pimpl is only needed for classes which are published. For all other classes this just means the cost of minimal indirection with no real gain.
> >     
> >     So I would rather vote for adding any members directly to this class, unless the plan is to expose this menu in the API in later releases.

Roger :). I need to learn to distinguish between those cases. Just like I need to learn about when including a .moc file is needed for some source file, etc.


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

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. 


- Sascha


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


On June 9, 2013, 10:35 a.m., Sascha Suelzer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110910/
> -----------------------------------------------------------
> 
> (Updated June 9, 2013, 10:35 a.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/58f83f02/attachment.htm>