Merge lp:~abreu-alexandre/unity/dbus-stick-unstick-launcher-icon into lp:unity

Proposed by Alexandre Abreu on 2013-02-21
Status: Merged
Approved by: Marco Trevisan (Treviño) on 2013-03-06
Approved revision: 3168
Merged at revision: 3188
Proposed branch: lp:~abreu-alexandre/unity/dbus-stick-unstick-launcher-icon
Merge into: lp:unity
Diff against target: 184 lines (+125/-1)
3 files modified
launcher/LauncherController.cpp (+79/-1)
launcher/LauncherControllerPrivate.h (+1/-0)
tests/test_launcher_controller.cpp (+45/-0)
To merge this branch: bzr merge lp:~abreu-alexandre/unity/dbus-stick-unstick-launcher-icon
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve on 2013-03-06
Marco Trevisan (Treviño) 2013-02-21 Approve on 2013-03-06
Review via email: mp+149935@code.launchpad.net

Description of the change

Add method to Launcher dbus interface to stick/unstick a launcher icon.

Needed in order to fix:

https://bugs.launchpad.net/libunity-webapps/+bug/1061056

To post a comment you must log in.
Marco Trevisan (Treviño) (3v1n0) wrote :

22 + std::string app_uri;
23 + if (desktop_path.empty())
24 + return app_uri;

You can just return "" in that case, without initializing app_uri.
Also at the end of the function returning a temporary it's fine anyway..

46 +Controller::Impl::OnLauncherUpdateIconStickyState(std::string const& icon_uri,
47 + bool sticky)

Please, keep everything in one-line.

61 + AbstractLauncherIcon::Ptr existing_icon_entry =
62 + GetIconByUri(target_uri);

One line, plus use const& (or auto const&).

72 + SortAndUpdate();

Please avoid to do this, if nothing has been actually changed.

86 + else
87 + favorite_store.RemoveFavorite(target_uri);

Add brackets here too please. However in that case, when can happen that a favorite has not a related application icon? Going in edge cases it's ok, anyway.

140 + ASSERT_FALSE(favorite_store.IsFavorite(app_icon->RemoteUri()));

This is quite redundant as it should be guarantee by other tests we have.

3168. By Alexandre Abreu on 2013-02-27

Fix MR nits

Alexandre Abreu (abreu-alexandre) wrote :

> 22 + std::string app_uri;
> 23 + if (desktop_path.empty())
> 24 + return app_uri;
>
> You can just return "" in that case, without initializing app_uri.

Done.

> Also at the end of the function returning a temporary it's fine anyway..

mmh, it's returned by value & copy constructed ...

> 46 +Controller::Impl::OnLauncherUpdateIconStickyState(std::string const&
> icon_uri,
> 47 + bool sticky)
>
> Please, keep everything in one-line.

Done.

>
>
> 61 + AbstractLauncherIcon::Ptr existing_icon_entry =
> 62 + GetIconByUri(target_uri);
>
> One line, plus use const& (or auto const&).

Done.

>
>
> 72 + SortAndUpdate();
>
> Please avoid to do this, if nothing has been actually changed.

Good catch, done.

>
>
> 86 + else
> 87 + favorite_store.RemoveFavorite(target_uri);
>
> Add brackets here too please. However in that case, when can happen that a
> favorite has not a related application icon? Going in edge cases it's ok,
> anyway.

Done.

>
>
> 140 + ASSERT_FALSE(favorite_store.IsFavorite(app_icon->RemoteUri()));
>
> This is quite redundant as it should be guarantee by other tests we have.

Yeah, but it is somewhat consistent w/ the test's logic & strict internals here imo,

Alexandre Abreu (abreu-alexandre) wrote :

ping :)

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

