Merge lp:~lukas-vacek/unity/bamficon_windowlist-raring into lp:unity

[Adolfo Jayme Barrientos]

ApplicationLauncherIcon: Show a list of related windows in a Launcher icon's Quicklist

(LP: #1107866)

Show a list of windows in a Launcher icon's Quicklist. (LP: #1107866)

[Michael Terry (mterry)]

Seems like it could use a test.

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

10 + WindowManager& wm = WindowManager::Default();
11 + // delete all menu items for windows
…
29 + char menu_item_name[name_size];
30 + sprintf(menu_item_name,"w_list_%d",i);
…
44 + _menu_items[menu_item_name] = menu_item;

Please, don't add these items to _menu_items. Use instead another container (such as
std::vector<glib::Object<DbusmenuMenuitem>> _windows_menu_items and add the items you generate there in AddMenuItemsWindowList.

22 + for ( auto w: Windows() ) {

Use for (auto const& w : Windows())

At that point call it into GetMenus() putting the items you computed into the result list.

36 + _gsignals.Add(
37 + new glib::Signal<void, DbusmenuMenuitem*, int>(menu_item,

You can use the utility Add function to save some space:
_gsignals.Add<void, DbusmenuMenuitem*, int>(menu_item, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED ...

39 + [w,&wm] (DbusmenuMenuitem*,int) -> void {

Just pass w, while generate the WM inside the lambda body (even if that pointer won't change, it's safer not to pass by reference inside a lambda that is going to be executed when the local variable is dead). Also it's not needed to specify the "-> void" return type. g++ will guess it for you.

45 + std::string winName( ( wm.GetWindowName( w->window_id() ) ) );
46 + dbusmenu_menuitem_property_set(menu_item,
47 + DBUSMENU_MENUITEM_PROP_LABEL, winName.c_str() );

Only use:
dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_LABEL, w->title().c_str());

51 + if (windows_added) {
52 + // add seperator
…
65 + }

Do this into GetMenus.

Also, long quicklist items should be cut. I'll provide I way to do it as I need it also on another branch, please ping me on IRC in case you'd need help.

Fix the style to match the one we use in unity (2 spaces indentation, brackets on next line)

Need tests.

review: Needs Fixing

[John Lea (johnlea)]

Also the maximum string length of each Window title should be limited to 45 characters including spaces. Anything more than 45 characters should be chopped at 43 characters and have "..." applied to the end e.g.

"bamficon_windowlist_rearing : Code : Unity..." (In the case of this window)

review: Needs Fixing (design)

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

> Also the maximum string length of each Window title should be limited to 45
> characters including spaces. Anything more than 45 characters should be
> chopped at 43 characters and have "..." applied to the end e.g.
>
> "bamficon_windowlist_rearing : Code : Unity..." (In the case of this window)

Yes, I agree with this (as I wrote above).
The fix for that is needed also by bug #893652 and will be provided by lp:~3v1n0/unity/quicklist-max-label-width.

Just set the QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY DBusMenuItem int property (wanted px-width will be provided by design).

[John Lea (johnlea)]

Update to my comment above:

- The max width should be 300px
- The window title should be ellipsized at the end
- This entry should only be displayed if there are 2 or more windows. If the application only has 1 or 0 windows open nothing should be displayed.

The format of the option should be:

====================
[application specific quicklist options area]
--------------
[application name]
--------------
Windows:
[window title 1]
[window title 2]
[window title 3]
--------------
Most recent files:
file one.odt
file two.odt
file three.odt
file four.odt
file five.odt
--------------
Keep in launcher
Quit
====================

thanks!

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

36 + // delete all menu items for windows
37 + _menu_items_windows.clear();

Please do this on EnsureMenuItemsWindowsReady

17 + _gsignals.Add<void, DbusmenuMenuitem*, int>(menu_item, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED,
18 + [w] (DbusmenuMenuitem*,int) {

This is ok, but since using [w] means copying the the WindowPtr, I'd prefer if you instead do:

Window xid = w->window_id();
_gsignals.Add<void, DbusmenuMenuitem*, int>(menu_item, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, [xid] (DbusmenuMenuitem*,int) {
...
wm.Raise(xid);
}

22 + } );

Remove this padding, please (use also for the lambda two spaces for indenting).

24 + dbusmenu_menuitem_property_set(menu_item, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, "300");

That property is an integer, you should do instead:

dbusmenu_menuitem_property_set_int(menu_item, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, MAXIMUM_QUICKLIST_WIDTH);

Where MAXIMUM_QUICKLIST_WIDTH is a const int that you should define in the unnamed namespace that we have at the top of the ApplicationLauncherIcon.cpp file ;).

40 + // add windows menu items
41 + if (_menu_items_windows.size() > 1 )

Mh, I'd prefer to iterate one more time than allocating some extra memory here, so just use if (Windows().size() > 1), and move EnsureMenuItemsWindowsReady under this check.

Thanks ;)

[Lukas Vacek (lukas-vacek)]

Thanks for your fast reply. I will fix that and push the changes tonight.

> 24 + dbusmenu_menuitem_property_set(menu_item,
> QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, "300");
>
> That property is an integer, you should do instead:
>
> dbusmenu_menuitem_property_set_int(menu_item,
> QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, MAXIMUM_QUICKLIST_WIDTH);
>
> Where MAXIMUM_QUICKLIST_WIDTH is a const int that you should define in the
> unnamed namespace that we have at the top of the ApplicationLauncherIcon.cpp
> file ;).
>

I am not sure about this one - to me it seems the property is const char* and it won't compile if I set it to an integer. I am not sure how to fix my code here.

[Andrea Azzarone (azzar1)]

> Thanks for your fast reply. I will fix that and push the changes tonight.
>
> > 24 + dbusmenu_menuitem_property_set(menu_item,
> > QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, "300");
> >
> > That property is an integer, you should do instead:
> >
> > dbusmenu_menuitem_property_set_int(menu_item,
> > QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, MAXIMUM_QUICKLIST_WIDTH);
> >
> > Where MAXIMUM_QUICKLIST_WIDTH is a const int that you should define in the
> > unnamed namespace that we have at the top of the ApplicationLauncherIcon.cpp
> > file ;).
> >
>
> I am not sure about this one - to me it seems the property is const char* and
> it won't compile if I set it to an integer. I am not sure how to fix my code
> here.

Keep in mind you need to use dbusmenu_menuitem_property_***set_int***

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

Ah, also... It would be needed an unit-test or AP test. If you need some guidance, ping me on IRC.

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

I think it's now ready to go in trunk... We have tests and correctly cut long entries!

review: Approve

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

New screenshot for design re-review: http://i.imgur.com/NUfgdKm.png

[John Lea (johnlea)]

Looks good to me now, thanks!

review: Approve (design)

=== modified file 'launcher/ApplicationLauncherIcon.cpp'
--- launcher/ApplicationLauncherIcon.cpp	2013-02-05 02:14:32 +0000
+++ launcher/ApplicationLauncherIcon.cpp	2013-02-08 15:07:28 +0000
@@ -51,6 +51,7 @@
 const std::string ICON_REMOVE_TIMEOUT = "bamf-icon-remove";
 //const std::string ICON_DND_OVER_TIMEOUT = "bamf-icon-dnd-over";
 const std::string DEFAULT_ICON = "application-default-icon";
+const int MAXIMUM_QUICKLIST_WIDTH = 300;
 }
 
 NUX_IMPLEMENT_OBJECT_TYPE(ApplicationLauncherIcon);
@@ -692,6 +693,42 @@
     EmitNeedsRedraw();
 }
 
