Merge lp://staging/~attente/unity/gnome-key-grabber into lp://staging/unity

FAILED: Continuous integration, rev:3620
http://jenkins.qa.ubuntu.com/job/unity-ci/564/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/100/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/96/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/98/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/564/rebuild

Hi William,

We are going to wait to review this until we get https://code.launchpad.net/~3v1n0/unity/unity-decorations/+merge/202582 merged into trunk. There will likely be some conflicts, so those will need to be fixed once that merges.

Thanks!

+bool PanelMenuView::SetMenuBarVisible(bool visible)
75 +{
76 + menu_bar_visible_ = visible;
77 + QueueDraw();
78 + return true;
79 +}
80 +

Only queuedraw and return true if value really changed (likely to happen, but nicer to see :)).

Also, in PanelMenuView you added a new var "menu_bar_visible_"... We already had show_now_activated_ for this purpose... I'm not sure if them might clash here, but what about just using one?

Also, to make the panel show the menu items correctly underlined, each PanelIndicatorEntry should have the show-now state...

251 + current_action_id_++;
Prefixed increment?

261 + CompAction& added(actions_.back());

Mnh, this is needed to access to the reference of it?
As I'd just use action instead... But maybe compiz definitions makes things harder.

262 + if (grabs_by_binding_[added.key()]++ == 0)
263 + screen_->addAction(&added);

A little hard to read... Probably using (with proper spacing):
auto& grab = grabs_by_binding_[added.key()];
if (grab == 0) { screen_->addAction(&added) }
++grab;

270 + std::map<const CompAction*, unsigned int>::const_iterator i(action_ids_by_action_.find(&action));

Auto is your friend here...
auto i = action_ids_by_action_.find(&action);
And everywhere we have iterators or complex or duplicated types (like A = new A()), just use it.

283 + if (--grabs_by_binding_[j->key()] == 0)
284 + screen_->removeAction(&*j);

As before... Also this &* thing is not the best to see...
248 +unsigned int GnomeKeyGrabber::Impl::addAction(const CompAction& action,
249 + bool addressable)

As for general style, we generally try to keep method names (with parameters) in just one line, unless it really becomes too long (where "too long" is not exactly specified) :P
Also we use Type const&, not const Type&...

315 + g_variant_builder_init(&builder, G_VARIANT_TYPE("au"));

This is, fine... But for your convenience you can use glib::Variant::FromVector for generating arrays such this...

action.setInitiate(boost::bind(&GnomeKeyGrabber::Impl::actionInitiated, this, _1, _2, _3));

std::bind? Or probably you can just use lambdas here, as they improve readability for small cases such these ones.

568 + std::map<const CompAction*, unsigned int> action_ids_by_action_;
569 + std::map<unsigned int, const CompAction*> actions_by_action_id_;

Mh, I'm not a fan of using non-smart pointers in stl containers, even though action_ vector is the owner... Could be possible to use something better than pure pointers here?

Can you also unit-tests for GnomeKeyGrabber (in the same way I did for GnomeSessionManager)?

Also I'd move the whole KeyGrabber thing on a separate library/folder... That links with unity-shared-compiz.

938 + priv->info_by_entry = g_hash_table_new_full (NULL, NULL, NULL, action_info_free);

720 + <option name="show_menu_bar" type="key">

Since this will clash with Hud reveal key, could you include in both descriptions that one is a "Tap", while the other is a key-press?

808 + PanelService *service;

As panelservice uses a singleto, I think you don't need to pass the service to every entry... Just use the global instance.

