The Location object must be able to return speed and direction in which the device is headed to. It would be beneficial for moving devices.
Created attachment 113926 [details] [review] Add property Speed to Location object In order to add Speed to the Location object, Speed was added as a property of type double inside the org.GeoClue2.xml file. To make it work, a speed parameter of type double was added to GeocodeLocationPrivate. Also, its getters and setters along with parameter specification were added to geocode-location.c file. Corresponding prototypes were added to geocode-location.h file. Speed was introduced inside get_property() and set_property() methods of GClueServiceLocation object. Speed was introduced inside set_location() method of GClueLocationSource object. GClueLocator object's set_location() method was updated in order to calculate speed on the basis of current location and the new location.
Created attachment 113927 [details] [review] Add property Heading to Location object Heading tells the direction in which the device is headed to. In order to add Heading to the Location object, Heading was added as a property of type unsigned int inside the org.GeoClue2.xml file. To make it work, a heading parameter of type double was added to GeocodeLocationPrivate. Also, its getters and setters along with parameter specification were added to geocode-location.c file. Corresponding prototypes were added to geocode-location.h file along with the macros for each direction's code and for nowhere. GeocodeHeading enum was added to gclue-enums.h for direction codes. Heading was introduced inside get_property() and set_property() methods of GClueServiceLocation object. Heading was introduced inside set_location() method of GClueLocationSource object. GClueLocator object's set_location() method was updated in order to calculate new heading direction on the basis of current location and the new location, it uses get_heading_direction() method of GeocodeLocation object which was also created in this commit.
Thanks for the patches. While I'll have a detailed look into them tomorrow, I just wanted to point out something so you might be able to improve your patches slightly before I get to review them properly. Firstly kudos for writing detailed log messages. However details part is not about describing every line of change in English but rather: 1. Summarizing the changes. 2. Describing all other details not found in the code: Rationale, benefits and any historical references (e.g this was broken in commit abc123) etc. Please go through git log to get an idea.
Created attachment 113948 [details] [review] Add property Speed to Location object The speed is calculated in meters per second on location change for the device. This property will be helpful in implementating code for moving devices.
Created attachment 113949 [details] [review] Add property Heading to Location object Heading property tells the direction in which the device is headed to. This property will be helpful in implementating code for moving devices. It's also calculating the new direction on location change.
Comment on attachment 113948 [details] [review] Add property Speed to Location object Review of attachment 113948 [details] [review]: ----------------------------------------------------------------- Really good work! Needs work still but as I warned you, this won't be a trivial bug to fix and its a good thing that you'll learn a lot about how to work with geoclue project by reworking your patches as per reviews. I would divide this further into 3 patches: 1. Introduction of speed property to Location. I guess this patch will go to geocode-glib project (see inline comment below) so the patch you'll add to geoclue is syncing of geocode-glib submodule (there is a update-from-geocode-glib.sh to do that for you. 2. Introduction of speed property to ServiceLocation (incl copying it over from Location). 2. Setting of this property by Locator. ::: src/gclue-locator.c @@ +74,5 @@ > GeocodeLocation *location) > { > GeocodeLocation *cur_location; > + guint64 cur_timestamp, timestamp; > + const gdouble ALPHA = 0.12; // This value can modulate noise in speed. modulate -> reduce? I like the code readable for math-illiterate as much as possible. :) @@ +97,5 @@ > } > > + if (cur_location == NULL) { > + // if it is the first location value > + speed = 0.0; no need for this variable in this case and no need to set speed on the location either. Just ensure its set to 0.0 by default. @@ +103,5 @@ > + cur_timestamp = geocode_location_get_timestamp (cur_location); > + timestamp = geocode_location_get_timestamp (location); > + cur_speed = geocode_location_get_speed (cur_location); > + /* applying low pass filter to reduce noise here > + * Speed[i] = Speed[i-1]*APLHA + Speed[i]*(1-ALPHA) */ What kind of noise does this exactly reduce? I don't see why we need these complications here, just derive speed from prev and new location. No need to look at previous speed either as that shouldn't matter with new speed. ::: src/geocode-glib/geocode-location.c @@ +43,4 @@ > gdouble latitude; > gdouble altitude; > gdouble accuracy; > + gdouble speed; the source code in this subdir is the geocode-glib project and must be retained as is. If we want to edit anything in this subdir, we need to change in the upstream geocode-glib project and then just update here. Geocode-glib git repo is here: https://git.gnome.org/browse/geocode-glib/ and its bugzilla is here: https://bugzilla.gnome.org/enter_bug.cgi?product=geocode-glib
Comment on attachment 113948 [details] [review] Add property Speed to Location object Review of attachment 113948 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-locator.c @@ +103,5 @@ > + cur_timestamp = geocode_location_get_timestamp (cur_location); > + timestamp = geocode_location_get_timestamp (location); > + cur_speed = geocode_location_get_speed (cur_location); > + /* applying low pass filter to reduce noise here > + * Speed[i] = Speed[i-1]*APLHA + Speed[i]*(1-ALPHA) */ I can't see how that code could work either (APLHA?). ::: src/geocode-glib/geocode-location.c @@ +496,5 @@ > + */ > + pspec = g_param_spec_double ("speed", > + "Speed", > + "The speed of the device in meters per second", > + 0.0, That would mean that application could differentiate between "no speed available" and "not moving". iOS uses negative values to mean "no speed available": https://developer.apple.com/library/ios/documentation/CoreLocation/Reference/CLLocation_Class/index.html#//apple_ref/occ/instp/CLLocation/speed @@ +781,5 @@ > + **/ > +gdouble > +geocode_location_get_speed (GeocodeLocation *loc) > +{ > + g_return_val_if_fail (GEOCODE_IS_LOCATION (loc), 0.0); Again, the default should be negative, and the negative value documented.
Comment on attachment 113949 [details] [review] Add property Heading to Location object Review of attachment 113949 [details] [review]: ----------------------------------------------------------------- Many of the comments on the prev. patch also apply here (e.g dividing the patch further, submitting the geocode-glib changes to geocode-glib etc). Good work with the commit log. Very few people learn to write good commit messages so fast. Some nitpick still: * In the previous patch you put this on a separate para: "This property will be helpful in implementating code for moving devices" so would be good to be consistent here. * "it's also calculating the new direction on location change." This gives the impression that direction is always set and *also* updated on new location, which isn't true. Moreover, its not the property that calculates anything. I'd just remove this sentence as the property is being added on location anyway so its kinda obvious. ::: src/gclue-locator.c @@ +77,4 @@ > guint64 cur_timestamp, timestamp; > const gdouble ALPHA = 0.12; // This value can modulate noise in speed. > gdouble cur_speed, speed; > + guint heading; Use the enum type. ::: src/geocode-glib/geocode-location.c @@ +44,4 @@ > gdouble altitude; > gdouble accuracy; > gdouble speed; > + guint heading; pleas use enum type you created for this. @@ +529,5 @@ > + * GeocodeLocation:heading: > + * > + * The direction in which the device is heading to. > + */ > + pspec = g_param_spec_uint ("heading", We want to declare this property as enum type. There is g_param_spec_enum @@ +936,5 @@ > } > + > +guint > +geocode_location_get_heading_direction (GeocodeLocation *loca, > + GeocodeLocation *locb) You'd want to add heading enum to geocode-glib project but I don't think you want the calculations for it here sine geocode-glib currently doesn't have any use for these. If speed calculation is in Locator, this also belongs there. @@ +969,5 @@ > + case -4: return GEOCODE_LOCATION_HEADING_WEST; > + case -3: return GEOCODE_LOCATION_HEADING_SOUTH_WEST; > + case -2: return GEOCODE_LOCATION_HEADING_SOUTH; > + case -1: return GEOCODE_LOCATION_HEADING_SOUTH_EAST; > + } Similar to speed calculations, I think this code can be much simpler. You can simply check for values of dx and dy. if (dx == 0) { if (dy > 0) direction = SOUTH; else if (dy < 0) direction = NORTH; else direction = UKNOWN; } else if (dx > 0) { ... } else { ... } ::: src/public-api/gclue-enums.h @@ +48,5 @@ > } GClueAccuracyLevel; > > +/** > + * GeocodeLocationHeading: > + * @GEOCODE_LOCATION_HEADING_NOHERE: If the device is heading nowhere I think we rather want 'unknown'.
(In reply to Zeeshan Ali from comment #6) > @@ +103,5 @@ > > + cur_timestamp = geocode_location_get_timestamp (cur_location); > > + timestamp = geocode_location_get_timestamp (location); > > + cur_speed = geocode_location_get_speed (cur_location); > > + /* applying low pass filter to reduce noise here > > + * Speed[i] = Speed[i-1]*APLHA + Speed[i]*(1-ALPHA) */ > > What kind of noise does this exactly reduce? I don't see why we need these > complications here, just derive speed from prev and new location. No need to > look at previous speed either as that shouldn't matter with new speed. > Say, we are driving at a continuous speed of 30km/h and due to some issue GPS predicts a faulty location for one reading, this will cause a huge change in speed for that instant. This kind of approach would smoothen that out for us. But I guess we could leave this part out because as an API our responsibility is to provide speed in its raw form.
(In reply to Bastien Nocera from comment #7) > ::: src/gclue-locator.c > @@ +103,5 @@ > > + cur_timestamp = geocode_location_get_timestamp (cur_location); > > + timestamp = geocode_location_get_timestamp (location); > > + cur_speed = geocode_location_get_speed (cur_location); > > + /* applying low pass filter to reduce noise here > > + * Speed[i] = Speed[i-1]*APLHA + Speed[i]*(1-ALPHA) */ > > I can't see how that code could work either (APLHA?). > It's nice little trick that I saw on Android developers page when I was working on motion sensors. I got fascinated by how it works, so I wasn't able to hold myself back when I thought I could use it here. http://developer.android.com/guide/topics/sensors/sensors_motion.html#sensors-motion-accel
(In reply to Zeeshan Ali from comment #8) > Good work with the commit log. Very few people learn to write good commit > messages so fast. Some nitpick still: Thanks :) > ::: src/gclue-locator.c > @@ +77,4 @@ > > guint64 cur_timestamp, timestamp; > > const gdouble ALPHA = 0.12; // This value can modulate noise in speed. > > gdouble cur_speed, speed; > > + guint heading; > > Use the enum type. I tried to use enum previously, but I ran into some complications. Anyways, I'll try this again and get back to you. > > ::: src/geocode-glib/geocode-location.c > @@ +44,4 @@ > > gdouble altitude; > > gdouble accuracy; > > gdouble speed; > > + guint heading; > > pleas use enum type you created for this. Same applies here for enum. > > @@ +936,5 @@ > > } > > + > > +guint > > +geocode_location_get_heading_direction (GeocodeLocation *loca, > > + GeocodeLocation *locb) > > You'd want to add heading enum to geocode-glib project but I don't think you > want the calculations for it here sine geocode-glib currently doesn't have > any use for these. If speed calculation is in Locator, this also belongs > there. Since speed and heading are the properties of Location object, wouldn't it make more sense to keep it here and add an additional method for speed calculations too. Additionally, it would provide a neat method inside GeocodeLocation object too. > > @@ +969,5 @@ > > + case -4: return GEOCODE_LOCATION_HEADING_WEST; > > + case -3: return GEOCODE_LOCATION_HEADING_SOUTH_WEST; > > + case -2: return GEOCODE_LOCATION_HEADING_SOUTH; > > + case -1: return GEOCODE_LOCATION_HEADING_SOUTH_EAST; > > + } > > Similar to speed calculations, I think this code can be much simpler. You > can simply check for values of dx and dy. > > if (dx == 0) { > if (dy > 0) > direction = SOUTH; > else if (dy < 0) > direction = NORTH; > else > direction = UKNOWN; > } else if (dx > 0) { > ... > } else { > ... > } I tried this approach, but I failed. It works very well if we have 4 directions; but if we consider 4 more directions, it becomes more difficult to decide which direction to go for. And also decisions like, "is the current direction of movement nearer to North or North-East?" becomes easier with the used approach. > > ::: src/public-api/gclue-enums.h > @@ +48,5 @@ > > } GClueAccuracyLevel; > > > > +/** > > + * GeocodeLocationHeading: > > + * @GEOCODE_LOCATION_HEADING_NOHERE: If the device is heading nowhere > > I think we rather want 'unknown'. IMO we should go for both: 'unknown' and 'nowhere' nowhere: when the device is standing still. unknown: when the geoclue is returning its first location and there is no way to calculate speed yet.
(In reply to Ankit from comment #11) > (In reply to Zeeshan Ali from comment #8) > > > ::: src/gclue-locator.c > > @@ +77,4 @@ > > > guint64 cur_timestamp, timestamp; > > > const gdouble ALPHA = 0.12; // This value can modulate noise in speed. > > > gdouble cur_speed, speed; > > > + guint heading; > > > > Use the enum type. > I tried to use enum previously, but I ran into some complications. > Anyways, I'll try this again and get back to you. I'd guess that issue is enum being defined in geoclue and geocode-glib code has no access to that. > > @@ +936,5 @@ > > > } > > > + > > > +guint > > > +geocode_location_get_heading_direction (GeocodeLocation *loca, > > > + GeocodeLocation *locb) > > > > You'd want to add heading enum to geocode-glib project but I don't think you > > want the calculations for it here sine geocode-glib currently doesn't have > > any use for these. If speed calculation is in Locator, this also belongs > > there. > Since speed and heading are the properties of Location object, wouldn't > it make more sense to keep it here and add an additional method for speed > calculations too. Sure but you'll have convince geocode-glib maintainers that its a good thing. Fortunately for you, Bastien is that maintainer. :) Actually a better idea would be to drop the properties on GeocodeLocation object and only provide these method. Geoclue can then use these methods and set the properties of D-Bus location object (GClueServiceLocation). > > @@ +969,5 @@ > > > + case -4: return GEOCODE_LOCATION_HEADING_WEST; > > > + case -3: return GEOCODE_LOCATION_HEADING_SOUTH_WEST; > > > + case -2: return GEOCODE_LOCATION_HEADING_SOUTH; > > > + case -1: return GEOCODE_LOCATION_HEADING_SOUTH_EAST; > > > + } > > > > Similar to speed calculations, I think this code can be much simpler. You > > can simply check for values of dx and dy. > > > > if (dx == 0) { > > if (dy > 0) > > direction = SOUTH; > > else if (dy < 0) > > direction = NORTH; > > else > > direction = UKNOWN; > > } else if (dx > 0) { > > ... > > } else { > > ... > > } > I tried this approach, but I failed. It works very well if we have 4 > directions; but if we consider 4 more directions, it becomes more difficult > to decide which > direction to go for. How so? ... else if (dx > 0) { if (dy > 0) direction = NORTH_EAST; else if (dy < 0) direction = SOUTH_EAST; } (Notice I got latitude & longitude mixed previously but the idea remains the same). Now that you mentioned complication, the only one we'd have is that of person traveling over the boundaries (e.g lat +89 to lat -89) but that can also be covered with more if/else branches that first checks for these conditions. Some comments in the code will also make it more readable. > And also decisions like, "is the current direction of movement nearer to > North or North-East?" becomes easier with the used approach. Don't think I quite follow but are you sure it can't be covered? If so, how not? > > ::: src/public-api/gclue-enums.h > > @@ +48,5 @@ > > > } GClueAccuracyLevel; > > > > > > +/** > > > + * GeocodeLocationHeading: > > > + * @GEOCODE_LOCATION_HEADING_NOHERE: If the device is heading nowhere > > > > I think we rather want 'unknown'. > IMO we should go for both: 'unknown' and 'nowhere' > nowhere: when the device is standing still. > unknown: when the geoclue is returning its first location and there is no way > to calculate speed yet. Good point I think. :)
Comment on attachment 113949 [details] [review] Add property Heading to Location object Review of attachment 113949 [details] [review]: ----------------------------------------------------------------- ::: src/geocode-glib/geocode-location.c @@ +969,5 @@ > + case -4: return GEOCODE_LOCATION_HEADING_WEST; > + case -3: return GEOCODE_LOCATION_HEADING_SOUTH_WEST; > + case -2: return GEOCODE_LOCATION_HEADING_SOUTH; > + case -1: return GEOCODE_LOCATION_HEADING_SOUTH_EAST; > + } The heading needs to be a float. Clamping what might be available from the hardware to 4 or even 8 values is not going to help applications. From: https://developer.apple.com/library/ios/documentation/CoreLocation/Reference/CLLocation_Class/index.html#//apple_ref/occ/instp/CLLocation/course " Course values are measured in degrees starting at due north and continuing clockwise around the compass. Thus, north is 0 degrees, east is 90 degrees, south is 180 degrees, and so on. Course values may not be available on all devices. A negative value indicates that the direction is invalid. " That seems like a good way to avoid problems with enums needing to be exported, and ensures precision on par with what the hardware would allow. (Note that this exports an angle compared to due north, not the magnetic north).
(In reply to Ankit from comment #10) > (In reply to Bastien Nocera from comment #7) > > > ::: src/gclue-locator.c > > @@ +103,5 @@ > > > + cur_timestamp = geocode_location_get_timestamp (cur_location); > > > + timestamp = geocode_location_get_timestamp (location); > > > + cur_speed = geocode_location_get_speed (cur_location); > > > + /* applying low pass filter to reduce noise here > > > + * Speed[i] = Speed[i-1]*APLHA + Speed[i]*(1-ALPHA) */ > > > > I can't see how that code could work either (APLHA?). > > > It's nice little trick that I saw on Android developers page when I was > working on motion sensors. I got fascinated by how it works, so I wasn't > able to hold myself back when I thought I could use it here. > http://developer.android.com/guide/topics/sensors/sensors_motion. > html#sensors-motion-accel You have a typo in that simili-code.
(In reply to Zeeshan Ali from comment #12) > Actually a better idea would be to drop the properties on GeocodeLocation > object and only provide these method. Geoclue can then use these methods and > set the properties of D-Bus location object (GClueServiceLocation). > At first I thought we could easily remove it, because my whole purpose was to store it inside GeocodeLocation object and then to use it for persistence of previous speed (because I was using that low pass filter thing). But on further observation I found out that if I remove those properties and just rely on methods then we'll have to pass these properties via functions or we could calculate it by fetching GClueServiceClient's prev_location property (but that is private). I don't know, I'm having hard time imagining "How can it be done without those properties?" :/
(In reply to Bastien Nocera from comment #13) > Comment on attachment 113949 [details] [review] [review] > Add property Heading to Location object > > Review of attachment 113949 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: src/geocode-glib/geocode-location.c > @@ +969,5 @@ > > + case -4: return GEOCODE_LOCATION_HEADING_WEST; > > + case -3: return GEOCODE_LOCATION_HEADING_SOUTH_WEST; > > + case -2: return GEOCODE_LOCATION_HEADING_SOUTH; > > + case -1: return GEOCODE_LOCATION_HEADING_SOUTH_EAST; > > + } > > The heading needs to be a float. Clamping what might be available from the > hardware to 4 or even 8 values is not going to help applications. > > From: > https://developer.apple.com/library/ios/documentation/CoreLocation/Reference/ > CLLocation_Class/index.html#//apple_ref/occ/instp/CLLocation/course > > " > Course values are measured in degrees starting at due north and continuing > clockwise around the compass. Thus, north is 0 degrees, east is 90 degrees, > south is 180 degrees, and so on. Course values may not be available on all > devices. A negative value indicates that the direction is invalid. > " > > That seems like a good way to avoid problems with enums needing to be > exported, and ensures precision on par with what the hardware would allow. > > (Note that this exports an angle compared to due north, not the magnetic > north). I think we can do this. What say Zeeshan, Bastien shall I go ahead on this? And, keep the property name to heading or change it to course?
(In reply to Bastien Nocera from comment #14) > (In reply to Ankit from comment #10) > > (In reply to Bastien Nocera from comment #7) > > > > > ::: src/gclue-locator.c > > > @@ +103,5 @@ > > > > + cur_timestamp = geocode_location_get_timestamp (cur_location); > > > > + timestamp = geocode_location_get_timestamp (location); > > > > + cur_speed = geocode_location_get_speed (cur_location); > > > > + /* applying low pass filter to reduce noise here > > > > + * Speed[i] = Speed[i-1]*APLHA + Speed[i]*(1-ALPHA) */ > > > > > > I can't see how that code could work either (APLHA?). > > > > > It's nice little trick that I saw on Android developers page when I was > > working on motion sensors. I got fascinated by how it works, so I wasn't > > able to hold myself back when I thought I could use it here. > > http://developer.android.com/guide/topics/sensors/sensors_motion. > > html#sensors-motion-accel > > You have a typo in that simili-code. You mean switching the values of Speed[i] and Speed[i-1]?
(In reply to Ankit from comment #17) > (In reply to Bastien Nocera from comment #14) > > (In reply to Ankit from comment #10) > > > (In reply to Bastien Nocera from comment #7) > > > > > > > ::: src/gclue-locator.c > > > > @@ +103,5 @@ > > > > > + cur_timestamp = geocode_location_get_timestamp (cur_location); > > > > > + timestamp = geocode_location_get_timestamp (location); > > > > > + cur_speed = geocode_location_get_speed (cur_location); > > > > > + /* applying low pass filter to reduce noise here > > > > > + * Speed[i] = Speed[i-1]*APLHA + Speed[i]*(1-ALPHA) */ > > > > > > > > I can't see how that code could work either (APLHA?). > > > > > > > It's nice little trick that I saw on Android developers page when I was > > > working on motion sensors. I got fascinated by how it works, so I wasn't > > > able to hold myself back when I thought I could use it here. > > > http://developer.android.com/guide/topics/sensors/sensors_motion. > > > html#sensors-motion-accel > > > > You have a typo in that simili-code. > > You mean switching the values of Speed[i] and Speed[i-1]? You wrote APLHA. APLHA, not ALPHA.
(In reply to Ankit from comment #15) > (In reply to Zeeshan Ali from comment #12) > > > Actually a better idea would be to drop the properties on GeocodeLocation > > object and only provide these method. Geoclue can then use these methods and > > set the properties of D-Bus location object (GClueServiceLocation). > > > At first I thought we could easily remove it, because my whole purpose was to > store it inside GeocodeLocation object and then to use it for persistence of > previous speed (because I was using that low pass filter thing). But on > further observation I found out that if I remove those properties and just > rely on methods then we'll have to pass these properties via functions or we > could calculate it by fetching GClueServiceClient's prev_location property > (but that is private). I don't understand. Locator keeps last location. Although communicating them to ServiceClient would imply addition of separate properties on LocationSource. Its just that its really weird to provide two properties and then independent methods to calculate their values (especially from purely geocode-glib POV). If we really want these properties, we probably want some setter methods instead: geocode_location_set_speed_from_prev_location (location, prev_location) geocode_location_set_direction_from_prev_location (location, prev_location) or just adding a 'previous-location' property to GeocodeLocation and if its set, we calculate/update these properties from that. I like this approach the best. > I don't know, I'm having hard time imagining "How can it be done without > those > properties?" :/ There is also the option of properties being calculated by ServiceClient or ServiceLocation.
(In reply to Ankit from comment #16) > (In reply to Bastien Nocera from comment #13) > > I think we can do this. > What say Zeeshan, Bastien shall I go ahead on this? Actually these angles are how you specify them on the radio in aviation too so I'm guessing this is the usual way so yes please. > And, keep the property name to heading or change it to course? Heading is still the valid name IMO.
(In reply to Bastien Nocera from comment #18) > You wrote APLHA. APLHA, not ALPHA. My bad.
(In reply to Zeeshan Ali from comment #19) > (In reply to Ankit from comment #15) > > (In reply to Zeeshan Ali from comment #12) > > > > > Actually a better idea would be to drop the properties on GeocodeLocation > > > object and only provide these method. Geoclue can then use these methods and > > > set the properties of D-Bus location object (GClueServiceLocation). > > > > > At first I thought we could easily remove it, because my whole purpose was to > > store it inside GeocodeLocation object and then to use it for persistence of > > previous speed (because I was using that low pass filter thing). But on > > further observation I found out that if I remove those properties and just > > rely on methods then we'll have to pass these properties via functions or we > > could calculate it by fetching GClueServiceClient's prev_location property > > (but that is private). > > I don't understand. Locator keeps last location. Although communicating them > to ServiceClient would imply addition of separate properties on > LocationSource. > > Its just that its really weird to provide two properties and then > independent methods to calculate their values (especially from purely > geocode-glib POV). If we really want these properties, we probably want some > setter methods instead: > > geocode_location_set_speed_from_prev_location (location, prev_location) > geocode_location_set_direction_from_prev_location (location, prev_location) > > or just adding a 'previous-location' property to GeocodeLocation and if its > set, we calculate/update these properties from that. I like this approach > the best. > > > I don't know, I'm having hard time imagining "How can it be done without > > those > > properties?" :/ > > There is also the option of properties being calculated by ServiceClient or > ServiceLocation. I think I need to study the code once more. I'll get back to you.
Created attachment 114326 [details] [review] Change DBus Interface namespace Changed DBus Interface namespace from GClue to GClueDBus. It had to be done in order to remove conflicts for the GClueLocation class (subclass of GeocodeLocation) which will be added afterwards in order to extend GeoClue.
Created attachment 114327 [details] [review] Change DBus Interface namespace Changed DBus Interface namespace from GClue to GClueDBus. It had to be done in order to remove conflicts for the GClueLocation class (subclass of GeocodeLocation) which will be added afterwards in order to extend GeoClue.
Created attachment 114328 [details] [review] Add GClueLocation class GClueLocation class is the subclass of GeocodeLocation. Whole code is now switched to GClueLocation instead of GeoclueLocation. It had to be done for extensibility of Location object. This commit will be followed by addition of Speed and Heading properties in our new location object. Speed and Heading properties will be required while implementing code for moving sources.
Created attachment 114329 [details] [review] Add property Speed to Location object The speed is calculated in meters per second on location change for the device. This property will be helpful in implementing code for moving devices.
Created attachment 114330 [details] [review] Add property Heading to Location object Heading property tells the direction in which device is headed to i.e. number of degrees with respect to North direction. This property will be helpful in implementing code for moving devices.
Firstly, really good work with already dividing the changes into patches in a very logical way.
Comment on attachment 114327 [details] [review] Change DBus Interface namespace Review of attachment 114327 [details] [review]: ----------------------------------------------------------------- Looks good apart from some minor things mentioned inline. nitpicks on log: * Use present continuous form of verbs: Changed -> Change. * Keep shortlog as short as possible (ideally < 50 chars): "Change DBus namespace to GClueDBus". Then you can start description with "Change DBus namespace from GClue to GClueDBus" * "It had to be done in order to remove" -> "This is needed to avoid". * "in order to extend GeoClue" doesn't tell anything so either just remove that. You can justify the need for the new class in the log of the patch that introduces it. ::: src/gclue-service-client.c @@ +290,3 @@ > data->invocation); > g_debug ("'%s' started.", > + gclue_dbus_client_get_desktop_id (GCLUE_DBUS_CLIENT (data->client))); I'm assuming these changes don't make the lines exceed the 80 chars/columns limit we follow? Please make sure of that. If a line exceeds the limit, please break it down, perhaps using variables. @@ +730,1 @@ > DEFAULT_ACCURACY_LEVEL); you need to re-align the second argument. ::: src/gclue-service-manager.c @@ +248,2 @@ > invocation, > existing_path); arguments need re-alignment. @@ +464,1 @@ > level); second arg needs re-alignment
Comment on attachment 114328 [details] [review] Add GClueLocation class Review of attachment 114328 [details] [review]: ----------------------------------------------------------------- * I'd split the existing code adaptation into a separate patch. * "Speed and Heading properties will be required while implementing code for moving sources". That is not correct. Sources are not whats moving but rather the user/device itself. I think this sentence is redundant. Apart from that and other inline comments (mostly nitpicks), looks good. ::: src/Makefile.am @@ +92,5 @@ > gclue-wifi.c \ > gclue-mozilla.h \ > gclue-mozilla.c \ > + gclue-location.h \ > + gclue-location.c \ The '\' are not aligned with others above them. It should be. ::: src/gclue-location-source.c @@ +271,4 @@ > priv->location = g_object_new (GCLUE_TYPE_LOCATION, NULL); > > g_object_set (priv->location, > + "latitude", geocode_location_get_latitude (GEOCODE_LOCATION (location)), Same comment about fitting inside 80 characters as in previous patch. ::: src/gclue-location.c @@ +1,4 @@ > +/* vim: set et ts=8 sw=8: */ > +/* gclue-location.c > + * > + * Copyright (C) 2014 Red Hat, Inc. You probably want to add your copyright here. @@ +16,5 @@ > + * You should have received a copy of the GNU General Public License along > + * with Geoclue; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > + * > + * Author: Ankit <ankitstarski@gmail.com> If you have based this on some existing code, would be nice to keep credits to original author(s). @@ +23,5 @@ > +#include "gclue-location.h" > + > +enum { > + PROP_0, > +}; No need for this enum or property getters in this patch. You want to make that part of the patch that adds the first property. ::: src/gclue-location.h @@ +1,4 @@ > +/* vim: set et ts=8 sw=8: */ > +/* gclue-location.h > + * > + * Copyright (C) 2014 Red Hat, Inc. Same comment about copyright. @@ +16,5 @@ > + * You should have received a copy of the GNU General Public License along > + * with Geoclue; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > + * > + * Author: Ankit <ankitstarski@gmail.com> Same comment about author. @@ +31,5 @@ > +#define GCLUE_TYPE_LOCATION (gclue_location_get_type ()) > +#define GCLUE_LOCATION(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), GCLUE_TYPE_LOCATION, GClueLocation)) > +#define GCLUE_IS_LOCATION(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GCLUE_TYPE_LOCATION)) > +#define GCLUE_LOCATION_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), GCLUE_TYPE_LOCATION, GClueLocationClass)) > +#define GCLUE_IS_LOCATION_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), GCLUE_TYPE_LOCATION)) Lets reduce the space between the macro signature and definition but keep them aligned. See gclue-location-source.h for example. @@ +60,5 @@ > +GClueLocation *gclue_location_new (gdouble latitude, > + gdouble longitude, > + gdouble accuracy); > + > +GClueLocation *gclue_location_new_with_description (gdouble latitude, you want to break this between lines if it doesn't fit 80 chars. gclue-location-source is good example again. ::: src/gclue-locator.c @@ +81,5 @@ > g_debug ("New location available"); > > if (cur_location != NULL && > + geocode_location_get_distance_from (GEOCODE_LOCATION (location), > + GEOCODE_LOCATION (cur_location)) < I'm guess this breaks 80 chars rule too and looks complicated. I suggest either using local variables to make it more readable. ::: src/gclue-service-client.c @@ +118,3 @@ > { > GClueServiceClientPrivate *priv = client->priv; > + GClueLocation *cur_location; if we only use it as GeocodeLocation below, better declare this of parent type and avoid the ugly cast below. ::: src/gclue-service-location.c @@ +138,5 @@ > gdouble altitude; > > loc = g_value_get_object (value); > gclue_dbus_location_set_latitude > + (location, geocode_location_get_latitude (GEOCODE_LOCATION (loc))); I think this is also going beyong 80 chars. Local variable(s) needed here too I think. ::: src/gclue-web-source.c @@ +305,4 @@ > web->priv->last_submitted + SUBMISSION_TIME_THRESHOLD) > return; > > + web->priv->last_submitted = geocode_location_get_timestamp (GEOCODE_LOCATION (location)); Same 80 cols issue here.
Comment on attachment 114329 [details] [review] Add property Speed to Location object Review of attachment 114329 [details] [review]: ----------------------------------------------------------------- * Make use of prefixes to make shortlogs short. How about "location: Add & set 'Speed' prop" ? * Lets divide the first sentence in description to reflect the two changes: Add a new property, 'Speed' that reflects the speed in meters per second. This property is re-calculated and updated on each location update." * I'd remove "for the device" in description. * The last para is a bit vague. How about "This API will be helpful for apps that are interested in user's movements"? ::: src/gclue-location.c @@ +23,5 @@ > #include "gclue-location.h" > +#include <math.h> > + > +struct _GClueLocationPrivate { > + gdouble speed; Too much space between type and name. @@ +74,4 @@ > switch (property_id) { > + case PROP_SPEED: > + gclue_location_set_speed (location, > + g_value_get_double (value)); The second arg needs to be aligned to the first. @@ +108,5 @@ > + * The speed of the device in meters per second. > + */ > + pspec = g_param_spec_double ("speed", > + "Speed", > + "The speed of the device in meters per second", "of the device" is redundant IMHO. @@ +197,5 @@ > + * @location: a #GClueLocation > + * @prev_location: a #GClueLocation > + * > + * Calculates the speed in meters per second using two GClueLocation > + * objects. This makes it sound like a generic function but its no longer that but rather a method that sets the speed on its object (the first arg) based on previous location. I'd ditch this and modify the sentence below. @@ +199,5 @@ > + * > + * Calculates the speed in meters per second using two GClueLocation > + * objects. > + * > + * Sets speed in meters per second. How about: Calculates the speed based on provided previous location @prev_location and sets it on @location. The property is already documented enough to be in meters per second so we don't need to repeat that here again IMO. @@ +210,5 @@ > + guint64 timestamp, prev_timestamp; > + > + g_return_if_fail (GCLUE_IS_LOCATION (location)); > + > + if (GCLUE_IS_LOCATION (prev_location)) { * We don't expect anything other than GClueLocation and null so you want to add: g_return_if_fail (prev_location == null || GCLUE_IS_LOCATION (prev_location)); above. * Code will look cleaner if you write this as: if (prev_location == null) { location->priv->speed = GCLUE_LOCATION_SPEED_UNKNOWN; return; } // normal speed calculation follows @@ +216,5 @@ > + prev_timestamp = geocode_location_get_timestamp (GEOCODE_LOCATION (prev_location)); > + > + speed = geocode_location_get_distance_from (GEOCODE_LOCATION (location), > + GEOCODE_LOCATION (prev_location)) * > + 1000.0 / (timestamp - prev_timestamp); Same comment about 80 chars here. ::: src/gclue-location.h @@ +43,5 @@ > { > /* Parent instance structure */ > GeocodeLocation parent_instance; > + > + /* instance members */ we don't have instance members (and its and old practice to have them ever sine the introduction of private structs to gobject). ::: src/org.freedesktop.GeoClue2.xml @@ +187,5 @@ > <!-- > + Speed: > + > + The speed of device in meters per second. When unknown, it's set to > + minimum double value, -1.7976931348623157e+308. Just any negative value or -1 would suffice, to keep things simple.
Comment on attachment 114330 [details] [review] Add property Heading to Location object Review of attachment 114330 [details] [review]: ----------------------------------------------------------------- Many of the comments on previous patch, apply here too. Other than that, I only had one comment. ::: src/gclue-location.c @@ +311,5 @@ > + > + // making North as 0 degrees now range of angle becomes (-270.90] > + angle -= 90.0; > + > + // converting nevative angles to their positive equivalents * We don't use '//' comments in C code by convention. * The comments are not really helping me understand the calculations here. :(
Comment on attachment 114329 [details] [review] Add property Speed to Location object Review of attachment 114329 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +210,5 @@ > + guint64 timestamp, prev_timestamp; > + > + g_return_if_fail (GCLUE_IS_LOCATION (location)); > + > + if (GCLUE_IS_LOCATION (prev_location)) { Shouldn't it be: g_return_if_fail (prev_location != null && GCLUE_IS_LOCATION (prev_location)); ?
Created attachment 114475 [details] [review] Change DBus namespace to GClueDBus Change DBus namespace from GClue to GClueDBus. This is needed to avoid conflicts for the GClueLocation class (subclass of GeocodeLocation) which will be added afterwards.
Created attachment 114476 [details] [review] Add GClueLocation class GClueLocation class is the subclass of GeocodeLocation. It is done for the extensibility of Location object. This commit will be followed by adaptation of GClueLocation class.
Created attachment 114477 [details] [review] Adapt GClueLocation class This commit swtiches from GeocodeLocation to GClueLocation, as default location class. This is needed in order to extend location object.
Created attachment 114478 [details] [review] location: Add & set 'Speed' prop Add a new property, 'Speed' that reflects the speed in meters per second. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements.
Created attachment 114479 [details] [review] location: Add & set 'Heading' prop Add a new property, 'Heading' that reflects the heading direction in degrees with respect to North direction. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements
Comment on attachment 114329 [details] [review] Add property Speed to Location object Review of attachment 114329 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +210,5 @@ > + guint64 timestamp, prev_timestamp; > + > + g_return_if_fail (GCLUE_IS_LOCATION (location)); > + > + if (GCLUE_IS_LOCATION (prev_location)) { No, if its null, its not of any particular type and GCLUE_IS_LOCATION (null) would therefore be false.
Comment on attachment 114475 [details] [review] Change DBus namespace to GClueDBus Review of attachment 114475 [details] [review]: ----------------------------------------------------------------- Looks good but just one minor nitpick in log: "conflict for" -> "conflict with".
Comment on attachment 114476 [details] [review] Add GClueLocation class Review of attachment 114476 [details] [review]: ----------------------------------------------------------------- Looks good otherwise. ::: src/gclue-location.c @@ +1,4 @@ > +/* vim: set et ts=8 sw=8: */ > +/* gclue-location.c > + * > + * Copyright (C) Ankit <ankitstarski@gmail.com> * How come its only your copyright but only Red Hat authors below? :) You want copyrights lines for both yourself and from the code your based this on. Same goes for Authors below, please add yourself there. * You're missing year from copyright line here. * I know the mess about your lastname not being in the documents (as you know I kinda share your pain) but I strongly recommend you write your last name (maybe in brackets at least). Just a recommendation! Feel free to ignore this. :) @@ +65,5 @@ > + GObjectClass *glocation_class = G_OBJECT_CLASS (klass); > + > + glocation_class->finalize = gclue_location_finalize; > + glocation_class->get_property = gclue_location_get_property; > + glocation_class->set_property = gclue_location_set_property; Props setter/getter should be introduced with the first property addition as pointed out in previous review: "No need for this enum or property getters in this patch. You want to make that part of the patch that adds the first property." You already removed the enum but forgot about the getter/setter.
Comment on attachment 114477 [details] [review] Adapt GClueLocation class Review of attachment 114477 [details] [review]: ----------------------------------------------------------------- Looks good apart from nitpicks about commit log: * "Adapt" isn't quite correct here. I'd go for "Switch to GClueLocation class" * "This commit" -> "This patch" or "This change". * "switches from" -> "switches the code from use of". * ", as default location class" is redundant. * We don't actually need this for extending the location class but rather to use the new API that will be added to GClueLocation.
Comment on attachment 114329 [details] [review] Add property Speed to Location object Review of attachment 114329 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +210,5 @@ > + guint64 timestamp, prev_timestamp; > + > + g_return_if_fail (GCLUE_IS_LOCATION (location)); > + > + if (GCLUE_IS_LOCATION (prev_location)) { err.. i didn't realize you changed the null check from == (as I wrote it above) to !=. You want to ensure here that either prev_location is null or GCLUE_IS_LOCATION if not null.
Comment on attachment 114478 [details] [review] location: Add & set 'Speed' prop Review of attachment 114478 [details] [review]: ----------------------------------------------------------------- Since you are adding both internal and external (D-Bus) properties in the same patch, I think we should mention that this is a D-Bus property in the log. So just "new property, 'Speed'" -> "new property, 'Speed' on D-Bus location". Also in shortlog "location" -> "dbus,location". ::: src/gclue-location.c @@ +104,5 @@ > + > + /** > + * GClueLocation:speed > + * > + * The speed of the device in meters per second. you wan to remove "of the device" in here too. @@ +178,5 @@ > +/** > + * gclue_location_get_speed: > + * @loc: a #GClueLocation > + * > + * Gets the speed of device in meters per second. here too. @@ +180,5 @@ > + * @loc: a #GClueLocation > + * > + * Gets the speed of device in meters per second. > + * > + * Returns: The speed of device. and here. @@ +208,5 @@ > + guint64 timestamp, prev_timestamp; > + GeocodeLocation *gloc, *prev_gloc; > + > + g_return_if_fail (location != NULL && > + GCLUE_IS_LOCATION (location)); As I pointed out in previous comment, this needs changing. @@ +210,5 @@ > + > + g_return_if_fail (location != NULL && > + GCLUE_IS_LOCATION (location)); > + > + if (prev_location == NULL || !GCLUE_IS_LOCATION (prev_location)) { Once g_return_if_fail check is corrected, you can remove the !GCLUE_IS_LOCATION part here since the g_return_if_fail will ensure prev_location is correct type if its not null. @@ +211,5 @@ > + g_return_if_fail (location != NULL && > + GCLUE_IS_LOCATION (location)); > + > + if (prev_location == NULL || !GCLUE_IS_LOCATION (prev_location)) { > + location->priv->speed = GCLUE_LOCATION_SPEED_UNKNOWN; I prefer empty lines above return to make it easy to spot return locations in functions. ::: src/org.freedesktop.GeoClue2.xml @@ +187,5 @@ > <!-- > + Speed: > + > + The speed of device in meters per second. When unknown, it's set to > + double value, -1.0 'to double value' is redundant, the data type is declared.
Comment on attachment 114479 [details] [review] location: Add & set 'Heading' prop Review of attachment 114479 [details] [review]: ----------------------------------------------------------------- Same comments about commit log as previous patch. ::: src/gclue-location.c @@ +139,5 @@ > + > + /** > + * GClueLocation:heading > + * > + * The direction in which device is headed to. I'd rather describe what exactly heading means here since the term heading doesn't need any explanation to most people. e.g "The heading as an angle between a line from previous and current location, and a line from previous location to north.". Assuming I got the definition right myself. :) @@ +143,5 @@ > + * The direction in which device is headed to. > + */ > + pspec = g_param_spec_double ("heading", > + "Heading", > + "The direction in which device is headed to", same here. @@ +314,5 @@ > + prev_lon = geocode_location_get_longitude(prev_gloc); > + > + dx = (lat - prev_lat); > + dy = (lon - prev_lon); > + Firstly, thanks for working on the comments here. Improves the readibility a lot already. @@ +315,5 @@ > + > + dx = (lat - prev_lat); > + dy = (lon - prev_lon); > + > + /* atan2 returns an angle in range (-PI,+PI] It sounds a bit like the function returns angles at random. I'd specify how it transforms the passed values to what it returns. @@ +317,5 @@ > + dy = (lon - prev_lon); > + > + /* atan2 returns an angle in range (-PI,+PI] > + * converting it to degrees we get (-180,180] > + * This means East=0deg; West=-180deg; North=90deg; South=-90deg I'd add a bit more spaces and be slightly more verbose: "East=0deg;" -> "East = 0 degrees;". Don't be afraid to put each of these East= on separate lines if they don't fit on the same line. @@ +319,5 @@ > + /* atan2 returns an angle in range (-PI,+PI] > + * converting it to degrees we get (-180,180] > + * This means East=0deg; West=-180deg; North=90deg; South=-90deg > + * > + * -dx will flip the angles about Y-axis as in -dx rather than dx? Id rather write "Passing atan2 a negative value of dx will flip...". @@ +324,5 @@ > + * This will make West=0deg; East=180deg; North=90deg; South=-90deg */ > + angle = atan2(dy, -dx) * 180.0 / M_PI; > + > + /* > + * Now, North is supposed to be 0deg; lets subtract 90deg from all A bunch of nitpicks here: * ';; -> '.' and capitalize 'l'. * xdeg -> x degrees * "all" -> "angle" * Its supposed to be usual English sentence, so you end it with a '.' at the end. @@ +332,5 @@ > + > + /* > + * Now, negative angles can be brought in to range [0,360) using > + * property: > + * angle = angle + n*360 ; where n is any integer * I don't really see us using this property. This property assumes overflow above 359 to round to 0 and we don't have a datatype that will ensure that for us. Also this property would mean adding any multiple of 360 would be the same as adding 0 but here were transform the values with this addition. I might be missing something important here so do let me know if that is the case. I haven't been to the school for a long time. :) * Either don't put this on separate line or add an empty line above it. This is also true for some of the other comment lines above. Its either a new paragraph or its continuation of an existing one. * spaces around *, especially if + got them. we don't want any operators to feel jealous. :) @@ +336,5 @@ > + * angle = angle + n*360 ; where n is any integer > + * > + * After this step > + * West=270deg; East=90deg; North=0deg; South=180deg */ > + angle = angle < 0 ? angle + 360.0 : angle; I'd rather: if (angle < 0) angle += 360.0; ::: src/org.freedesktop.GeoClue2.xml @@ +194,5 @@ > > <!-- > + Heading: > + > + The direction to which device is headed to in degrees with respect Lets remove mention of 'device' from here too. You seem to have done a good job of describing this property here. Lets describe it in the same way both here and and where we describe the internel GClueLocation property. Feel free to combine the example text I provided with what you have written here.. @@ +197,5 @@ > + > + The direction to which device is headed to in degrees with respect > + to North direction, in clockwise order. That means North becomes 0 > + degree, East: 90 degrees, South: 180 degrees, West: 270 degrees and > + so on. When unknown, it's set to double value, -1.0 No need to mention type here either. Its always double value and its declared below.
Comment on attachment 114479 [details] [review] location: Add & set 'Heading' prop Review of attachment 114479 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +332,5 @@ > + > + /* > + * Now, negative angles can be brought in to range [0,360) using > + * property: > + * angle = angle + n*360 ; where n is any integer We're using the special case of this property, the case where n=1 when the angle goes negative. This property tells us that in angles, 365 degrees is the same thing as 5 degrees and so is 725 degrees.
Created attachment 114492 [details] [review] Change DBus namespace to GClueDBus Change DBus namespace from GClue to GClueDBus. This is needed to avoid conflict with the GClueLocation class (subclass of GeocodeLocation) which will be added afterwards.
Created attachment 114493 [details] [review] Add GClueLocation class GClueLocation class is the subclass of GeocodeLocation. It is done for the new API, which will be added to GClueLocation. The code will be switched from GeoclueLocation to GClueLocation in the next patch.
Created attachment 114494 [details] [review] Switch to GClueLocation class This patch switches the code from use of GeocodeLocation to GClueLocation. This is needed for the new API which will be added to GClueLocation.
Created attachment 114495 [details] [review] dbus,location: Add & set 'Speed' prop Add a new property, 'Speed' on D-Bus location that reflects the speed in meters per second. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements.
Created attachment 114496 [details] [review] dbus,location: Add & set 'Heading' prop Add a new property, 'Heading' on D-Bus location that reflects the heading direction in degrees with respect to North direction. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements
Comment on attachment 114479 [details] [review] location: Add & set 'Heading' prop Review of attachment 114479 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +332,5 @@ > + > + /* > + * Now, negative angles can be brought in to range [0,360) using > + * property: > + * angle = angle + n*360 ; where n is any integer I understand that we are taking the case of n=1 but that would make it: angle = angle + 360, which is still wrong unless you take the property of angles to round back to 0 on 360. As I pointed out, we are using originary arthematics here and hence we can't simply use this property and we are not. Also as I pointed out, with this property the angle should remain the same but we are transforming it into another value. So I find it very unlikely that you are using this property. Please do point me to any links about this property (e.g wikipedia) in case you think I'm not understanding something.
Created attachment 114552 [details] [review] dbus,location: Add & set 'Heading' prop Add a new property, 'Heading' on D-Bus location that reflects the heading direction in degrees with respect to North direction. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements.
Created attachment 114554 [details] [review] dbus,location: Add & set 'Heading' prop Add a new property, 'Heading' on D-Bus location that reflects the heading direction in degrees with respect to North direction. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements.
Created attachment 114556 [details] [review] dbus,location: Add & set 'Heading' prop Add a new property, 'Heading' on D-Bus location that reflects the heading direction in degrees with respect to North direction. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements. https://bugs.freedesktop.org/show_bug.cgi?id=89395
Comment on attachment 114493 [details] [review] Add GClueLocation class Review of attachment 114493 [details] [review]: ----------------------------------------------------------------- Still needs some work. ::: src/gclue-location.c @@ +1,4 @@ > +/* vim: set et ts=8 sw=8: */ > +/* gclue-location.c > + * > + * Copyright (C) 2015 Red Hat, Inc. I think the file you based this on is copyright 2012 Bastien Nocera. @@ +1,5 @@ > +/* vim: set et ts=8 sw=8: */ > +/* gclue-location.c > + * > + * Copyright (C) 2015 Red Hat, Inc. > + * Copyright (C) 2015 Ankit I see you ignored my suggestion for specifying your full name. Fine by me if you want to give your copyrights to every Ankit out there. :) @@ +30,5 @@ > +static void > +gclue_location_get_property (GObject *object, > + guint property_id, > + GValue *value, > + GParamSpec *pspec) You didn't remove the getter/setter themselves, just their usage. If you had compiled this, you'd have gotten compiler warnings on this. :) ::: src/gclue-location.h @@ +1,4 @@ > +/* vim: set et ts=8 sw=8: */ > +/* gclue-location.h > + * > + * Copyright (C) 2015 Red Hat, Inc. Same notes about copyrights here. @@ +52,5 @@ > + /* Parent class structure */ > + GeocodeLocationClass parent_class; > +}; > + > +/* used by GCLUE_TYPE_LOCATION */ This comment is over-documentation, please Remove. @@ +57,5 @@ > +GType gclue_location_get_type (void); > + > +/* > + * Method definitions. > + */ They are method declarations but i think this comment comes in over-documentation.
Comment on attachment 114495 [details] [review] dbus,location: Add & set 'Speed' prop Review of attachment 114495 [details] [review]: ----------------------------------------------------------------- Looks good otherwise. ::: src/gclue-location.c @@ +62,5 @@ > +gclue_location_set_speed (GClueLocation *loc, > + gdouble speed) > +{ > + loc->priv->speed = speed; > +} Why keep the setter private? You set the speed from outside this class later. ::: src/gclue-service-location.c @@ +98,5 @@ > gclue_dbus_location_get_accuracy (location), > gclue_dbus_location_get_description (location)); > + g_object_set (loc, > + "speed", > + gclue_dbus_location_get_speed (location), NULL); The setter methods a preferred over g_object_set for performance reasons. Please make the setter public method and use that here.
Comment on attachment 114556 [details] [review] dbus,location: Add & set 'Heading' prop Review of attachment 114556 [details] [review]: ----------------------------------------------------------------- Almost there! ::: src/gclue-location.c @@ +74,5 @@ > } > > static void > +gclue_location_set_heading (GClueLocation *loc, > + gdouble heading) This should also be public like the speed setter. @@ +142,5 @@ > + /** > + * GClueLocation:heading > + * > + * The positive angle between the direction of movement and the North > + * direction, in clockwise direction. The angle is measured in degrees. Nice description! @@ +241,5 @@ > + * Gets the direction of movement in degrees due North direction in clockwise > + * direction. > + * > + * Returns: The positive angle between the direction of movement and the North > + * direction, in clockwise direction. The angle is measured in degrees. These two will be in the same place in the documentation so no need to repeat the whole thing. I'd move 'he angle is measured in degrees.' to method description above and only write "The heading, or %GCLUE_LOCATION_HEADING_UNKNOWN if heading is unknown. @@ +343,5 @@ > + * from angle. After this step West = -90 degrees, East = 90 degrees, > + * North = 0 degree, South = -180 degrees. */ > + angle -= 90.0; > + > + /* As we know, angle â¡ angle + 360; using this on negative values would seems my browser is also unable to handle this unicode character. Lets use '~=' instead maybe? @@ +347,5 @@ > + /* As we know, angle â¡ angle + 360; using this on negative values would > + * bring the the angle in range [0,360). > + * > + * After this step West = 270 degrees, East = 90 degrees, > + * North = 0 degree, South = 180 degrees. */ Really good work with comments. The whole code is now so easy to follow.
Comment on attachment 114495 [details] [review] dbus,location: Add & set 'Speed' prop Review of attachment 114495 [details] [review]: ----------------------------------------------------------------- Looks good otherwise. ::: src/gclue-location.c @@ +62,5 @@ > +gclue_location_set_speed (GClueLocation *loc, > + gdouble speed) > +{ > + loc->priv->speed = speed; > +} Why keep the setter private? You set the speed from outside this class later. @@ +182,5 @@ > + * @loc: a #GClueLocation > + * > + * Gets the speed in meters per second. > + * > + * Returns: The speed. "." -> ", or %GCLUE_LOCATION_SPEED_UNKNOWN if speed in unknown. ::: src/gclue-service-location.c @@ +98,5 @@ > gclue_dbus_location_get_accuracy (location), > gclue_dbus_location_get_description (location)); > + g_object_set (loc, > + "speed", > + gclue_dbus_location_get_speed (location), NULL); The setter methods a preferred over g_object_set for performance reasons. Please make the setter public method and use that here.
Created attachment 114586 [details] [review] Add GClueLocation class GClueLocation class is the subclass of GeocodeLocation. It is done for the new API, which will be added to GClueLocation. The code will be switched from GeoclueLocation to GClueLocation in the next patch.
Created attachment 114587 [details] [review] Switch to GClueLocation class This patch switches the code from use of GeocodeLocation to GClueLocation. This is needed for the new API which will be added to GClueLocation.
Created attachment 114588 [details] [review] dbus,location: Add & set 'Speed' prop Add a new property, 'Speed' on D-Bus location that reflects the speed in meters per second. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements.
Created attachment 114589 [details] [review] dbus,location: Add & set 'Heading' prop Add a new property, 'Heading' on D-Bus location that reflects the heading direction in degrees with respect to North direction. This property is re-calculated and updated on each location update. This API will be helpful for apps that are interested in user's movements.
Comment on attachment 114586 [details] [review] Add GClueLocation class Review of attachment 114586 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 114492 [details] [review] Change DBus namespace to GClueDBus Review of attachment 114492 [details] [review]: ----------------------------------------------------------------- ack
Comment on attachment 114587 [details] [review] Switch to GClueLocation class Review of attachment 114587 [details] [review]: ----------------------------------------------------------------- ack
Comment on attachment 114588 [details] [review] dbus,location: Add & set 'Speed' prop Review of attachment 114588 [details] [review]: ----------------------------------------------------------------- looks good otherwise. ::: src/gclue-location.c @@ +69,5 @@ > +gclue_location_set_speed (GClueLocation *loc, > + gdouble speed) > +{ > + loc->priv->speed = speed; > +} now that is public, it needs to move below along with other public methods but if I don't find many other issues in the following patches, i can fix it before merging along with other minor issues.. ::: src/org.freedesktop.GeoClue2.xml @@ +186,5 @@ > > <!-- > + Speed: > + > + The speed in meters per second. When unknown, it's set to -1.0 '.' missing at end of statement.
Comment on attachment 114589 [details] [review] dbus,location: Add & set 'Heading' prop Review of attachment 114589 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +88,5 @@ > + * Sets the heading. > + **/ > +void > +gclue_location_set_heading (GClueLocation *loc, > + gdouble heading) just like speed setter, this also needs to go below. @@ +361,5 @@ > + /* As we know, angle ~= angle + 360; using this on negative values would > + * bring the the angle in range [0,360). > + * > + * After this step West = 270 degrees, East = 90 degrees, > + * North = 0 degree, South = 180 degrees. */ it would be even easier to follow if you keep the order of directions the same in all the comments. ::: src/org.freedesktop.GeoClue2.xml @@ +196,5 @@ > + > + The heading direction in degrees with respect to North direction, in > + clockwise order. That means North becomes 0 degree, East: 90 degrees, > + South: 180 degrees, West: 270 degrees and so on. When unknown, > + it's set to -1.0 '.' missing at the end of sentence again.
Comment on attachment 114588 [details] [review] dbus,location: Add & set 'Speed' prop Review of attachment 114588 [details] [review]: ----------------------------------------------------------------- looks good otherwise. ::: src/gclue-location.c @@ +68,5 @@ > +void > +gclue_location_set_speed (GClueLocation *loc, > + gdouble speed) > +{ > + loc->priv->speed = speed; oh and this is missing a g_object_notify call. @@ +69,5 @@ > +gclue_location_set_speed (GClueLocation *loc, > + gdouble speed) > +{ > + loc->priv->speed = speed; > +} now that is public, it needs to move below along with other public methods but if I don't find many other issues in the following patches, i can fix it before merging along with other minor issues.. @@ +235,5 @@ > + > + speed = geocode_location_get_distance_from (gloc, prev_gloc) * > + 1000.0 / (timestamp - prev_timestamp); > + > + location->priv->speed = speed; This too. ::: src/org.freedesktop.GeoClue2.xml @@ +186,5 @@ > > <!-- > + Speed: > + > + The speed in meters per second. When unknown, it's set to -1.0 '.' missing at end of statement.
Pushed all patches with changes pointed out during review. Please do diff git master against your local branch to see what I changed exactly for future ref. Attachment 114492 [details] pushed as 4be447f - Change DBus namespace to GClueDBus Attachment 114586 [details] pushed as 3a03cd3 - Add GClueLocation class Attachment 114587 [details] pushed as fa72e03 - Switch to GClueLocation class Attachment 114588 [details] pushed as 1753324 - dbus,location: Add & set 'Speed' prop Attachment 114589 [details] pushed as 6808be6 - dbus,location: Add & set 'Heading' prop Would be lovely to also modify demo/where-am-i to display these new props.
Since, where-am-i only asks for one location, first location always returns GCLUE_LOCATION_SPEED_UNKNOWN and GCLUE_LOCATION_HEADING_UNKNOWN for speed and heading respectively. So it makes no sense to include these in where-am-i without having continuous location polling in it.
(In reply to Ankit from comment #71) > Since, where-am-i only asks for one location, Nope, it waits for location updates so if you move, the location changes. Also if you have multiple sources. > first location always returns > GCLUE_LOCATION_SPEED_UNKNOWN and GCLUE_LOCATION_HEADING_UNKNOWN for speed > and heading respectively. Yes, in those cases it can say: speed: Unknown heading: Unknown > So it makes no sense to include these in > where-am-i without having continuous location polling in it. In glib/gtk+ world, apps typically don't poll but use main loop to wait for events to occur and be notified through signals and callbacks. where-am-i does that too. It even has a parameter you can use to modify the time it will wait for.
Created attachment 114805 [details] [review] demo: Add 'Speed' and 'Heading' Addition of 'Speed' and 'Heading' properties to 'where-am-i.c'. This is needed after the addition of 'Speed' and 'Heading' properties to GClueLocation and GClueDBusLocation.
Comment on attachment 114805 [details] [review] demo: Add 'Speed' and 'Heading' Review of attachment 114805 [details] [review]: ----------------------------------------------------------------- Patch is good, nitpicks on log: * Shortlog can still have " to where-am-i" appended to it while remaining under 50 chars. * "Addition of" -> "Add". Remember, we use imperative form of verbs. * "'where-am-i.c'" -> where-am-i. i-e name of the app rather than its C file. * This is not exactly needed but rather something that would be good to have. * Demo apps are independent of geoclue service and hence geoclue internal API shouldn't be mentioned here. So I'd replace "to GClueLocation.." with "to Location interface" or "to org.freedesktop.GeoClue2.Location interface".
Created attachment 114930 [details] [review] demo: Add 'Speed' and 'Heading' to where-am-i Addition of 'Speed' and 'Heading' properties to where-am-i. This provides help on usage of 'Speed' and 'Heading' properties after their addition to Location interface.
Created attachment 114931 [details] [review] demo: Add 'Speed' and 'Heading' to where-am-i This provides help on usage of 'Speed' and 'Heading' properties after their addition to Location interface.
Comment on attachment 114931 [details] [review] demo: Add 'Speed' and 'Heading' to where-am-i Review of attachment 114931 [details] [review]: ----------------------------------------------------------------- Well it doesn't provide any help on properties either but rather demo app now displays those properties. I'll fix the log when pushing.. thanks.
Created attachment 114960 [details] [review] demo,where-am-i: Print speed & heading Now that speed and heading are reported on Location interface, let's print them.
Attachment 114960 [details] pushed as 6305278 - demo,where-am-i: Print speed & heading
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.