+void ApplicationLauncherIcon::EnsureMenuItemsWindowsReady()
+{
+  // delete all menu items for windows
+  _menu_items_windows.clear();
+
+  auto const& windows = Windows();
+
+  // We only add quicklist menu-items for windows if we have more than one window
+  if (windows.size() < 2)
+    return;
+
+  // add menu items for all open windows
+  for (auto const& w : windows)
+  {
+    if (w->title().empty())
+      continue;
+
+    glib::Object<DbusmenuMenuitem> menu_item(dbusmenu_menuitem_new());
+    dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_LABEL, w->title().c_str());
+    dbusmenu_menuitem_property_set_bool(menu_item, DBUSMENU_MENUITEM_PROP_ENABLED, true);
+    dbusmenu_menuitem_property_set_bool(menu_item, DBUSMENU_MENUITEM_PROP_VISIBLE, true);
+    dbusmenu_menuitem_property_set_bool(menu_item, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY, true);
+    dbusmenu_menuitem_property_set_int(menu_item, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, MAXIMUM_QUICKLIST_WIDTH);
+
+    Window xid = w->window_id();
+    _gsignals.Add<void, DbusmenuMenuitem*, int>(menu_item, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED,
+      [xid] (DbusmenuMenuitem*, int) {
+        WindowManager& wm = WindowManager::Default();
+        wm.Activate(xid);
+        wm.Raise(xid);
+    });
+
+    _menu_items_windows.push_back(menu_item);
+  }
+}
+
 void ApplicationLauncherIcon::UpdateMenus()
 {
   // add dynamic quicklist
@@ -926,6 +963,28 @@
   }
   result.push_back(item);
 
