Unity

Merge lp://qastaging/~azzar1/unity/shortcut-hint into lp://qastaging/unity

shortcut-hint
Merge into trunk

Proposed by Andrea Azzarone on 2011-12-14

Status:

Merged

Approved by:

Andrea Azzarone on 2012-01-13

Approved revision:

no longer in the source branch.

Merged at revision:

1833

Proposed branch:

lp://qastaging/~azzar1/unity/shortcut-hint

Merge into:

lp://qastaging/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://qastaging/~azzar1/unity/shortcut-hint

High

Fix Released

Link a bug report

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: mp+85575@code.qastaging.launchpad.net

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://launchpadlibrarian.net/85352653/Super_key_shutcuts_overlay.png
Branch: http://ubuntuone.com/7cqBbWAbvXTRCT4ySFj7z4

Visual diff: http://ubuntuone.com/014zDKCoxaQYmtm8mS5pp8

Keep in mind that something is different because the shortcuts values are not hardcoded.

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-12-08: Posted in a previous version of this proposal

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 ;).

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2011-12-08: Posted in a previous version of this proposal

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_escape_text ;).

We no longer need it ;)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-12-08: Posted in a previous version of this proposal

That was not a review, just a comment! :)

Revision history for this message

Jeremy Bícha (jbicha) wrote on 2011-12-11: Posted in a previous version of this proposal

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.

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2011-12-11: Posted in a previous version of this proposal

It's a WIP btw here's the mockup: https://launchpadlibrarian.net/85352653/Super_key_shutcuts_overlay.png.

Revision history for this message

Marco Biscaro (marcobiscaro2112) wrote on 2011-12-11: Posted in a previous version of this proposal

Another comment :) there is a typo here:

+ hints_.push_back(new shortcut::Hint(_("Switching"), "", "", _("Moves the foucs."), shortcut::HARDCODED_OPTION, _("Cursor Left & Right")));

Revision history for this message

Tim Penhey (thumper) wrote on 2011-12-13: Posted in a previous version of this proposal

another comment, since this isn't yet a review :-)

properties should be lower case.

Revision history for this message

Tim Penhey (thumper) wrote on 2011-12-14:

Hi Andy,

Can you add links for the design mockup, and a picture of what is
rendered with this branch?

Cheers,
Tim

Revision history for this message

Tim Penhey (thumper) wrote on 2011-12-14:

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

review: Needs Information

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2011-12-14:

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

Revision history for this message

John Lea (johnlea) wrote on 2011-12-15:

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://launchpadlibrarian.net/85352653/Super_key_shutcuts_overlay.png (the design itself)
- https://launchpadlibrarian.net/86861725/keyboard_shortcuts_sizes.png (the grid the design is build on)

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://docs.google.com/a/canonical.com/document/d/1jqeKtIJwqLtl58Wk_fqjr9Rrgxn9zsouCYOo-cZsLSE/edit?authkey=CLGG9NkJ&hl=en_GB

Once these are fixed we are good to go, thx!

review: Needs Fixing (design)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-12-15:

> > 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://ubuntuone.com/5bB2px39meddmAtL4bomM2

review: Needs Fixing

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2011-12-15:

> 6. The shortcuts need updating to exactly match those specified in the
> following document https://docs.google.com/a/canonical.com/document/d
> /1jqeKtIJwqLtl58Wk_fqjr9Rrgxn9zsouCYOo-cZsLSE/edit?authkey=CLGG9NkJ&hl=en_GB

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

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2011-12-16:

> 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://launchpadlibrarian.net/85352653/Super_key_shutcuts_overlay.png (the
> design itself)
> - https://launchpadlibrarian.net/86861725/keyboard_shortcuts_sizes.png (the
> 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://docs.google.com/a/canonical.com/document/d
> /1jqeKtIJwqLtl58Wk_fqjr9Rrgxn9zsouCYOo-cZsLSE/edit?authkey=CLGG9NkJ&hl=en_GB
>
> Once these are fixed we are good to go, thx!

I'll do it tomorrow.

Revision history for this message

Andrea Azzarone (azzar1) wrote on 2011-12-19:

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?

Revision history for this message

Tim Penhey (thumper) wrote on 2011-12-20:

A general pass over the code and it seems OK, but nothing in UnityCore.

review: Approve

Revision history for this message

John Lea (johnlea) wrote on 2011-12-20:

awesome, everything perfect, great to see a attached .png with the visual design matched up against the implementation.

thanks!

review: Approve (design)

Revision history for this message

Marco Trevisan (Treviño) (3v1n0) wrote on 2012-01-06:

I've just checked again this branch, but I still get that visual issue that I've already mentioned :(