After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 722375 - Set 'DesktopId' property on Geoclue client proxy
Set 'DesktopId' property on Geoclue client proxy
Status: RESOLVED FIXED
Product: gnome-clocks
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Clocks maintainer(s)
Clocks maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-01-16 20:17 UTC by Zeeshan Ali
Modified: 2014-01-29 12:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
geocoding: Provide desktop id to geoclue (1.16 KB, patch)
2014-01-20 18:09 UTC, Zeeshan Ali
none Details | Review
geocoding: Specify needed accuracy level to geoclue (1.48 KB, patch)
2014-01-20 18:10 UTC, Zeeshan Ali
committed Details | Review
geocoding: Provide desktop id to geoclue (1.49 KB, patch)
2014-01-20 22:03 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2014-01-16 20:17:20 UTC
For security reasons, Apps must provide id of their desktop id (name of desktop
file minus '.desktop' extension).
Comment 2 Giovanni Campagna 2014-01-17 16:38:35 UTC
gnome-weather does not use Geoclue anywhere right now. I won't forget to set the DesktopId when I start using it, but for now, I guess I can close this.
Comment 3 Zeeshan Ali 2014-01-17 17:55:47 UTC
(In reply to comment #2)
> gnome-weather does not use Geoclue anywhere right now. I won't forget to set
> the DesktopId when I start using it, but for now, I guess I can close this.

Sorry, this was supposed to be for clocks.
Comment 4 Paolo Borelli 2014-01-17 20:23:33 UTC
feel free to push a patch :-)
Comment 5 Zeeshan Ali 2014-01-20 18:09:59 UTC
Created attachment 266769 [details] [review]
geocoding: Provide desktop id to geoclue
Comment 6 Zeeshan Ali 2014-01-20 18:10:03 UTC
Created attachment 266770 [details] [review]
geocoding: Specify needed accuracy level to geoclue
Comment 7 Paolo Borelli 2014-01-20 21:13:30 UTC
Review of attachment 266769 [details] [review]:

any reason to make this a public property?

I would just define a static private constant DESKTOP_ID and set the client to that
Comment 8 Zeeshan Ali 2014-01-20 22:00:34 UTC
(In reply to comment #7)
> Review of attachment 266769 [details] [review]:
> 
> any reason to make this a public property?

Sorry? Dbus props are always public and each geoclue client gets a private client object anyways so no other app can access it unless it pretends to be clocks and if it can do that, desktop-id is hardly anything thats hidden from it. :)

> I would just define a static private constant DESKTOP_ID and set the client to
> that

Ah ok, will change it then.
Comment 9 Zeeshan Ali 2014-01-20 22:03:58 UTC
Created attachment 266813 [details] [review]
geocoding: Provide desktop id to geoclue

v2: Use a const field for desktop id.
Comment 10 Paolo Borelli 2014-01-21 07:10:34 UTC
(In reply to comment #8)
> Sorry? Dbus props are always public and each geoclue client gets a private
> client object anyways so no other app can access it unless it pretends to be
> clocks and if it can do that, desktop-id is hardly anything thats hidden from
> it. :)
> 


Sorry, my fault, I was reviewing the patch from the phone so I didn't see it properly (or with the needed attention :)

My point was that I did not want to be able to set those properties from outside since for clocks they are always the same fixed value, but I now see that there are two classes, one implementing the interface and exposing the properties and one using the interface and setting them.

It is a bit of a shame that the accuracy level enum must be redefined in each app instead of being in a header of geoclue, but we've been over that before ;)


Feel free to push both, and thank you for looking into it!
Comment 11 Paolo Borelli 2014-01-21 07:10:55 UTC
Review of attachment 266813 [details] [review]:

looks good
Comment 12 Paolo Borelli 2014-01-21 07:11:48 UTC
Review of attachment 266770 [details] [review]:

looks good (will probably need a small fixup since the other patch changed, but feel free to do it and push directly)
Comment 13 Zeeshan Ali 2014-01-21 12:37:57 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > Sorry? Dbus props are always public and each geoclue client gets a private
> > client object anyways so no other app can access it unless it pretends to be
> > clocks and if it can do that, desktop-id is hardly anything thats hidden from
> > it. :)
> > 
> 
> 
> Sorry, my fault, I was reviewing the patch from the phone so I didn't see it
> properly (or with the needed attention :)
> 
> My point was that I did not want to be able to set those properties from
> outside since for clocks they are always the same fixed value, but I now see
> that there are two classes, one implementing the interface and exposing the
> properties and one using the interface and setting them.
> 
> It is a bit of a shame that the accuracy level enum must be redefined in each
> app instead of being in a header of geoclue, but we've been over that before ;)

Yeah and in the end none of the people pushing for geoclue providing a library provided patches. :(

> 
> Feel free to push both, and thank you for looking into it!

No problems, this was easy one.
Comment 14 Zeeshan Ali 2014-01-21 12:43:09 UTC
Attachment 266770 [details] pushed as 952d671 - geocoding: Specify needed accuracy level to geoclue
Attachment 266813 [details] pushed as 8e7ef76 - geocoding: Provide desktop id to geoclue
Comment 15 Evgeny Bobkin 2014-01-29 12:07:12 UTC
(In reply to comment #14)
> Attachment 266770 [details] pushed as 952d671 - geocoding: Specify needed accuracy level
> to geoclue
> Attachment 266813 [details] pushed as 8e7ef76 - geocoding: Provide desktop id to geoclue

Thank you for the patches!