+  EnsureMenuItemsWindowsReady();
+
+  if (!_menu_items_windows.empty())
+  {
+    for (auto const& it : _menu_items_windows)
+      result.push_back(it);
+
+    auto third_sep = _menu_items_extra.find("ThirdSeparator");
+    if (third_sep != _menu_items_extra.end())
+    {
+      item = third_sep->second;
+    }
+    else
+    {
+      item = dbusmenu_menuitem_new();
+      dbusmenu_menuitem_property_set(item, DBUSMENU_MENUITEM_PROP_TYPE, DBUSMENU_CLIENT_TYPES_SEPARATOR);
+      _menu_items_extra["ThirdSeparator"] = glib::Object<DbusmenuMenuitem>(item);
+    }
+
+    result.push_back(item);
+  }
+
   EnsureMenuItemsReady();
 
   for (auto it : _menu_items)

=== modified file 'launcher/ApplicationLauncherIcon.h'
--- launcher/ApplicationLauncherIcon.h	2013-02-05 02:14:32 +0000
+++ launcher/ApplicationLauncherIcon.h	2013-02-08 15:07:28 +0000
@@ -108,6 +108,7 @@
   };
 
   void EnsureWindowState();
+  void EnsureMenuItemsWindowsReady();
   void EnsureMenuItemsReady();
   void UpdateBackgroundColor();
   void UpdateMenus();
@@ -130,6 +131,7 @@
   std::map<std::string, glib::Object<DbusmenuClient>> _menu_clients;
   std::map<std::string, glib::Object<DbusmenuMenuitem>> _menu_items;
   std::map<std::string, glib::Object<DbusmenuMenuitem>> _menu_items_extra;
+  std::vector<glib::Object<DbusmenuMenuitem>> _menu_items_windows;
   glib::Object<IndicatorDesktopShortcuts> _desktop_shortcuts;
   glib::Object<DbusmenuMenuitem> _menu_desktop_shortcuts;
   glib::Object<GFileMonitor> _desktop_file_monitor;

