Review Request: Cleanup : Move all gesture facilities of KAction in KGestureMap

[Kevin Ottens]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130104/57823501/attachment-0001.html>