Ops, sorry I missed, this... About app_uri variable I meant to remove it at all, but it's not a big thing, so we can stay with it, considering that I can clean it when merging this with lp:~3v1n0/unity/gdbus-server-use, so approved! ;)

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'launcher/LauncherController.cpp'
--- launcher/LauncherController.cpp 2013-02-14 21:01:58 +0000
+++ launcher/LauncherController.cpp 2013-02-27 18:35:24 +0000
@@ -68,6 +68,11 @@
68 " <arg type='s' name='aptdaemon_task' direction='in'/>"68 " <arg type='s' name='aptdaemon_task' direction='in'/>"
69 " </method>"69 " </method>"
70 ""70 ""
71 " <method name='UpdateLauncherIconFavoriteState'>"
72 " <arg type='s' name='icon_uri' direction='in'/>"
73 " <arg type='b' name='is_sticky' direction='in'/>"
74 " </method>"
75 ""
71 " </interface>"76 " </interface>"
72 "</node>";77 "</node>";
73}78}
@@ -89,6 +94,16 @@
89 const std::string RUNNING_APPS_URI = FavoriteStore::URI_PREFIX_UNITY + "running-apps";94 const std::string RUNNING_APPS_URI = FavoriteStore::URI_PREFIX_UNITY + "running-apps";
90 const std::string DEVICES_URI = FavoriteStore::URI_PREFIX_UNITY + "devices";95 const std::string DEVICES_URI = FavoriteStore::URI_PREFIX_UNITY + "devices";
91}96}
97std::string CreateAppUriNameFromDesktopPath(const std::string &desktop_path)
98{
99 std::string app_uri;
100 if (desktop_path.empty())
101 return "";
102
103 app_uri = FavoriteStore::URI_PREFIX_APP +
104 DesktopUtilities::GetDesktopID(desktop_path);
105 return app_uri;
106}
92}107}
93108
94GDBusInterfaceVTable Controller::Impl::interface_vtable =109GDBusInterfaceVTable Controller::Impl::interface_vtable =
@@ -348,7 +363,7 @@
348 if (icon_uri.find(FavoriteStore::URI_PREFIX_FILE) == 0)363 if (icon_uri.find(FavoriteStore::URI_PREFIX_FILE) == 0)
349 {364 {
350 auto const& desktop_path = icon_uri.substr(FavoriteStore::URI_PREFIX_FILE.length());365 auto const& desktop_path = icon_uri.substr(FavoriteStore::URI_PREFIX_FILE.length());
351 app_uri = FavoriteStore::URI_PREFIX_APP + DesktopUtilities::GetDesktopID(desktop_path);366 app_uri = local::CreateAppUriNameFromDesktopPath(desktop_path);
352 }367 }
353368
354 auto const& icon = GetIconByUri(app_uri.empty() ? icon_uri : app_uri);369 auto const& icon = GetIconByUri(app_uri.empty() ? icon_uri : app_uri);
@@ -433,6 +448,58 @@
433}448}
434449
435void450void
451Controller::Impl::OnLauncherUpdateIconStickyState(std::string const& icon_uri, bool sticky)
452{
453 if (icon_uri.empty())
454 return;
455
456 std::string target_uri = icon_uri;
457 if (icon_uri.find(FavoriteStore::URI_PREFIX_FILE) == 0)
458 {
459 auto const& desktop_path =
460 icon_uri.substr(FavoriteStore::URI_PREFIX_FILE.length());
461
462 // app uri instead
463 target_uri = local::CreateAppUriNameFromDesktopPath(desktop_path);
464 }
465 auto const& existing_icon_entry =
466 GetIconByUri(target_uri);
467
468 if (existing_icon_entry)
469 {
470 // use the backgroung mechanism of model updates & propagation
471 bool should_update = (existing_icon_entry->IsSticky() != sticky);
472 if (should_update)
473 {
474 if (sticky)
475 existing_icon_entry->Stick(true);
476 else
477 existing_icon_entry->UnStick();
478
479 SortAndUpdate();
480 }
481 }
482 else
483 {
484 FavoriteStore& favorite_store = FavoriteStore::Instance();
485
486 bool should_update = (favorite_store.IsFavorite(target_uri) != sticky);
487 if (should_update)
488 {
489 if (sticky)
490 {
491 favorite_store.AddFavorite(target_uri, -1);
492 RegisterIcon(CreateFavoriteIcon(target_uri));
493 }
494 else
495 {
496 favorite_store.RemoveFavorite(target_uri);
497 }
498 }
499 }
500}
501
502void
436Controller::Impl::OnLauncherAddRequestSpecial(std::string const& path,503Controller::Impl::OnLauncherAddRequestSpecial(std::string const& path,
437 std::string const& aptdaemon_trans_id,504 std::string const& aptdaemon_trans_id,
438 std::string const& icon_path,505 std::string const& icon_path,
@@ -1499,6 +1566,17 @@
14991566
1500 g_dbus_method_invocation_return_value(invocation, nullptr);1567 g_dbus_method_invocation_return_value(invocation, nullptr);
1501 }1568 }
1569 else if (g_strcmp0(method_name, "UpdateLauncherIconFavoriteState") == 0)
1570 {
1571 auto self = static_cast<Controller::Impl*>(user_data);
1572 gboolean is_sticky;
1573 glib::String icon_uri;
1574 g_variant_get(parameters, "(sb)", &icon_uri, &is_sticky);
1575
1576 self->OnLauncherUpdateIconStickyState(icon_uri.Str(), is_sticky);
1577
1578 g_dbus_method_invocation_return_value(invocation, nullptr);
1579 }
1502}1580}
15031581
1504} // namespace launcher1582} // namespace launcher
15051583
=== modified file 'launcher/LauncherControllerPrivate.h'
--- launcher/LauncherControllerPrivate.h 2013-02-09 15:16:41 +0000
+++ launcher/LauncherControllerPrivate.h 2013-02-27 18:35:24 +0000
@@ -71,6 +71,7 @@
71 void OnLauncherAddRequest(std::string const& icon_uri, AbstractLauncherIcon::Ptr const& before);71 void OnLauncherAddRequest(std::string const& icon_uri, AbstractLauncherIcon::Ptr const& before);
72 void OnLauncherAddRequestSpecial(std::string const& path, std::string const& aptdaemon_trans_id,72 void OnLauncherAddRequestSpecial(std::string const& path, std::string const& aptdaemon_trans_id,
73 std::string const& icon_path, int icon_x, int icon_y, int icon_size);73 std::string const& icon_path, int icon_x, int icon_y, int icon_size);
74 void OnLauncherUpdateIconStickyState(std::string const& desktop_file, bool sticky);
74 void OnLauncherRemoveRequest(AbstractLauncherIcon::Ptr const& icon);75 void OnLauncherRemoveRequest(AbstractLauncherIcon::Ptr const& icon);
7576
76 void OnLauncherEntryRemoteAdded(LauncherEntryRemote::Ptr const& entry);77 void OnLauncherEntryRemoteAdded(LauncherEntryRemote::Ptr const& entry);
7778
=== modified file 'tests/test_launcher_controller.cpp'
--- tests/test_launcher_controller.cpp 2013-02-14 19:30:15 +0000
+++ tests/test_launcher_controller.cpp 2013-02-27 18:35:24 +0000
@@ -1692,4 +1692,49 @@
1692 Utils::WaitUntil(check_fn, true);1692 Utils::WaitUntil(check_fn, true);
1693}1693}
16941694
1695TEST_F(TestLauncherController, SetExistingLauncherIconAsFavorite)
1696{
1697 const char * desktop_file = "normal-icon.desktop";
1698 MockApplicationLauncherIcon::Ptr
1699 app_icon(new NiceMock<MockApplicationLauncherIcon>(true, desktop_file));
1700 lc.Impl()->RegisterIcon(app_icon);
1701 ASSERT_FALSE(favorite_store.IsFavorite(app_icon->RemoteUri()));
1702
1703 const std::string icon_uri = FavoriteStore::URI_PREFIX_APP + desktop_file;
1704 lc.Impl()->OnLauncherUpdateIconStickyState(icon_uri, true);
1705
1706 ASSERT_TRUE(app_icon->IsSticky());
1707 EXPECT_TRUE(favorite_store.IsFavorite(app_icon->RemoteUri()));
1708}
1709
1710TEST_F(TestLauncherController, SetExistingLauncherIconAsNonFavorite)
1711{
1712 const char * desktop_file = "normal-icon.desktop";
1713 MockApplicationLauncherIcon::Ptr
1714 app_icon(new NiceMock<MockApplicationLauncherIcon>(true, desktop_file));
1715 lc.Impl()->RegisterIcon(app_icon);
1716 ASSERT_FALSE(favorite_store.IsFavorite(app_icon->RemoteUri()));
1717 app_icon->Stick(true);
1718
1719 EXPECT_CALL(*app_icon, UnStick());
1720
1721 const std::string icon_uri = FavoriteStore::URI_PREFIX_APP + desktop_file;
1722 lc.Impl()->OnLauncherUpdateIconStickyState(icon_uri, false);
1723}
1724
1725TEST_F(TestLauncherController, SetNonExistingLauncherIconAsFavorite)
1726{
1727 std::string desktop = app::BZR_HANDLE_PATCH;
1728 std::string icon_uri = FavoriteStore::URI_PREFIX_APP + DesktopUtilities::GetDesktopID(desktop);
1729
1730 lc.Impl()->OnLauncherUpdateIconStickyState(icon_uri, true);
1731
1732 // Make sure that the icon now exists and is sticky
1733 EXPECT_TRUE(favorite_store.IsFavorite(icon_uri));
1734
1735 auto const& icon = lc.Impl()->GetIconByUri(icon_uri);
1736 ASSERT_TRUE(icon.IsValid());
1737 ASSERT_TRUE(icon->IsSticky());
1738}
1739
1695}1740}