Indicator Location

Merge lp:~charlesk/indicator-location/lp-1435141-fix-false-state into lp:indicator-location/15.04

  1. lp-1435141-fix-false-state
  2. Merge into trunk.15.04
Proposed by Charles Kerr
Status: Merged
Approved by: Antti Kaijanmäki
Approved revision: 150
Merged at revision: 145
Proposed branch: lp:~charlesk/indicator-location/lp-1435141-fix-false-state
Merge into: lp:indicator-location/15.04
Diff against target: 355 lines (+135/-22)
9 files modified
debian/control (+1/-0)
src/controller-mock.h (+7/-6)
src/controller-ualc.cc (+34/-2)
src/controller-ualc.h (+7/-4)
src/controller.h (+5/-1)
src/phone.cc (+22/-8)
src/phone.h (+1/-0)
tests/gtest-dbus-indicator-fixture.h (+7/-0)
tests/phone-test.cc (+51/-1)
To merge this branch: bzr merge lp:~charlesk/indicator-location/lp-1435141-fix-false-state
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Antti Kaijanmäki (community) Approve
Review via email: mp+256332@code.launchpad.net

Commit message

Try harder to keep in sync with platform-api backend and hide the indicator on startup until we know we're synced.

Description of the change

1. The platform-api backend takes awhile to start on some hardware. (Reported on tangxi project, also sometimes repeatable on krillin). Iff our first query to platform-api returns an error, set a timer to try again a few seconds later, and cancel the timer once we've got a successful query.

2. Add an is_valid bool property to Controller and have it be true iff we're able to succesfully talking to platform-api. Hide the indicator until is_valid becomes true, and also make the loc/gps menuitems disabled when is_valid is false so that testers can't mistakenly set a state that will be discarded.

3. Add unit tests to track these changes.

Tested on (1) krillin rtm r270 and (2) mako vivid r171

To post a comment you must log in.
Revision history for this message
Antti Kaijanmäki (kaijanmaki) wrote :

