Merge lp:~azzar1/unity/shortcut-hint into lp:unity
Status: | Merged |
---|---|
Approved by: | Andrea Azzarone on 2012-01-13 |
Approved revision: | 1771 |
Merged at revision: | 1833 |
Proposed branch: | lp:~azzar1/unity/shortcut-hint |
Merge into: | lp:unity |
Diff against target: |
2326 lines (+2055/-7) 25 files modified
plugins/unityshell/src/AbstractSeparator.cpp (+68/-0) plugins/unityshell/src/AbstractSeparator.h (+49/-0) plugins/unityshell/src/AbstractShortcutHint.h (+102/-0) plugins/unityshell/src/BackgroundEffectHelper.cpp (+1/-1) plugins/unityshell/src/LineSeparator.cpp (+73/-0) plugins/unityshell/src/LineSeparator.h (+44/-0) plugins/unityshell/src/MockShortcutHint.h (+72/-0) plugins/unityshell/src/ShortcutController.cpp (+194/-0) plugins/unityshell/src/ShortcutController.h (+87/-0) plugins/unityshell/src/ShortcutHint.cpp (+137/-0) plugins/unityshell/src/ShortcutHint.h (+53/-0) plugins/unityshell/src/ShortcutHintPrivate.cpp (+79/-0) plugins/unityshell/src/ShortcutHintPrivate.h (+39/-0) plugins/unityshell/src/ShortcutModel.cpp (+60/-0) plugins/unityshell/src/ShortcutModel.h (+64/-0) plugins/unityshell/src/ShortcutView.cpp (+424/-0) plugins/unityshell/src/ShortcutView.h (+93/-0) plugins/unityshell/src/unityshell.cpp (+99/-1) plugins/unityshell/src/unityshell.h (+9/-1) plugins/unityshell/unityshell.xml.in (+6/-0) standalone-clients/CMakeLists.txt (+26/-3) standalone-clients/TestShortcut.cpp (+109/-0) tests/CMakeLists.txt (+9/-1) tests/test_shortcut_model.cpp (+84/-0) tests/test_shortcut_private.cpp (+74/-0) |
To merge this branch: | bzr merge lp:~azzar1/unity/shortcut-hint |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Marco Trevisan (Treviño) | Approve on 2012-01-13 | ||
John Lea (community) | design | 2011-12-14 | Approve on 2011-12-20 |
Tim Penhey (community) | 2011-12-14 | Approve on 2011-12-20 | |
Review via email:
|
This proposal supersedes a proposal from 2011-12-07.
Description of the change
Display a shortcut hints overlay during the super key pressing.
It includes two unit tests and a standalone test.
Mockup: https:/
Branch: http://
Visual diff: http://
Keep in mind that something is different because the shortcuts values are not hardcoded.
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
Andrea Azzarone (azzar1) wrote : | # |
Marco you should not review a WIP :P
> I've just looked at few things but it seem good, however remember to unregister the ubus controller (i.e as done in lp:~vanvugt/unity/fix-887465-trunk )
Thanks for tip.
> and to use g_markup_
We no longer need it ;)
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
That was not a review, just a comment! :)
Jeremy Bicha (jbicha) wrote : | # |
Hi, I hope you don't mind me giving feedback on the wording choices since it's still WIP...
Please use Trash instead of Rubbish Bin; the en_GB translation can change it back to Rubbish Bin.
The verb should consistently add the trailing "s" or not. In other words, "open" or "opens". For the Ubuntu system help, we left off the "s" but it could just as easily go the other way.
I'd like to see the keyboard keys always capitalized; I think some are stored as lowercase in metacity or whatever, but for display you should be able to upper-case them. ("Super + s" should be "Super + S").
How about "arrow keys" instead of "cursor keys"?
I believe menu bar is slightly preferred over top bar.
Andrea Azzarone (azzar1) wrote : | # |
It's a WIP btw here's the mockup: https:/
Marco Biscaro (marcobiscaro2112) wrote : | # |
Another comment :) there is a typo here:
+ hints_.
Tim Penhey (thumper) wrote : | # |
another comment, since this isn't yet a review :-)
properties should be lower case.
Tim Penhey (thumper) wrote : | # |
Hi Andy,
Can you add links for the design mockup, and a picture of what is
rendered with this branch?
Cheers,
Tim
Tim Penhey (thumper) wrote : | # |
Purely visual review:
The mockup lines are not white, but either transparent white or
transparent grey. The headings of each group are also not white.
The lines in the mockup are 1px thin grey with distinct ends. The view
has thicker lines that are brighter that taper at the ends.
The white-space above the "Windows" header is greater than the rest.
Some of the key-bindings are lower case, why is that?
Why are we missing some of the key bindings, like moving the focused
window to other workspaces?
Tim
Andrea Azzarone (azzar1) wrote : | # |
> Purely visual review:
>
> The mockup lines are not white, but either transparent white or
> transparent grey. The headings of each group are also not white.
>
> The lines in the mockup are 1px thin grey with distinct ends. The view
> has thicker lines that are brighter that taper at the ends.
>
I'll look to it.
>
> The white-space above the "Windows" header is greater than the rest.
>
Yeah, because i've commented a couple of key bindings because they are not yet implemented.
>
> Some of the key-bindings are lower case, why is that?
>
Because Compiz CompOption gives me something like that: <Super>f... I've writed a very simple function to change it in "Super + f". I think the best solution is to capitalize each word of the shortcut.
> Why are we missing some of the key bindings, like moving the focused
> window to other workspaces?
As said, some options are not yet implmented. For example, regarding "Move focused window to different workspace" I can't find it in CCSM.
John Lea (johnlea) wrote : | # |
Looking great! ;-) However there are a few minor issues that need to be fixed before this lands.
The feedback below is based on comparing the implementation screenshot posted with the following designs:
- https:/
- https:/
Items in need of fixing:
1. The divider lines should by 10% opacity (while remaining 100% white)
2. The divider should start flush with text, and end flush with grid (see grid design above)
3. The divider lines should have flat ends
4. The spacing of the title "Keyboard Shortcuts" is slightly out, should have slightly more space above than below (see design)
5. The spacing of the section titles is slightly out, should have slightly more space above each title than below (see design)
6. The shortcuts need updating to exactly match those specified in the following document https:/
Once these are fixed we are good to go, thx!
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> > Why are we missing some of the key bindings, like moving the focused
> > window to other workspaces?
>
> As said, some options are not yet implmented. For example, regarding "Move
> focused window to different workspace" I can't find it in CCSM.
It's under desktop wall -> bindings -> Move with window within wall
Plus, I put here for reference as well, the bottom edge has some issues here:
- http://
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> 6. The shortcuts need updating to exactly match those specified in the
> following document https:/
> /1jqeKtIJwqLtl5
About the Alt+` shortcut... As it depends to the keyboard layout (i.e. in the Italian we have Alt+\ in fact), I guess that it should be localized...
Andrea Azzarone (azzar1) wrote : | # |
> Looking great! ;-) However there are a few minor issues that need to be fixed
> before this lands.
>
> The feedback below is based on comparing the implementation screenshot posted
> with the following designs:
> - https:/
> design itself)
> - https:/
> grid the design is build on)
>
> Items in need of fixing:
>
> 1. The divider lines should by 10% opacity (while remaining 100% white)
>
Done.
> 2. The divider should start flush with text, and end flush with grid (see grid
> design above)
Done.
>
> 3. The divider lines should have flat ends
Done.
>
> 4. The spacing of the title "Keyboard Shortcuts" is slightly out, should have
> slightly more space above than below (see design)
>
Done.
> 5. The spacing of the section titles is slightly out, should have slightly
> more space above each title than below (see design)
Done.
>
> 6. The shortcuts need updating to exactly match those specified in the
> following document https:/
> /1jqeKtIJwqLtl5
>
> Once these are fixed we are good to go, thx!
I'll do it tomorrow.
Andrea Azzarone (azzar1) wrote : | # |
Ok ready for the review. I've fixed something in the layout but I don't know if I've fixed the Marco's problem. Marco can you test it please? If you still have the same problem, can you check your DPI?
Tim Penhey (thumper) wrote : | # |
A general pass over the code and it seems OK, but nothing in UnityCore.
John Lea (johnlea) wrote : | # |
awesome, everything perfect, great to see a attached .png with the visual design matched up against the implementation.
thanks!
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
I've just checked again this branch, but I still get that visual issue that I've already mentioned :(
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
I'm working on a branch dependent on this (lp:~3v1n0/unity/unity-dialog), working to fix the issue I got.
So, approving.
Unity Merger (unity-merger) wrote : | # |
Attempt to merge into lp:unity failed due to conflicts:
text conflict in standalone-
- 1771. By Andrea Azzarone on 2012-01-13
-
Merge trunk.
I've just looked at few things but it seem good, however remember to unregister the ubus controller (i.e as done in lp:~vanvugt/unity/fix-887465-trunk ) in ~Controller and to use g_markup_ escape_ text ;).