=== modified file 'launcher/QuicklistMenuItem.cpp'
--- launcher/QuicklistMenuItem.cpp	2013-01-31 14:54:19 +0000
+++ launcher/QuicklistMenuItem.cpp	2013-02-08 15:07:28 +0000
@@ -29,6 +29,7 @@
 namespace unity
 {
 const char* QuicklistMenuItem::MARKUP_ENABLED_PROPERTY = "unity-use-markup";
+const char* QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY = "unity-disable-accel";
 const char* QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY = "unity-max-label-width";
 const char* QuicklistMenuItem::OVERLAY_MENU_ITEM_PROPERTY = "unity-overlay-item";
 
@@ -280,7 +281,11 @@
   std::shared_ptr<PangoFontDescription> desc(pango_font_description_from_string(font_name), pango_font_description_free);
   pango_layout_set_font_description(layout, desc.get());
   pango_layout_set_wrap(layout, PANGO_WRAP_WORD_CHAR);
-  pango_layout_set_markup_with_accel(layout, _text.c_str(), -1, '_', nullptr);
+
+  if (IsMarkupAccelEnabled())
+    pango_layout_set_markup_with_accel(layout, _text.c_str(), -1, '_', nullptr);
+  else
+    pango_layout_set_markup(layout, _text.c_str(), -1);
 
   if (GetMaxLabelWidth() > 0)
   {
@@ -360,6 +365,24 @@
   return (markup != FALSE);
 }
 
+void QuicklistMenuItem::EnableLabelMarkupAccel(bool enabled)
+{
+  if (IsMarkupAccelEnabled() != enabled)
+  {
+    dbusmenu_menuitem_property_set_bool(_menu_item, MARKUP_ACCEL_DISABLED_PROPERTY, enabled ? FALSE : TRUE);
+    InitializeText();
+  }
+}
+
+bool QuicklistMenuItem::IsMarkupAccelEnabled() const
+{
+  if (!_menu_item)
+    return false;
+
+  gboolean disabled = dbusmenu_menuitem_property_get_bool(_menu_item, MARKUP_ACCEL_DISABLED_PROPERTY);
+  return (disabled == FALSE);
+}
+
 void QuicklistMenuItem::SetMaxLabelWidth(int max_width)
 {
   if (GetMaxLabelWidth() != max_width)

=== modified file 'launcher/QuicklistMenuItem.h'
--- launcher/QuicklistMenuItem.h	2013-01-31 13:41:35 +0000
+++ launcher/QuicklistMenuItem.h	2013-02-08 15:07:28 +0000
@@ -61,6 +61,8 @@
 
   void EnableLabelMarkup(bool enabled);
   bool IsMarkupEnabled() const;
+  void EnableLabelMarkupAccel(bool enabled);
+  bool IsMarkupAccelEnabled() const;
 
   void SetMaxLabelWidth(int max_width);
   int GetMaxLabelWidth() const;
@@ -85,6 +87,7 @@
   sigc::signal<void, QuicklistMenuItem*, int, int> sigMouseDrag;
 
   static const char* MARKUP_ENABLED_PROPERTY;
+  static const char* MARKUP_ACCEL_DISABLED_PROPERTY;
   static const char* MAXIMUM_LABEL_WIDTH_PROPERTY;
   static const char* OVERLAY_MENU_ITEM_PROPERTY;
 

=== modified file 'tests/mock-application.h'
--- tests/mock-application.h	2013-02-05 02:10:45 +0000
+++ tests/mock-application.h	2013-02-08 15:07:28 +0000
@@ -30,6 +30,7 @@
   MockApplicationWindow(Window xid)
     : xid_(xid)
     , monitor_(0)
+    , title_("MockApplicationWindow "+std::to_string(xid_))
     , type_("window")
     , visible_(true)
     , active_(false)

=== modified file 'tests/test_application_launcher_icon.cpp'
--- tests/test_application_launcher_icon.cpp	2013-02-05 02:14:46 +0000
+++ tests/test_application_launcher_icon.cpp	2013-02-08 15:07:28 +0000
@@ -180,4 +180,86 @@
   EXPECT_TRUE(mock_icon->IsActive());
 }
 
+TEST_F(TestApplicationLauncherIcon, NoWindowListMenusWithOneWindow)
+{
+  auto win = std::make_shared<MockApplicationWindow>(g_random_int());
+  mock_app->window_list_ = { win };
+
+  auto const& menus = mock_icon->Menus();
+  auto menu_it = std::find_if(menus.begin(), menus.end(), [win] (glib::Object<DbusmenuMenuitem> it) {
+    auto* label = dbusmenu_menuitem_property_get(it, DBUSMENU_MENUITEM_PROP_LABEL);
+    return (label && std::string(label) == win->title());
+  });
+
+  EXPECT_EQ(menu_it, menus.end());
+}
+
+TEST_F(TestApplicationLauncherIcon, WindowListMenusWithTwoWindows)
+{
+  auto win1 = std::make_shared<MockApplicationWindow>(1);
+  auto wm_win1 = std::make_shared<StandaloneWindow>(win1->window_id());
+  auto win2 = std::make_shared<MockApplicationWindow>(2);
+  auto wm_win2 = std::make_shared<StandaloneWindow>(win2->window_id());
+
+  mock_app->window_list_ = { win1, win2 };
+  WM->AddStandaloneWindow(wm_win1);
+  WM->AddStandaloneWindow(wm_win2);
+  ASSERT_TRUE(wm_win2->active());
+
+  auto const& menus = mock_icon->Menus();
+
+  auto menu1_it = std::find_if(menus.begin(), menus.end(), [win1] (glib::Object<DbusmenuMenuitem> it) {
+    auto* label = dbusmenu_menuitem_property_get(it, DBUSMENU_MENUITEM_PROP_LABEL);
+    return (label && std::string(label) == win1->title());
+  });
+
+  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));
+  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);
+
+  auto menu2_it = std::find_if(menus.begin(), menus.end(), [win2] (glib::Object<DbusmenuMenuitem> it) {
+    auto* label = dbusmenu_menuitem_property_get(it, DBUSMENU_MENUITEM_PROP_LABEL);
+    return (label && std::string(label) == win2->title());
+  });
+
+  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));
+  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);
+
+  bool activated = false;
+  wm_win1->active.changed.connect([&activated] (bool a) { activated = a; });
+  g_signal_emit_by_name(*menu1_it, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, 0);
+
+  EXPECT_TRUE(wm_win1->active());
+  EXPECT_TRUE(activated);
+
+  activated = false;
+  wm_win2->active.changed.connect([&activated] (bool a) { activated = a; });
+  g_signal_emit_by_name(*menu2_it, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, 0);
+
+  EXPECT_TRUE(wm_win2->active());
+  EXPECT_TRUE(activated);
+}
+
+TEST_F(TestApplicationLauncherIcon, WindowListMenusWithEmptyTitles)
+{
+  auto win1 = std::make_shared<MockApplicationWindow>(1);
+  auto win2 = std::make_shared<MockApplicationWindow>(2);
+  win1->title_.clear();
+
+  mock_app->window_list_ = { win1, win2 };
+  auto const& menus = mock_icon->Menus();
+
+  auto menu1_it = std::find_if(menus.begin(), menus.end(), [win1] (glib::Object<DbusmenuMenuitem> it) {
+    auto* label = dbusmenu_menuitem_property_get(it, DBUSMENU_MENUITEM_PROP_LABEL);
+    return (label && std::string(label) == win1->title());
+  });
+
+  ASSERT_EQ(menu1_it, menus.end());
+}
+
 }

