Merge lp:~tigrangab/unity/unity-fix-1152477 into lp:unity

Proposed by Tigran Gabrielyan on 2013-03-09
Status: Merged
Approved by: Marco Trevisan (Treviño) on 2013-04-05
Approved revision: 3206
Merged at revision: 3281
Proposed branch: lp:~tigrangab/unity/unity-fix-1152477
Merge into: lp:unity
Diff against target: 48 lines (+14/-0)
2 files modified
launcher/ApplicationLauncherIcon.cpp (+8/-0)
tests/test_application_launcher_icon.cpp (+6/-0)
To merge this branch: bzr merge lp:~tigrangab/unity/unity-fix-1152477
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve on 2013-04-05
Marco Trevisan (Treviño) Approve on 2013-04-05
Andrea Azzarone (community) 2013-03-09 Approve on 2013-03-28
Review via email: mp+152554@code.launchpad.net

Commit message

Toggle radio state to checked for active window in quicklist menu (LP: #1152477)

Description of the change

To post a comment you must log in.
Andrea Azzarone (azzar1) wrote :

This bug need design feedback.

review: Needs Fixing
Andrea Azzarone (azzar1) wrote :

*needs

Andrea Azzarone (azzar1) wrote :

Can you add tests for it? Ping me on IRC if you need help :)

review: Needs Fixing
Adolfo Jayme (fitojb) wrote :

Also, don't forget to set your commit message in this merge proposal! :-)

Tigran Gabrielyan (tigrangab) wrote :

> Can you add tests for it? Ping me on IRC if you need help :)

Yea, I will need help :) Is your name on irc the same, andyrock? The channel is #ubuntu-devel, right?

Andrea Azzarone (azzar1) wrote :

> > Can you add tests for it? Ping me on IRC if you need help :)
>
> Yea, I will need help :) Is your name on irc the same, andyrock? The channel
> is #ubuntu-devel, right?

Yeah andyrock. I'm on ubuntu-devel too but ping on me ubuntu-unity :)

3204. By Tigran Gabrielyan on 2013-03-21

Added tests for radio state in quicklist window switcher (LP: #1152477)

Andrea Azzarone (azzar1) wrote :

Thanks for testing :) Changes look good, just a small note about g_strcmp. I'd prefer to use ASSERT_STR* (see http://code.google.com/p/googletest/wiki/Primer#String_Comparison).

Also the bug says:

* = this one should have the white background already (like when you hover over it) showing it's active or have the |> icon like the launcher do showing its the active one.»

You're using a radio button instead. Are you sure designers are ok with that? :)

Tigran Gabrielyan (tigrangab) wrote :

> Thanks for testing :) Changes look good, just a small note about g_strcmp. I'd
> prefer to use ASSERT_STR* (see
> http://code.google.com/p/googletest/wiki/Primer#String_Comparison).

I'm fixing the test right now.

> Also the bug says:
>
> * = this one should have the white background already (like when you hover
> over it) showing it's active or have the |> icon like the launcher do showing
> its the active one.»
>
> You're using a radio button instead. Are you sure designers are ok with that?
> :)

I asked in my bug report if that is what the design team gave approval for. It should be because I provided screenshots and they made a note of it, but we'll see once they confirm.

I made it a radio button because I have no idea how to highlight it :)

3205. By Tigran Gabrielyan on 2013-03-21

Use ASSERT_STREQ instead of g_strcmp for test

3206. By Tigran Gabrielyan on 2013-03-21

Make code clearer as to what it's testing

Adolfo Jayme (fitojb) wrote :

John said in the bug that a radio button is fine :)

Tigran Gabrielyan (tigrangab) wrote :

Is there anything else holding this back from being merged? Will it require a FFE?

Andrea Azzarone (azzar1) wrote :

> Is there anything else holding this back from being merged? Will it require a
> FFE?

Yes please ;) Sorry btw, I missed John's comment.

Andrea Azzarone (azzar1) wrote :

Approve, don't merge it (It does require a FFe).

review: Approve
Marco Trevisan (Treviño) (3v1n0) wrote :

A part the indentation below, it's fine for me as well.

36 + DBUSMENU_MENUITEM_TOGGLE_STATE_UNCHECKED);

46 + DBUSMENU_MENUITEM_TOGGLE_STATE_CHECKED);

review: Approve
Marco Trevisan (Treviño) (3v1n0) wrote :

UIFe got, let's merge then! :)

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'launcher/ApplicationLauncherIcon.cpp'
2--- launcher/ApplicationLauncherIcon.cpp 2013-03-14 18:55:36 +0000
3+++ launcher/ApplicationLauncherIcon.cpp 2013-03-21 18:59:25 +0000
4@@ -712,6 +712,8 @@
5 // We only add quicklist menu-items for windows if we have more than one window
6 if (windows.size() < 2)
7 return;
8+
9+ Window active = WindowManager::Default().GetActiveWindow();
10
11 // add menu items for all open windows
12 for (auto const& w : windows)
13@@ -733,6 +735,12 @@
14 wm.Activate(xid);
15 wm.Raise(xid);
16 });
17+
18+ if (xid == active)
19+ {
20+ dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE, DBUSMENU_MENUITEM_TOGGLE_RADIO);
21+ dbusmenu_menuitem_property_set_int(menu_item, DBUSMENU_MENUITEM_PROP_TOGGLE_STATE, DBUSMENU_MENUITEM_TOGGLE_STATE_CHECKED);
22+ }
23
24 _menu_items_windows.push_back(menu_item);
25 }
26
27=== modified file 'tests/test_application_launcher_icon.cpp'
28--- tests/test_application_launcher_icon.cpp 2013-03-14 16:44:51 +0000
29+++ tests/test_application_launcher_icon.cpp 2013-03-21 18:59:25 +0000
30@@ -416,6 +416,9 @@
31 ASSERT_NE(menu1_it, menus.end());
32 EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu1_it, DBUSMENU_MENUITEM_PROP_ENABLED));
33 EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu1_it, DBUSMENU_MENUITEM_PROP_VISIBLE));
34+ ASSERT_STREQ(NULL, dbusmenu_menuitem_property_get(*menu1_it, DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE));
35+ EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu1_it, DBUSMENU_MENUITEM_PROP_TOGGLE_STATE),
36+ DBUSMENU_MENUITEM_TOGGLE_STATE_UNCHECKED);
37 EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu1_it, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY));
38 EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu1_it, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY), 300);
39
40@@ -427,6 +430,9 @@
41 ASSERT_NE(menu2_it, menus.end());
42 EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu2_it, DBUSMENU_MENUITEM_PROP_ENABLED));
43 EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu2_it, DBUSMENU_MENUITEM_PROP_VISIBLE));
44+ ASSERT_STREQ(DBUSMENU_MENUITEM_TOGGLE_RADIO, dbusmenu_menuitem_property_get(*menu2_it, DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE));
45+ EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu2_it, DBUSMENU_MENUITEM_PROP_TOGGLE_STATE),
46+ DBUSMENU_MENUITEM_TOGGLE_STATE_CHECKED);
47 EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu2_it, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY));
48 EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu2_it, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY), 300);
49