----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107973/#review24635 -----------------------------------------------------------
OK, we're getting there. More fine-grained API proposals this time, I also looked at some of the implementation style. kdeui/actions/kaction.h <http://git.reviewboard.kde.org/r/107973/#comment18890> Please get rid of this spurious change. kdeui/actions/kaction.cpp <http://git.reviewboard.kde.org/r/107973/#comment18891> } and else should be on the same line kdeui/actions/kaction.cpp <http://git.reviewboard.kde.org/r/107973/#comment18892> } and else should be on the same line kdeui/actions/kaction.cpp <http://git.reviewboard.kde.org/r/107973/#comment18893> There should be a space after if, and no space after ( or before ) kdeui/actions/kaction.cpp <http://git.reviewboard.kde.org/r/107973/#comment18894> There should be a space after if, and no space after ( or before ) kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18900> We shouldn't expose those two types... In fact with the change I'll propose below they won't be necessary anymore. kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18901> I'd expect to see the action as first parameter since we're kind of moving from "action->setGesture(gesture)" seeing it become "map->setGesture(action, gesture)" feels more natural to me (the assignment order is respected). Of course this comment holds for all the other setters in that class, I won't open separate issues. kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18902> I propose to remove that one. It's a convenience which will reduce readability in the client code. I can already see myself wondering which one is the default gesture every time I'll encounter a call to that method. :) kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18903> Please, remove it like the one above. kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18904> I would remove that one too. Since we move to a setter semantic, I don't see a use for it... also it was kind of odd as public API... I would need the right gesture/action pair for it to be useful. kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18905> Like above I propose to remove it. kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18906> This one looks like a nice convenience to keep though as there's several gesture types, etc. I'd rename it to removeAllGestures though. kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18908> Instead of that one we should have two methods instead: shapeGesture and defaultShapeGesture. This way we avoid the QPair completely from the API (again, which one would be default? .first? or .second?). kdeui/shortcuts/kgesturemap.h <http://git.reviewboard.kde.org/r/107973/#comment18909> Like above, replace with rockerGesture and defaultRockerGesture. kdeui/shortcuts/kgesturemap.cpp <http://git.reviewboard.kde.org/r/107973/#comment18897> It should probably be a qWarning(), and it's likely we want a wording closer to what we use when we detect a collision with shortcuts (just to stay consistent). kdeui/shortcuts/kgesturemap.cpp <http://git.reviewboard.kde.org/r/107973/#comment18896> It should probably be a qWarning(), and it's likely we want a wording closer to what we use when we detect a collision with shortcuts (just to stay consistent). kdeui/shortcuts/kgesturemap.cpp <http://git.reviewboard.kde.org/r/107973/#comment18898> It should probably be a qWarning(), and it's likely we want a wording closer to what we use when we detect a collision with shortcuts (just to stay consistent). kdeui/shortcuts/kgesturemap.cpp <http://git.reviewboard.kde.org/r/107973/#comment18899> It should probably be a qWarning(), and it's likely we want a wording closer to what we use when we detect a collision with shortcuts (just to stay consistent). - Kevin Ottens On Dec. 30, 2012, 3:43 p.m., Valentin Rusu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107973/ > ----------------------------------------------------------- > > (Updated Dec. 30, 2012, 3:43 p.m.) > > > Review request for KDE Frameworks. > > > Description > ------- > > This changeset removes all gesture facilities from KAction and define them in > KGestureMap. Thanks for the feedback. > > > Diffs > ----- > > kdeui/actions/kaction.h f45c94d > kdeui/actions/kaction.cpp b78d1d6 > kdeui/actions/kaction_p.h f87272c > KDE5PORTING.html 86dfc1a > kdeui/dialogs/kshortcutseditor.cpp 5653984 > kdeui/dialogs/kshortcutseditoritem.cpp e85a203 > kdeui/shortcuts/kgesturemap.h 56b42b6 > kdeui/shortcuts/kgesturemap.cpp 1350dbe > > Diff: http://git.reviewboard.kde.org/r/107973/diff/ > > > Testing > ------- > > Code builds OK. KGestureMap is not actually used, though, as > KGestureMap::eventFilter method return false right away. > > > Thanks, > > Valentin Rusu > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
