Merge lp:~marcustomlinson/indicator-location/lp-1407921 into lp:indicator-location

Proposed by Marcus Tomlinson on 2016-05-24
Status: Merged
Approved by: Charles Kerr on 2016-06-10
Approved revision: 167
Merged at revision: 156
Proposed branch: lp:~marcustomlinson/indicator-location/lp-1407921
Merge into: lp:indicator-location
Diff against target: 232 lines (+100/-3)
6 files modified
src/controller.h (+1/-0)
src/location-service-controller.cc (+63/-1)
src/location-service-controller.h (+1/-0)
src/phone.cc (+29/-2)
src/phone.h (+1/-0)
tests/controller-mock.h (+5/-0)
To merge this branch: bzr merge lp:~marcustomlinson/indicator-location/lp-1407921
Reviewer Review Type Date Requested Status
Charles Kerr (community) 2016-05-24 Approve on 2016-06-10
PS Jenkins bot continuous-integration Pending
Review via email: mp+295570@code.launchpad.net

Commit message

Give indicator feedback when location-service is being used by apps/scopes

To post a comment you must log in.
167. By Marcus Tomlinson on 2016-06-08

Removed debug

Charles Kerr (charlesk) wrote :

The code looks good; Approved.

Marcus, I do have one optional request though, it would be slightly cleaner to add an enum class { Disabled, Idle, Active } to the controller and expose that state as a property, rather than the current patch which has two slightly overlapping bool states for enabled/disabled active/inactive.

review: Approve
Charles Kerr (charlesk) wrote :

Marcus, also sorry for the delay on this review, I've been on holiday :)

Marcus Tomlinson (marcustomlinson) wrote :

> The code looks good; Approved.
>
> Marcus, I do have one optional request though, it would be slightly cleaner to
> add an enum class { Disabled, Idle, Active } to the controller and expose that
> state as a property, rather than the current patch which has two slightly
> overlapping bool states for enabled/disabled active/inactive.

Yeah, that is definitely a good idea. The motivation behind doing this way though, was in order to allow us to release this independent of the location-service updates. If I depend solely on the "State" property for both the enabled and active states, we would have to land them together.

While both are currently in a silo together, we are currently blocked on the location-service updates. Thomas is looking into it, but at least this way we have the option of landing independently if it comes to that.

Marcus Tomlinson (marcustomlinson) wrote :

> > The code looks good; Approved.
> >
> > Marcus, I do have one optional request though, it would be slightly cleaner
> to
> > add an enum class { Disabled, Idle, Active } to the controller and expose
> that
> > state as a property, rather than the current patch which has two slightly
> > overlapping bool states for enabled/disabled active/inactive.
>
> Yeah, that is definitely a good idea. The motivation behind doing this way
> though, was in order to allow us to release this independent of the location-
> service updates. If I depend solely on the "State" property for both the
> enabled and active states, we would have to land them together.
>
> While both are currently in a silo together, we are currently blocked on the
> location-service updates. Thomas is looking into it, but at least this way we
> have the option of landing independently if it comes to that.