938 + priv->info_by_entry = g_hash_table_new_full (NULL, NULL, NULL, action_info...

PASSED: Continuous integration, rev:3622
http://jenkins.qa.ubuntu.com/job/unity-ci/577/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/113
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/109
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/111

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/577/rebuild

PASSED: Continuous integration, rev:3631
http://jenkins.qa.ubuntu.com/job/unity-ci/585/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/121
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/117
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/119

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/585/rebuild

PASSED: Continuous integration, rev:3633
http://jenkins.qa.ubuntu.com/job/unity-ci/586/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/122
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/118
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/120

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/586/rebuild

PASSED: Continuous integration, rev:3634
http://jenkins.qa.ubuntu.com/job/unity-ci/589/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/125
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/121
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/123

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/589/rebuild

FAILED: Continuous integration, rev:3636
http://jenkins.qa.ubuntu.com/job/unity-ci/591/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/127/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/123/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/125/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/591/rebuild

Hi Chris and Marco,

Thanks for the comments, I've resolved most of them. The MP is quite a bit smaller too, thanks for that!

> 261 + CompAction& added(actions_.back());
>
> Mnh, this is needed to access to the reference of it?
> As I'd just use action instead... But maybe compiz definitions makes things
> harder.

Yes, you're correct, we must pass a non-const pointer to compiz.

> 315 + g_variant_builder_init(&builder, G_VARIANT_TYPE("au"));
>
> This is, fine... But for your convenience you can use
> glib::Variant::FromVector for generating arrays such this...

Thanks, I'm leaving this as is since we need it to type properly if we get an empty array. Also, it might be more efficient than constructing an intermediate vector.

> 568 + std::map<const CompAction*, unsigned int> action_ids_by_action_;
> 569 + std::map<unsigned int, const CompAction*> actions_by_action_id_;
>
> Mh, I'm not a fan of using non-smart pointers in stl containers, even though
> action_ vector is the owner... Could be possible to use something better than
> pure pointers here?

So, while not as nice looking, I think I can justify it for a couple of reasons. We never end up de-referencing those pointers; their use is only for lookups of action ids. Also, I don't want the key grabber to keep those objects alive with a shared_ptr, and using weak_ptrs might have bad effects with the map when they get zeroed out.

> Can you also unit-tests for GnomeKeyGrabber (in the same way I did for
> GnomeSessionManager)?

I added a few autopilot tests for this.

> However, as for general design thing... Whouldn't be possible to handle the
> Activation, and registration of actions inside unity itself (instead of doing
> the work by dbus)? I know it's the standard way at this point, but maybe it
> would have kept the panel service simpler...

Thanks, moved it into the panel implementation instead :)

110 + auto accelerator(gtk_accelerator_name(gdk_keyval_to_lower(gdk_unicode_to_keyval(mnemonic)), GDK_MOD1_MASK));

use glib::String here.

111 + auto action(std::make_shared<CompAction>());

Why not just auto action = std::make_shared... ? g++ would just use the best way to initialize it. Same in other places...

