----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110910/#review34156 -----------------------------------------------------------
you are a good enough coder to get some very picky review, so you can learn more :) libs/widgets/KoResourceItemChooser.cpp <http://git.reviewboard.kde.org/r/110910/#comment25070> 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. libs/widgets/KoResourceItemChooser.cpp <http://git.reviewboard.kde.org/r/110910/#comment25073> 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" libs/widgets/KoResourceItemChooser.cpp <http://git.reviewboard.kde.org/r/110910/#comment25071> 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 :) libs/widgets/KoResourceItemChooserContextMenu.h <http://git.reviewboard.kde.org/r/110910/#comment25067> Why not setX here? Would be the usual pattern in Qt/KDE/Calligra. So e.g. void setResource(KoResource *resource); libs/widgets/KoResourceItemChooserContextMenu.h <http://git.reviewboard.kde.org/r/110910/#comment25072> signal names are usually named in the style "xHappened". "doX" breaks this pattern and thus might confuse a little. Surely something like "tagToResourceAddingRequested" is quite longer, but still more descriptive, at least to me. libs/widgets/KoResourceItemChooserContextMenu.h <http://git.reviewboard.kde.org/r/110910/#comment25069> 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. - Friedrich W. H. Kossebau 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 > >
_______________________________________________ calligra-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/calligra-devel