I just realised after hitting save that we could still abstract this behind the controller. If you feel strongly about it, sure, can do.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/controller.h'
2--- src/controller.h 2016-01-22 18:16:12 +0000
3+++ src/controller.h 2016-06-08 05:47:18 +0000
4@@ -32,6 +32,7 @@
5
6 virtual const core::Property<bool>& gps_enabled() const = 0;
7 virtual const core::Property<bool>& location_service_enabled() const = 0;
8+ virtual const core::Property<bool>& location_service_active() const = 0;
9
10 virtual void set_gps_enabled(bool enabled) = 0;
11 virtual void set_location_service_enabled(bool enabled) = 0;
12
13=== modified file 'src/location-service-controller.cc'
14--- src/location-service-controller.cc 2016-01-22 18:16:12 +0000
15+++ src/location-service-controller.cc 2016-06-08 05:47:18 +0000
16@@ -57,6 +57,11 @@
17 return m_loc_enabled;
18 }
19
20+ const core::Property<bool>& location_service_active() const
21+ {
22+ return m_loc_active;
23+ }
24+
25 void set_gps_enabled(bool enabled)
26 {
27 set_bool_property(PROP_KEY_GPS_ENABLED, enabled);
28@@ -137,7 +142,8 @@
29 {
30 const char* name;
31 GAsyncReadyCallback callback;
32- } props[] = {{PROP_KEY_LOC_ENABLED, on_loc_enabled_reply}, {PROP_KEY_GPS_ENABLED, on_gps_enabled_reply}};
33+ } props[] = {{PROP_KEY_LOC_ENABLED, on_loc_enabled_reply}, {PROP_KEY_GPS_ENABLED, on_gps_enabled_reply},
34+ {PROP_KEY_LOC_STATE, on_loc_state_reply}};
35 for (const auto& prop : props)
36 {
37 g_dbus_connection_call(system_bus, BUS_NAME, OBJECT_PATH, PROP_IFACE_NAME, "Get",
38@@ -194,6 +200,11 @@
39 {
40 self->m_gps_enabled.set(g_variant_get_boolean(val));
41 }
42+ else if (!g_strcmp0(key, PROP_KEY_LOC_STATE))
43+ {
44+ auto state_str = std::string(g_variant_get_string(val, nullptr));
45+ self->m_loc_active.set(state_str == "active");
46+ }
47
48 g_variant_unref(val);
49 }
50@@ -238,6 +249,38 @@
51 return std::make_tuple(success, result);
52 }
53
54+ static std::tuple<bool, std::string> get_string_reply_from_call(GObject* source, GAsyncResult* res)
55+ {
56+ GError* error;
57+ GVariant* v;
58+ bool success{false};
59+ std::string result{""};
60+
61+ error = nullptr;
62+ GDBusConnection* conn = G_DBUS_CONNECTION(source);
63+ v = g_dbus_connection_call_finish(conn, res, &error);
64+ if (v != nullptr)
65+ {
66+ GVariant* inner{};
67+ g_variant_get(v, "(v)", &inner);
68+ success = true;
69+ result = g_variant_get_string(inner, nullptr);
70+ g_variant_unref(inner);
71+ g_variant_unref(v);
72+ }
73+ else if (error != nullptr)
74+ {
75+ if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
76+ {
77+ g_warning("Error calling dbus method: %s", error->message);
78+ }
79+ g_error_free(error);
80+ success = false;
81+ }
82+
83+ return std::make_tuple(success, result);
84+ }
85+
86 static void on_loc_enabled_reply(GObject* source_object, GAsyncResult* res, gpointer gself)
87 {
88 bool success, value;
89@@ -260,6 +303,18 @@
90 }
91 }
92
93+ static void on_loc_state_reply(GObject* source_object, GAsyncResult* res, gpointer gself)
94+ {
95+ bool success;
96+ std::string state_str;
97+ std::tie(success, state_str) = get_string_reply_from_call(source_object, res);
98+ g_debug("service state reply: success %d value '%s'", int(success), state_str.c_str());
99+ if (success)
100+ {
101+ static_cast<Impl*>(gself)->m_loc_active.set(state_str == "active");
102+ }
103+ }
104+
105 /***
106 **** org.freedesktop.dbus.properties.Set handling
107 ***/
108@@ -313,9 +368,11 @@
109 static constexpr const char* PROP_IFACE_NAME{"org.freedesktop.DBus.Properties"};
110 static constexpr const char* PROP_KEY_LOC_ENABLED{"IsOnline"};
111 static constexpr const char* PROP_KEY_GPS_ENABLED{"DoesSatelliteBasedPositioning"};
112+ static constexpr const char* PROP_KEY_LOC_STATE{"State"};
113
114 core::Property<bool> m_gps_enabled{false};
115 core::Property<bool> m_loc_enabled{false};
116+ core::Property<bool> m_loc_active{false};
117 core::Property<bool> m_is_valid{false};
118
119 std::shared_ptr<GCancellable> m_cancellable{};
120@@ -352,6 +409,11 @@
121 return impl->location_service_enabled();
122 }
123
124+const core::Property<bool>& LocationServiceController::location_service_active() const
125+{
126+ return impl->location_service_active();
127+}
128+
129 void LocationServiceController::set_gps_enabled(bool enabled)
130 {
131 impl->set_gps_enabled(enabled);
132
133=== modified file 'src/location-service-controller.h'
134--- src/location-service-controller.h 2016-01-22 18:16:12 +0000
135+++ src/location-service-controller.h 2016-06-08 05:47:18 +0000
136@@ -32,6 +32,7 @@
137 const core::Property<bool>& is_valid() const override;
138 const core::Property<bool>& gps_enabled() const override;
139 const core::Property<bool>& location_service_enabled() const override;
140+ const core::Property<bool>& location_service_active() const override;
141 void set_gps_enabled(bool enabled) override;
142 void set_location_service_enabled(bool enabled) override;
143
144
145=== modified file 'src/phone.cc'
146--- src/phone.cc 2016-02-03 19:52:36 +0000
147+++ src/phone.cc 2016-06-08 05:47:18 +0000
148@@ -52,6 +52,12 @@
149 };
150 controller_connections.push_back(controller->location_service_enabled().changed().connect(on_loc));
151
152+ auto on_loc_active = [this](bool active)
153+ {
154+ update_header();
155+ };
156+ controller_connections.push_back(controller->location_service_active().changed().connect(on_loc_active));
157+
158 auto on_valid = [this](bool valid)
159 {
160 update_actions_enabled();
161@@ -90,6 +96,16 @@
162 return controller->location_service_enabled().get();
163 }
164
165+bool Phone::location_service_active() const
166+{
167+ if (!controller->is_valid())
168+ {
169+ return false;
170+ }
171+
172+ return controller->location_service_active().get();
173+}
174+
175 GVariant* Phone::action_state_for_root() const
176 {
177 GVariantBuilder builder;
178@@ -104,8 +120,19 @@
179 gboolean visible = should_be_visible();
180 g_variant_builder_add(&builder, "{sv}", "visible", g_variant_new_boolean(visible));
181
182- const char* icon_name = "gps";
183- GIcon* icon = g_themed_icon_new_with_default_fallbacks(icon_name);
184+ GIcon* icon;
185+ if (!visible)
186+ {
187+ icon = g_themed_icon_new_with_default_fallbacks("location-disabled");
188+ }
189+ else if (location_service_active())
190+ {
191+ icon = g_themed_icon_new_with_default_fallbacks("location-active");
192+ }
193+ else
194+ {
195+ icon = g_themed_icon_new_with_default_fallbacks("location-idle");
196+ }
197 GVariant* serialized_icon = g_icon_serialize(icon);
198 if (serialized_icon != NULL)
199 {
200
201=== modified file 'src/phone.h'
202--- src/phone.h 2016-01-22 18:16:12 +0000
203+++ src/phone.h 2016-06-08 05:47:18 +0000
204@@ -52,6 +52,7 @@
205
206 private:
207 bool should_be_visible() const;
208+ bool location_service_active() const;
209 GVariant* action_state_for_root() const;
210 GSimpleAction* create_root_action();
211 void update_header();
212
213=== modified file 'tests/controller-mock.h'
214--- tests/controller-mock.h 2016-01-22 18:16:12 +0000
215+++ tests/controller-mock.h 2016-06-08 05:47:18 +0000
216@@ -43,6 +43,10 @@
217 {
218 return m_location_service_enabled;
219 }
220+ const core::Property<bool>& location_service_active() const override
221+ {
222+ return m_location_service_active;
223+ }
224
225 void set_gps_enabled(bool enabled) override
226 {
227@@ -57,4 +61,5 @@
228 core::Property<bool> m_is_valid{true};
229 core::Property<bool> m_gps_enabled{false};
230 core::Property<bool> m_location_service_enabled{false};
231+ core::Property<bool> m_location_service_active{false};
232 };

Subscribers

People subscribed via source and target branches