Code review comment for lp://staging/~ubuntu-sdk-team/ubuntu-ui-toolkit/shortcuts

Revision history for this message
Cris Dywan (kalikiana) wrote :

> if object is expected to be a QQuickItem,
> you should cast to it and that has a window()
> that you can use to get the window context...?

No. The action itself is in fact a QObject. We're walking up the parents to find a window.

> Shouldn't we also check whether the action is enabled?

No need, trigger() has a check already.

> What about testing also Alt+Ctrl, Shift+Ctrl and Alt+Shift+Ctrl?
> Or should we just assume that Qt shortcut system works with those?

Basically yeah, I don't think we want to duplicate their tests since we don't add any functionality. But I added another one with Atl+Shift+Ctrl just to be sure we'll know if anything obvious breaks in a Qt upgrade.

« Back to merge proposal