=== modified file 'tests/test_quicklist_menu_item.cpp'
--- tests/test_quicklist_menu_item.cpp	2013-01-31 13:43:13 +0000
+++ tests/test_quicklist_menu_item.cpp	2013-02-08 15:07:28 +0000
@@ -139,6 +139,26 @@
   EXPECT_EQ(dbusmenu_menuitem_property_get_int(item, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY), max_width);
 }
 
+TEST_F(TestQuicklistMenuItem, MarkupAccelEnabled)
+{
+  dbusmenu_menuitem_property_set(item, DBUSMENU_MENUITEM_PROP_LABEL, "Label");
+  dbusmenu_menuitem_property_set_bool(item, DBUSMENU_MENUITEM_PROP_ENABLED, true);
+
+  nux::ObjectPtr<QuicklistMenuItemLabel> qlitem(new QuicklistMenuItemLabel(item));
+  EXPECT_TRUE(qlitem->IsMarkupAccelEnabled());
+
+  dbusmenu_menuitem_property_set_bool(item, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY, true);
+  EXPECT_FALSE(qlitem->IsMarkupAccelEnabled());
+
+  qlitem->EnableLabelMarkupAccel(true);
+  EXPECT_TRUE(qlitem->IsMarkupAccelEnabled());
+  EXPECT_FALSE(dbusmenu_menuitem_property_get_bool(item, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY));
+
+  qlitem->EnableLabelMarkupAccel(false);
+  EXPECT_FALSE(qlitem->IsMarkupAccelEnabled());
+  EXPECT_TRUE(dbusmenu_menuitem_property_get_bool(item, QuicklistMenuItem::MARKUP_ACCEL_DISABLED_PROPERTY));
+}
+
 TEST_F(TestQuicklistMenuItem, ItemActivate)
 {
   dbusmenu_menuitem_property_set(item, DBUSMENU_MENUITEM_PROP_LABEL, "Label");