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
1=== modified file 'launcher/LauncherController.cpp'
2--- launcher/LauncherController.cpp 2013-02-14 21:01:58 +0000
3+++ launcher/LauncherController.cpp 2013-02-27 18:35:24 +0000
4@@ -68,6 +68,11 @@
5 " <arg type='s' name='aptdaemon_task' direction='in'/>"
6 " </method>"
7 ""
8+ " <method name='UpdateLauncherIconFavoriteState'>"
9+ " <arg type='s' name='icon_uri' direction='in'/>"
10+ " <arg type='b' name='is_sticky' direction='in'/>"
11+ " </method>"
12+ ""
13 " </interface>"
14 "</node>";
15 }
16@@ -89,6 +94,16 @@
17 const std::string RUNNING_APPS_URI = FavoriteStore::URI_PREFIX_UNITY + "running-apps";
18 const std::string DEVICES_URI = FavoriteStore::URI_PREFIX_UNITY + "devices";
19 }
20+std::string CreateAppUriNameFromDesktopPath(const std::string &desktop_path)
21+{
22+ std::string app_uri;
23+ if (desktop_path.empty())
24+ return "";
25+
26+ app_uri = FavoriteStore::URI_PREFIX_APP +
27+ DesktopUtilities::GetDesktopID(desktop_path);
28+ return app_uri;
29+}
30 }
31
32 GDBusInterfaceVTable Controller::Impl::interface_vtable =
33@@ -348,7 +363,7 @@
34 if (icon_uri.find(FavoriteStore::URI_PREFIX_FILE) == 0)
35 {
36 auto const& desktop_path = icon_uri.substr(FavoriteStore::URI_PREFIX_FILE.length());
37- app_uri = FavoriteStore::URI_PREFIX_APP + DesktopUtilities::GetDesktopID(desktop_path);
38+ app_uri = local::CreateAppUriNameFromDesktopPath(desktop_path);
39 }
40
41 auto const& icon = GetIconByUri(app_uri.empty() ? icon_uri : app_uri);
42@@ -433,6 +448,58 @@
43 }
44
45 void
46+Controller::Impl::OnLauncherUpdateIconStickyState(std::string const& icon_uri, bool sticky)
47+{
48+ if (icon_uri.empty())
49+ return;
50+
51+ std::string target_uri = icon_uri;
52+ if (icon_uri.find(FavoriteStore::URI_PREFIX_FILE) == 0)
53+ {
54+ auto const& desktop_path =
55+ icon_uri.substr(FavoriteStore::URI_PREFIX_FILE.length());
56+
57+ // app uri instead
58+ target_uri = local::CreateAppUriNameFromDesktopPath(desktop_path);
59+ }
60+ auto const& existing_icon_entry =
61+ GetIconByUri(target_uri);
62+
63+ if (existing_icon_entry)
64+ {
65+ // use the backgroung mechanism of model updates & propagation
66+ bool should_update = (existing_icon_entry->IsSticky() != sticky);
67+ if (should_update)
68+ {
69+ if (sticky)
70+ existing_icon_entry->Stick(true);
71+ else
72+ existing_icon_entry->UnStick();
73+
74+ SortAndUpdate();
75+ }
76+ }
77+ else
78+ {
79+ FavoriteStore& favorite_store = FavoriteStore::Instance();
80+
81+ bool should_update = (favorite_store.IsFavorite(target_uri) != sticky);
82+ if (should_update)
83+ {
84+ if (sticky)
85+ {
86+ favorite_store.AddFavorite(target_uri, -1);
87+ RegisterIcon(CreateFavoriteIcon(target_uri));
88+ }
89+ else
90+ {
91+ favorite_store.RemoveFavorite(target_uri);
92+ }
93+ }
94+ }
95+}
96+
97+void
98 Controller::Impl::OnLauncherAddRequestSpecial(std::string const& path,
99 std::string const& aptdaemon_trans_id,
100 std::string const& icon_path,
101@@ -1499,6 +1566,17 @@
102
103 g_dbus_method_invocation_return_value(invocation, nullptr);
104 }
105+ else if (g_strcmp0(method_name, "UpdateLauncherIconFavoriteState") == 0)
106+ {
107+ auto self = static_cast<Controller::Impl*>(user_data);
108+ gboolean is_sticky;
109+ glib::String icon_uri;
110+ g_variant_get(parameters, "(sb)", &icon_uri, &is_sticky);
111+
112+ self->OnLauncherUpdateIconStickyState(icon_uri.Str(), is_sticky);
113+
114+ g_dbus_method_invocation_return_value(invocation, nullptr);
115+ }
116 }
117
118 } // namespace launcher
119
120=== modified file 'launcher/LauncherControllerPrivate.h'
121--- launcher/LauncherControllerPrivate.h 2013-02-09 15:16:41 +0000
122+++ launcher/LauncherControllerPrivate.h 2013-02-27 18:35:24 +0000
123@@ -71,6 +71,7 @@
124 void OnLauncherAddRequest(std::string const& icon_uri, AbstractLauncherIcon::Ptr const& before);
125 void OnLauncherAddRequestSpecial(std::string const& path, std::string const& aptdaemon_trans_id,
126 std::string const& icon_path, int icon_x, int icon_y, int icon_size);
127+ void OnLauncherUpdateIconStickyState(std::string const& desktop_file, bool sticky);
128 void OnLauncherRemoveRequest(AbstractLauncherIcon::Ptr const& icon);
129
130 void OnLauncherEntryRemoteAdded(LauncherEntryRemote::Ptr const& entry);
131
132=== modified file 'tests/test_launcher_controller.cpp'
133--- tests/test_launcher_controller.cpp 2013-02-14 19:30:15 +0000
134+++ tests/test_launcher_controller.cpp 2013-02-27 18:35:24 +0000
135@@ -1692,4 +1692,49 @@
136 Utils::WaitUntil(check_fn, true);
137 }
138
139+TEST_F(TestLauncherController, SetExistingLauncherIconAsFavorite)
140+{
141+ const char * desktop_file = "normal-icon.desktop";
142+ MockApplicationLauncherIcon::Ptr
143+ app_icon(new NiceMock<MockApplicationLauncherIcon>(true, desktop_file));
144+ lc.Impl()->RegisterIcon(app_icon);
145+ ASSERT_FALSE(favorite_store.IsFavorite(app_icon->RemoteUri()));
146+
147+ const std::string icon_uri = FavoriteStore::URI_PREFIX_APP + desktop_file;
148+ lc.Impl()->OnLauncherUpdateIconStickyState(icon_uri, true);
149+
150+ ASSERT_TRUE(app_icon->IsSticky());
151+ EXPECT_TRUE(favorite_store.IsFavorite(app_icon->RemoteUri()));
152+}
153+
154+TEST_F(TestLauncherController, SetExistingLauncherIconAsNonFavorite)
155+{
156+ const char * desktop_file = "normal-icon.desktop";
157+ MockApplicationLauncherIcon::Ptr
158+ app_icon(new NiceMock<MockApplicationLauncherIcon>(true, desktop_file));
159+ lc.Impl()->RegisterIcon(app_icon);
160+ ASSERT_FALSE(favorite_store.IsFavorite(app_icon->RemoteUri()));
161+ app_icon->Stick(true);
162+
163+ EXPECT_CALL(*app_icon, UnStick());
164+
165+ const std::string icon_uri = FavoriteStore::URI_PREFIX_APP + desktop_file;
166+ lc.Impl()->OnLauncherUpdateIconStickyState(icon_uri, false);
167+}
168+
169+TEST_F(TestLauncherController, SetNonExistingLauncherIconAsFavorite)
170+{
171+ std::string desktop = app::BZR_HANDLE_PATCH;
172+ std::string icon_uri = FavoriteStore::URI_PREFIX_APP + DesktopUtilities::GetDesktopID(desktop);
173+
174+ lc.Impl()->OnLauncherUpdateIconStickyState(icon_uri, true);
175+
176+ // Make sure that the icon now exists and is sticky
177+ EXPECT_TRUE(favorite_store.IsFavorite(icon_uri));
178+
179+ auto const& icon = lc.Impl()->GetIconByUri(icon_uri);
180+ ASSERT_TRUE(icon.IsValid());
181+ ASSERT_TRUE(icon->IsSticky());
182+}
183+
184 }