115 + action->setInitiate([=](CompAction* action, CompAction::State state, CompOption::Vector& options)
116 + {

Just pass [this, id] to the lambda, where id is the entry->id() copied to a string.

1061 + CompScreen* screen_;
FYI CompScreen is exported by <core/screen.h> as a global "screen" variable and is always available since the plugin initialization, so you can just use that reference if you want.

884 + action.setInitiate([=]
892 + action.setTerminate([=]

It doesn't seem you need to pass anything a part from [this] to these lambdas, so just avoid to copy everything please.

1066 + std::map<CompAction const*, unsigned int> action_ids_by_action_;
1067 + std::map<unsigned int, CompAction const*> actions_by_action_id_;

As you're never iterating over them, using std::unordered_map here might give us some benefit.

Thanks Marco! Cleaned up those issues.

> 1061 + CompScreen* screen_;
> FYI CompScreen is exported by <core/screen.h> as a global "screen" variable
> and is always available since the plugin initialization, so you can just use
> that reference if you want.

Thanks for the heads up, personally, I don't mind passing it down to the grabber.

So, I've just tested in the real world, and it works very well...

However, I'm getting some console errors we should avoid:

ERROR 2014-02-04 19:44:10 unityfree <unknown>:0 the GVariant format string '(b)' has a type of '(b)' but the given value has a type of 'b'
ERROR 2014-02-04 19:44:10 unityfree <unknown>:0 g_variant_get: assertion 'valid_format_string (format_string, TRUE, value)' failed

I'm not sure where this is happening...

Also, there's a small regression: on Alt pressure, the menu entries should show after a small delay. The logic is already handled by PanelMenuView, so using your code just replacing your PanelMenuView::SetMenuBarVisible with http://pastebin.ubuntu.com/6874592/ is enough.

However, imho you don't need to be so invasive... and you can just use this way: http://pastebin.ubuntu.com/6874662/ (apply this to your branch)

Hi William,

I've run the AP tests and I'm getting a failure for the unity.tests.test_gnome_key_grabber.GnomeKeyGrabberTests.test_grab_accelerator test. Here is the AP log of the failure:

Traceback (most recent call last):
  File "/home/townsend/Reviews/misc/unity/tests/autopilot/unity/tests/test_gnome_key_grabber.py", line 87, in test_grab_accelerator
    self.check_accelerator(action, 'Shift+Control+Alt+a')
  File "/home/townsend/Reviews/misc/unity/tests/autopilot/unity/tests/test_gnome_key_grabber.py", line 50, in check_accelerator
    self.assertTrue(activated[0])
  File "/usr/lib/python2.7/unittest/case.py", line 424, in assertTrue
    raise self.failureException(msg)
AssertionError: False is not true

PASSED: Continuous integration, rev:3638
http://jenkins.qa.ubuntu.com/job/unity-ci/594/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/130
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/126
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/128

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/594/rebuild

> However, I'm getting some console errors we should avoid:
>
> ERROR 2014-02-04 19:44:10 unityfree <unknown>:0 the GVariant format string
> '(b)' has a type of '(b)' but the given value has a type of 'b'
> ERROR 2014-02-04 19:44:10 unityfree <unknown>:0 g_variant_get: assertion
> 'valid_format_string (format_string, TRUE, value)' failed
>
> I'm not sure where this is happening...

It's something unrelated in trunk it seems, here's an independent fix:

https://code.launchpad.net/~attente/unity/hud-controller-gvariant-type-error/+merge/204893

PASSED: Continuous integration, rev:3641
http://jenkins.qa.ubuntu.com/job/unity-ci/598/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/134
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/130
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/132

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/598/rebuild

Hmm, I'm still getting the same AP test failure as I mentioned above.

Also, it's probably better to leave out the print's in your AP code. If you really need to capture that output, then perhaps having it output when --verbose is passed to autopilot would be better.

PASSED: Continuous integration, rev:3642
http://jenkins.qa.ubuntu.com/job/unity-ci/601/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/137
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/133
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/135

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/601/rebuild

Hi Christopher, can you re-run the tests and paste the output? I'm unable to replicate the problem.

PASSED: Continuous integration, rev:3643
http://jenkins.qa.ubuntu.com/job/unity-ci/604/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/140
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/136
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/138

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/604/rebuild

I don't get failures as well.

However, speaking of the code itself, I'm ok with it now.

As for testing this AP code sees a little too complex: I mean, it's better to keep single tests small adding more functions. Also, since we're in the AP land, it would be nice if you might test the real world usage (i.e. open a window, better a mocked one, and test that it's menus are shown on Alt or opened on accelerators).
In general I prefer unit tests (especially for non-visual things like this), and in this case you might have done them quite easily.

Even though the tests are non-visual, I think it's easiest for us to test the key presses and releases using AP.

Unity already has AP tests for the panel that are currently failing, no? I saw them under unity.tests.test_panel.PanelKeyNavigationTests, and running them I only get a test failure for test_panel_first_menu_show_works, but it seems to be for the Alt+F10 key which is unrelated to this MP.

Sorry, on second glance, I see only an "Alt+F" test, I don't see one for holding "Alt". I'll add another panel AP test for that.

PASSED: Continuous integration, rev:3644
http://jenkins.qa.ubuntu.com/job/unity-ci/606/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/142
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/138
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/140

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/606/rebuild

Hi Christopher, I think the reason the new AP tests are failing is the same reason why this MP exists in the first place: compiz and g-s-d are still fighting over the key grabs. You need the g-s-d from this branch to fully fix the bug and have the AP tests working again: https://code.launchpad.net/~attente/gnome-settings-daemon/gnome-key-grabber.

I uploaded that g-s-d to a PPA so you can just download the package from here without building that branch: https://launchpad.net/~attente/+archive/gnome-key-grabber.

Hi William,

I installed your PPA's g-s-d and I still get the failure for that same particular test:(

PASSED: Continuous integration, rev:3645
http://jenkins.qa.ubuntu.com/job/unity-ci/608/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/144
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/140
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/142

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/608/rebuild

PASSED: Continuous integration, rev:3646
http://jenkins.qa.ubuntu.com/job/unity-ci/610/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/146
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/142
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/144

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/610/rebuild

Ok, things are working well here, so I think we can safely approve it.

As for more tests, feel free to propose other branches. :)

Hi William,

Before we globally approve, you'll need to merge trunk again as there is now a conflict in test_panel.py.

Once you do that, we will go ahead and merge since I'm the only one getting the test_grab_accelerator issue. Brandon tried as well and it passes for him, so I think there is probably some issue with my test machine. At any rate, I'll keep an eye on the daily-build Jenkins CI runs to make sure it passes there as well.

Ok, approving this as well. Let's get it in!