Approved.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:150
http://jenkins.qa.ubuntu.com/job/indicator-location-ci/61/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-location-vivid-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-location-vivid-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/indicator-location-vivid-armhf-ci/4/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-location-ci/61/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2014-09-02 16:56:38 +0000
3+++ debian/control 2015-04-15 14:41:11 +0000
4@@ -15,6 +15,7 @@
5 liburl-dispatcher1-dev,
6 libubuntu-application-api-dev,
7 python,
8+ libproperties-cpp-dev,
9 Standards-Version: 3.9.4
10 Homepage: http://launchpad.net/indicator-location/
11 # If you aren't a member of ~indicator-applet-developers but need to upload
12
13=== modified file 'src/controller-mock.h'
14--- src/controller-mock.h 2013-08-25 20:31:26 +0000
15+++ src/controller-mock.h 2015-04-15 14:41:11 +0000
16@@ -26,10 +26,11 @@
17 {
18 public:
19
20- MockController (): valid(true), gps(false), loc(false) { }
21- virtual ~MockController() { }
22+ MockController() =default;
23+ virtual ~MockController() =default;
24
25- bool is_valid () const { return valid; }
26+ core::Property<bool>& is_valid() { return m_is_valid; }
27+ const core::Property<bool>& is_valid() const override { return m_is_valid; }
28 bool is_gps_enabled () const { return gps; }
29 bool is_location_service_enabled () const { return loc; }
30
31@@ -38,9 +39,9 @@
32
33 private:
34
35- bool valid;
36- bool gps;
37- bool loc;
38+ core::Property<bool> m_is_valid {true};
39+ bool gps { false };
40+ bool loc { false };
41 };
42
43 #endif // __INDICATOR_LOCATION_CONTROLLER_MOCK__H__
44
45=== modified file 'src/controller-ualc.cc'
46--- src/controller-ualc.cc 2014-08-07 19:46:08 +0000
47+++ src/controller-ualc.cc 2015-04-15 14:41:11 +0000
48@@ -37,6 +37,9 @@
49
50 UbuntuAppLocController :: ~UbuntuAppLocController ()
51 {
52+ if (m_update_status_tag)
53+ g_source_remove(m_update_status_tag);
54+
55 if (ualc != nullptr)
56 ua_location_service_controller_unref (ualc);
57 }
58@@ -48,17 +51,46 @@
59 }
60
61 void
62+UbuntuAppLocController :: update_status_soon()
63+{
64+ if (m_update_status_tag == 0)
65+ {
66+ constexpr int retry_interval_seconds = 5;
67+
68+ auto callback = [](gpointer gself){
69+ g_debug("%s periodic check", G_STRLOC);
70+ auto self = static_cast<UbuntuAppLocController*>(gself);
71+ self->m_update_status_tag = 0;
72+ self->update_status();
73+ return G_SOURCE_REMOVE;
74+ };
75+
76+ m_update_status_tag = g_timeout_add_seconds(retry_interval_seconds,
77+ callback,
78+ this);
79+ }
80+}
81+
82+void
83 UbuntuAppLocController :: update_status ()
84 {
85 const bool loc_was_enabled = is_location_service_enabled();
86 const bool gps_was_enabled = is_gps_enabled();
87
88 // update this.current_status with a fresh ualc status
89- UALocationServiceStatusFlags flags;
90- if (ua_location_service_controller_query_status (ualc, &flags) == U_STATUS_SUCCESS)
91+ UALocationServiceStatusFlags flags {};
92+ auto status = ua_location_service_controller_query_status(ualc, &flags);
93+ if (status == U_STATUS_SUCCESS)
94 {
95 g_debug("%s %s updating ualc controller status with flags: %d", G_STRLOC, G_STRFUNC, (int)flags);
96 current_status = flags;
97+ m_is_valid.set(true);
98+ }
99+ else
100+ {
101+ g_warning("%s %s ualc_query_status returned %d", G_STRLOC, G_STRFUNC, status);
102+ m_is_valid.set(false);
103+ update_status_soon();
104 }
105
106 const bool loc_is_enabled = is_location_service_enabled();
107
108=== modified file 'src/controller-ualc.h'
109--- src/controller-ualc.h 2014-08-07 19:46:08 +0000
110+++ src/controller-ualc.h 2015-04-15 14:41:11 +0000
111@@ -33,7 +33,7 @@
112 UbuntuAppLocController ();
113 virtual ~UbuntuAppLocController();
114
115- bool is_valid () const { return ualc != nullptr; }
116+ virtual const core::Property<bool>& is_valid() const override { return m_is_valid; }
117 bool is_gps_enabled () const { return (current_status & UA_LOCATION_SERVICE_GPS_ENABLED) != 0; }
118 bool is_location_service_enabled () const { return (current_status & UA_LOCATION_SERVICE_ENABLED) != 0; }
119
120@@ -42,11 +42,14 @@
121
122 private:
123
124- UALocationServiceStatusFlags current_status = 0;
125- UbuntuApplicationLocationServiceController * ualc;
126+ UALocationServiceStatusFlags current_status {};
127+ UbuntuApplicationLocationServiceController* ualc {};
128+ core::Property<bool> m_is_valid {false};
129+ unsigned int m_update_status_tag {};
130
131 static void on_ualc_status_changed (UALocationServiceStatusFlags, void *vself);
132- void update_status ();
133+ void update_status();
134+ void update_status_soon();
135 };
136
137 #endif // __INDICATOR_LOCATION_CONTROLLER_UALC__H__
138
139=== modified file 'src/controller.h'
140--- src/controller.h 2013-08-25 20:31:26 +0000
141+++ src/controller.h 2015-04-15 14:41:11 +0000
142@@ -22,6 +22,8 @@
143
144 #include <set>
145
146+#include <core/property.h>
147+
148 class ControllerListener
149 {
150 public:
151@@ -43,7 +45,9 @@
152 void add_listener (ControllerListener *);
153 void remove_listener (ControllerListener *);
154
155- virtual bool is_valid () const = 0;
156+ /// True iff we've gotten status info from the location service
157+ virtual const core::Property<bool>& is_valid() const =0;
158+
159 virtual bool is_gps_enabled () const = 0;
160 virtual bool is_location_service_enabled () const = 0;
161
162
163=== modified file 'src/phone.cc'
164--- src/phone.cc 2014-10-03 16:48:55 +0000
165+++ src/phone.cc 2015-04-15 14:41:11 +0000
166@@ -29,6 +29,9 @@
167
168 #define PROFILE_NAME "phone"
169
170+#define LOCATION_ACTION_KEY "location-detection-enabled"
171+#define GPS_ACTION_KEY "gps-detection-enabled"
172+
173 Phone :: Phone (const std::shared_ptr<Controller>& controller_,
174 const std::shared_ptr<LicenseController>& license_controller_,
175 const std::shared_ptr<GSimpleActionGroup>& action_group_):
176@@ -51,6 +54,10 @@
177 g_action_map_add_action (G_ACTION_MAP(action_group.get()), G_ACTION(a));
178 g_object_unref (a);
179 }
180+
181+ // the profile should track whether the controller is valid or not
182+ controller->is_valid().changed().connect([this](bool){on_is_valid_changed();});
183+ on_is_valid_changed();
184 }
185
186 Phone :: ~Phone ()
187@@ -66,6 +73,9 @@
188 bool
189 Phone :: should_be_visible () const
190 {
191+ if (!controller->is_valid())
192+ return false;
193+
194 // as per "Indicators - RTM Usability Fix" document:
195 // visible iff location and/or GPS is enabled
196 return controller->is_location_service_enabled()
197@@ -118,12 +128,22 @@
198 action_state_for_root());
199 }
200
201+void
202+Phone :: on_is_valid_changed()
203+{
204+ const auto map = G_ACTION_MAP(action_group.get());
205+ const bool is_valid = controller->is_valid().get();
206+ std::array<const char*,2> keys = { LOCATION_ACTION_KEY, GPS_ACTION_KEY };
207+ for(const auto& key : keys)
208+ g_simple_action_set_enabled(G_SIMPLE_ACTION(g_action_map_lookup_action(map, key)), is_valid);
209+
210+ update_header();
211+}
212+
213 /***
214 ****
215 ***/
216
217-#define LOCATION_ACTION_KEY "location-detection-enabled"
218-
219 GVariant *
220 Phone :: action_state_for_location_detection ()
221 {
222@@ -168,8 +188,6 @@
223 nullptr,
224 action_state_for_location_detection());
225
226- g_simple_action_set_enabled (action, controller->is_valid());
227-
228 g_signal_connect (action, "activate",
229 G_CALLBACK(on_detection_location_activated), this);
230
231@@ -180,8 +198,6 @@
232 ****
233 ***/
234
235-#define GPS_ACTION_KEY "gps-detection-enabled"
236-
237 GVariant *
238 Phone :: action_state_for_gps_detection ()
239 {
240@@ -215,8 +231,6 @@
241 nullptr,
242 action_state_for_gps_detection());
243
244- g_simple_action_set_enabled (action, controller->is_valid());
245-
246 g_signal_connect (action, "activate",
247 G_CALLBACK(on_detection_gps_activated), this);
248
249
250=== modified file 'src/phone.h'
251--- src/phone.h 2014-10-03 16:48:55 +0000
252+++ src/phone.h 2015-04-15 14:41:11 +0000
253@@ -40,6 +40,7 @@
254 protected:
255 std::shared_ptr<Controller> controller;
256 std::shared_ptr<LicenseController> license_controller;
257+ virtual void on_is_valid_changed();
258 virtual void on_gps_enabled_changed (bool is_enabled);
259 virtual void on_location_service_enabled_changed (bool is_enabled);
260 void on_license_accepted_changed(bool license_accepted) override;
261
262=== modified file 'tests/gtest-dbus-indicator-fixture.h'
263--- tests/gtest-dbus-indicator-fixture.h 2014-10-03 16:48:12 +0000
264+++ tests/gtest-dbus-indicator-fixture.h 2015-04-15 14:41:11 +0000
265@@ -181,6 +181,13 @@
266 g_free (signal_name);
267 }
268
269+ void wait_for_action_enabled_change (const gchar * action_name)
270+ {
271+ gchar * signal_name = g_strdup_printf ("action-enabled-changed::%s", action_name);
272+ wait_for_signal (action_group, signal_name);
273+ g_free (signal_name);
274+ }
275+
276 protected:
277
278 bool find_menu_item_for_action (const char * action_key, GMenuModel ** setme, int * item_index)
279
280=== modified file 'tests/phone-test.cc'
281--- tests/phone-test.cc 2014-10-03 16:55:46 +0000
282+++ tests/phone-test.cc 2015-04-15 14:41:11 +0000
283@@ -91,6 +91,7 @@
284 ASSERT_TRUE (action_exists ("location-detection-enabled"));
285 ASSERT_TRUE (action_exists ("phone-header"));
286 ASSERT_TRUE (action_exists ("settings"));
287+ ASSERT_TRUE (action_exists ("licence"));
288 }
289
290 TEST_F (PhoneTest, MenuitemsExist)
291@@ -100,6 +101,55 @@
292 //ASSERT_TRUE (action_menuitem_exists ("indicator.settings"));
293 }
294
295+TEST_F (PhoneTest, IsValidEnabled)
296+{
297+ bool is_valid = myController->is_valid().get();
298+ auto ag = G_ACTION_GROUP(action_group);
299+ constexpr int n_iters = 4;
300+ const std::array<const char*,2> action_names = { "gps-detection-enabled",
301+ "location-detection-enabled" };
302+
303+ // test that the loc/gps actions are enabled/disabled based on controller->is_valid()
304+ for (int i=0; i<n_iters; ++i)
305+ {
306+ is_valid = !is_valid;
307+ myController->is_valid().set(is_valid);
308+ wait_for_action_enabled_change(action_names[0]);
309+ for(const auto& action_name : action_names)
310+ {
311+ EXPECT_EQ(is_valid, g_action_group_get_action_enabled(ag, action_name));
312+ }
313+ }
314+}
315+
316+TEST_F (PhoneTest, IsValidVisible)
317+{
318+ // make sure something's enabled so that the indicator should be visible
319+ if (!myController->is_gps_enabled()) {
320+ myController->set_gps_enabled(true);
321+ wait_for_action_state_change("gps-detection-enabled");
322+ }
323+
324+ // test the header's 'invisible' entry tracks the controller's is_valid() state
325+ constexpr int n_iters = 4;
326+ const char* header_action_name = INDICATOR_PROFILE "-header";
327+ auto ag = G_ACTION_GROUP(action_group);
328+ for (int i=0; i<n_iters; ++i)
329+ {
330+ myController->is_valid().set(!myController->is_valid().get());
331+ wait_for_action_state_change(header_action_name);
332+
333+ auto dict = g_action_group_get_action_state(ag, header_action_name);
334+ EXPECT_TRUE(dict != nullptr);
335+ EXPECT_TRUE(g_variant_is_of_type(dict, G_VARIANT_TYPE_VARDICT));
336+ auto v = g_variant_lookup_value(dict, "visible", G_VARIANT_TYPE_BOOLEAN);
337+ EXPECT_TRUE(v != nullptr);
338+ EXPECT_EQ(myController->is_valid().get(), g_variant_get_boolean(v));
339+ g_clear_pointer(&v, g_variant_unref);
340+ g_clear_pointer(&dict, g_variant_unref);
341+ }
342+}
343+
344 TEST_F (PhoneTest, PlatformTogglesGPS)
345 {
346 bool enabled;
347@@ -255,7 +305,7 @@
348 EXPECT_EQ(std::string("indicator.")+action_name, str);
349 g_clear_pointer(&str, g_free);
350
351- // cusory first look at the header
352+ // cursory first look at the header
353 auto dict = g_action_group_get_action_state(action_group, action_name);
354 EXPECT_TRUE(dict != nullptr);
355 EXPECT_TRUE(g_variant_is_of_type(dict, G_VARIANT_TYPE_VARDICT));

Subscribers

People subscribed via source and target branches