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

[Tigran Gabrielyan]

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

Implementation for https://bugs.launchpad.net/ubuntu/+source/unity/+bug/1152477

[Andrea Azzarone (azzar1)]

This bug need design feedback.

review: Needs Fixing

[Andrea Azzarone (azzar1)]

*needs

[Andrea Azzarone (azzar1)]

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

review: Needs Fixing

[Adolfo Jayme Barrientos (fitojb)]

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

[Tigran Gabrielyan (tigrangab)]

> 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)]

> > 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 :)

[Andrea Azzarone (azzar1)]

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)]

> 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 :)

[Adolfo Jayme Barrientos (fitojb)]

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

[Tigran Gabrielyan (tigrangab)]

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

[Andrea Azzarone (azzar1)]

> 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)]

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

review: Approve

[Marco Trevisan (Treviño) (3v1n0)]

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

36 + DBUSMENU_MENUITEM_TOGGLE_STATE_UNCHECKED);

46 + DBUSMENU_MENUITEM_TOGGLE_STATE_CHECKED);

[Marco Trevisan (Treviño) (3v1n0)]

review: Approve

[Marco Trevisan (Treviño) (3v1n0)]

UIFe got, let's merge then! :)

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'launcher/ApplicationLauncherIcon.cpp'
--- launcher/ApplicationLauncherIcon.cpp	2013-03-14 18:55:36 +0000
+++ launcher/ApplicationLauncherIcon.cpp	2013-03-21 18:59:25 +0000
@@ -712,6 +712,8 @@
   // We only add quicklist menu-items for windows if we have more than one window
   if (windows.size() < 2)
     return;
+   
+  Window active = WindowManager::Default().GetActiveWindow();
 
   // add menu items for all open windows
   for (auto const& w : windows)
@@ -733,6 +735,12 @@
         wm.Activate(xid);
         wm.Raise(xid);
     });
+    
+    if (xid == active)
+    {
+      dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE, DBUSMENU_MENUITEM_TOGGLE_RADIO);
+      dbusmenu_menuitem_property_set_int(menu_item, DBUSMENU_MENUITEM_PROP_TOGGLE_STATE, DBUSMENU_MENUITEM_TOGGLE_STATE_CHECKED);
+    }
 
     _menu_items_windows.push_back(menu_item);
   }

=== modified file 'tests/test_application_launcher_icon.cpp'
--- tests/test_application_launcher_icon.cpp	2013-03-14 16:44:51 +0000
+++ tests/test_application_launcher_icon.cpp	2013-03-21 18:59:25 +0000
@@ -416,6 +416,9 @@
   ASSERT_NE(menu1_it, menus.end());
   EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu1_it, DBUSMENU_MENUITEM_PROP_ENABLED));
   EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu1_it, DBUSMENU_MENUITEM_PROP_VISIBLE));
+  ASSERT_STREQ(NULL, dbusmenu_menuitem_property_get(*menu1_it, DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE));
+  EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu1_it, DBUSMENU_MENUITEM_PROP_TOGGLE_STATE), 
+  	DBUSMENU_MENUITEM_TOGGLE_STATE_UNCHECKED);
   EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu1_it, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY));
   EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu1_it, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY), 300);
 
@@ -427,6 +430,9 @@
   ASSERT_NE(menu2_it, menus.end());
   EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu2_it, DBUSMENU_MENUITEM_PROP_ENABLED));
   EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu2_it, DBUSMENU_MENUITEM_PROP_VISIBLE));
+  ASSERT_STREQ(DBUSMENU_MENUITEM_TOGGLE_RADIO, dbusmenu_menuitem_property_get(*menu2_it, DBUSMENU_MENUITEM_PROP_TOGGLE_TYPE));
+  EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu2_it, DBUSMENU_MENUITEM_PROP_TOGGLE_STATE), 
+  	DBUSMENU_MENUITEM_TOGGLE_STATE_CHECKED);
   EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(*menu2_it, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY));
   EXPECT_EQ(dbusmenu_menuitem_property_get_int(*menu2_it, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY), 300);