Merge lp://qastaging/~unity-2d-team/unity-2d/unity-2d_enable-super-numkey-combo into lp://qastaging/unity-2d

Proposed by Albert Astals Cid
Status: Rejected
Rejected by: Gerry Boland
Proposed branch: lp://qastaging/~unity-2d-team/unity-2d/unity-2d_enable-super-numkey-combo
Merge into: lp://qastaging/unity-2d
Diff against target: 292 lines (+148/-35)
6 files modified
launcher/app/launcherview.cpp (+34/-13)
libunity-2d-private/src/hotkey.cpp (+24/-18)
libunity-2d-private/src/hotkey.h (+5/-1)
libunity-2d-private/src/hotkeymonitor.cpp (+2/-2)
libunity-2d-private/src/hotkeymonitor.h (+1/-1)
tests/launcher/enable-super-numkey.rb (+82/-0)
To merge this branch: bzr merge lp://qastaging/~unity-2d-team/unity-2d/unity-2d_enable-super-numkey-combo
Reviewer Review Type Date Requested Status
Gerry Boland (community) Disapprove
Paweł Stołowski (community) Needs Fixing
Albert Astals Cid (community) Needs Fixing
Michał Sawicz Pending
Review via email: mp+91436@code.qastaging.launchpad.net

Description of the change

[launcher] Enabled Super+NumKey[0..9] shortcuts for launcher tiles.

We need separate hotkey's for them.

To post a comment you must log in.
Revision history for this message
Albert Astals Cid (aacid) wrote :

This is the heir of https://code.launchpad.net/~unity-2d-team/unity-2d/enable-super-numkey-combo/+merge/89883 that was rejected because was against unity-2d-shell instead of unity-2d

Copying here Michał's comments of what needs to be fixed.

The test is broken:
* if the launcher item is already in "running" state, the test won't really test anything
* if the app in question doesn't start within TIMEOUT, the test will fail even though the feature actually worked
* LibreOffice Impress, for example, doesn't go into "running" state before you actually create a slidedeck, so the test will fail in a clean environment every time
* the result of this test is a workspace cluttered with up to 10 new windows that aren't closed

review: Needs Fixing
Revision history for this message
Paweł Stołowski (stolowski) wrote :

1. I think that the logic based on isX11Keysym in LauncherView::forwardNumericHotkey(), as well as Hotkey constructor are good candidates for introducing two simple classes inherited from HotKey: NumericHotkey and NumpadNumHotKey that hide x11keysm-based logic. They would inherit from Hotkey and from a common abstract class that provides index() interface, hinding index = key - .... calculation/comparison logic. I don't insist, but I prefer to avoid leaking of logic outside of classes and use polymorphic classes instead.

2. I think HotkeyMonitor::getHotkeyFor(Qt::Key ...) should check for equality of isX11keysym flag, and not only key and modifiers values.

review: Needs Fixing
Revision history for this message
Gerry Boland (gerboland) wrote :
review: Disapprove

Unmerged revisions

881. By Albert Astals Cid

forgot the test :D

880. By Albert Astals Cid

Backport r947 from unity-2d-shell

message:
  [shell][launcher] Enabled Super+NumKey[0..9] shortcuts for launcher tiles.
  We need separate hotkey's for them.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches