Re: [Sugar-devel] [FEATURE] Remove Presence Service --- cleanup for release
On Mon, Sep 13, 2010 at 15:02, Simon Schampijer si...@schampijer.de wrote: On 08/30/2010 08:19 AM, Simon Schampijer wrote: Hi Tomeu, your Feature [1] has found it's way into the 0.90 release, thank you for getting this far! This is just great! The following items need to be done until the release at the end of the month (ordered by priority): * Instructions for testing (URGENT): As this Feature touches one of the key features of Sugar (Collaboration) we should make sure to not introduce any regressions. I know that testing sharing has many cases but we should at least get a list what areas need testing and can refine then from there. * Bug fixing: As you stated in your proposal you will be around for fixing bugs introduced by your code. This is great. * Testing: As we do not have a very wide community of testers (yet) I would like to ask you to help in the testing efforts. * Release Notes: If you could add a few lines in the Feature page how you would best describe your Feature to a possibly non technical audience would be great. In behalf of the sugar community, Your Release Team [1] http://wiki.sugarlabs.org/go/Features/Remove_Presence_Service Ping, can we get the items listed above done? Have updated the testing and release notes sections. Thanks for reminding. Regards, Tomeu ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [FEATURE] Remove Presence Service --- cleanup for release
On 08/30/2010 08:19 AM, Simon Schampijer wrote: Hi Tomeu, your Feature [1] has found it's way into the 0.90 release, thank you for getting this far! This is just great! The following items need to be done until the release at the end of the month (ordered by priority): * Instructions for testing (URGENT): As this Feature touches one of the key features of Sugar (Collaboration) we should make sure to not introduce any regressions. I know that testing sharing has many cases but we should at least get a list what areas need testing and can refine then from there. * Bug fixing: As you stated in your proposal you will be around for fixing bugs introduced by your code. This is great. * Testing: As we do not have a very wide community of testers (yet) I would like to ask you to help in the testing efforts. * Release Notes: If you could add a few lines in the Feature page how you would best describe your Feature to a possibly non technical audience would be great. In behalf of the sugar community, Your Release Team [1] http://wiki.sugarlabs.org/go/Features/Remove_Presence_Service Ping, can we get the items listed above done? Thanks, Simon ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
[Sugar-devel] [FEATURE] Remove Presence Service --- cleanup for release
Hi Tomeu, your Feature [1] has found it's way into the 0.90 release, thank you for getting this far! This is just great! The following items need to be done until the release at the end of the month (ordered by priority): * Instructions for testing (URGENT): As this Feature touches one of the key features of Sugar (Collaboration) we should make sure to not introduce any regressions. I know that testing sharing has many cases but we should at least get a list what areas need testing and can refine then from there. * Bug fixing: As you stated in your proposal you will be around for fixing bugs introduced by your code. This is great. * Testing: As we do not have a very wide community of testers (yet) I would like to ask you to help in the testing efforts. * Release Notes: If you could add a few lines in the Feature page how you would best describe your Feature to a possibly non technical audience would be great. In behalf of the sugar community, Your Release Team [1] http://wiki.sugarlabs.org/go/Features/Remove_Presence_Service ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [FEATURE] Remove Presence Service
On Mon, Aug 16, 2010 at 23:33, Marco Pesenti Gritti ma...@marcopg.org wrote:
I don't have enough time for a very detailed review and I don't really
know telepathy, but here a few comments. Mostly nitpicks, it looks
great in general.
+ BuddyIcon.__init__(self, buddy=get_owner_instance(), size=size)
Maybe assign to self._buddy here and reuse later to pass to the menu?
Done.
+ if not self._buddy.props.present or \
+ not self._buddy.props.current_activity:
I would align the not :)
Done.
+ name = self._get_new_icon_name(self._buddy.props.current_activity)
Do we still need to use .props? I thought at some point gobject add
some magic to be able to just use properties.
Done, though I'm a bit scared of name clashes.
+ p_text = glib.markup_escape_text(self._model.bundle.get_name())
+ p_icon = Icon(file=self._model.bundle.get_icon(),
Not your fault but I hate abbreviating like this... It's not
immediately clear what the variable refers to.
I really hate it as well, but I have refrained myself to fixing
several of these things because then the patch would have doubled in
size (at least!). I think we should have a pylint rule that warns
about all identifiers with less than 4 chars.
+ item.show()
+ self._invite_to_item[invite] = item
I'd /n here to make the two blocks separate.
Done
+ def set_present(self, present):
+ self._present = present
+
+ present = gobject.property(type=bool, default=False, getter=is_present,
+ setter=set_present)
I still think we should move away from GObject for non UI stuff :)
Agreed, but as above I think it should be a cleanup goal rather than
having feature patches fixing it bit by bit.
+ if service.startswith('org.freedesktop.Telepathy.Connection.'):
+ path = '/%s' % service.replace('.', '/')
+ Connection(service, path, bus,
+ ready_handler=self.__connection_ready_cb)
I don't know enough about telepathy, but the path guessing here looks weird.
It's weird, but actually correct :) The connection service name is
part of the API. see
http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Connection.html#description
+ logging.debug('__got_dispatch_operation_cb')
+ dispatch_operation_path = kwargs['dispatch_operation_path']
Nitpicking again... In several places, I think it would be clearer to
separate the logging in its own block.
Yeah, logging is a big issue with this patch because it's really
verbose but also very useful when debugging because of the difficulty
of reproducing all situations as the code is asynchronous. I have
agreed with Simon in removing most of it once things stabilize.
+ if connection_path == '/':
+ return
Why are we ignoring this? Unless it's obvious to someone that
understands telepathy, a comment would be useful here.
It's a DBus convention for object paths, equivalent to None. Will add a comment.
+ #self._start_listening()
Leftover?
Yup, we don't need to start listening again. Removed.
+ if handle.invited:
+ wait_loop = gobject.MainLoop()
+ self._client_handler = _ClientHandler(
+ self.get_bundle_id(),
+ partial(self.__got_channel_cb, wait_loop))
+ # The current API requires that self.shared_activity is set
before
+ # exiting from __init__, so we wait until we have got the shared
+ # activity.
+ wait_loop.run()
Ouch, quite an hack :) I'd open a bug and reference it here, it should
go away at some point.
Done http://bugs.sugarlabs.org/ticket/2168
+ # Cannot call datastore.write async for creates:
+ # https://dev.laptop.org/ticket/3071
+ datastore.write(self._jobject)
Update the bug reference to sugarlabs.org while you are changing this?
Done
+ if handle.object_id is None and create_jobject:
+ logging.debug('Creating a jobject.')
+ self._jobject = datastore.create()
+ title = _('%s Activity') % get_bundle_name()
+ self._jobject.metadata['title'] = title
+ self.set_title(self._jobject.metadata['title'])
+ self._jobject.metadata['title_set_by_user'] = '0'
+ self._jobject.metadata['activity'] = self.get_bundle_id()
+ self._jobject.metadata['activity_id'] = self.get_id()
+ self._jobject.metadata['keep'] = '0'
+ self._jobject.metadata['preview'] = ''
+ self._jobject.metadata['share-scope'] = SCOPE_PRIVATE
+ if self.shared_activity is not None:
+ icon_color = self.shared_activity.props.color
+ else:
+ client = gconf.client_get_default()
+ icon_color = client.get_string('/desktop/sugar/user/color')
+
Re: [Sugar-devel] [FEATURE] Remove Presence Service
On Tue, Aug 17, 2010 at 11:01 AM, Tomeu Vizoso tomeu.viz...@collabora.co.uk wrote: I still think we should move away from GObject for non UI stuff :) Agreed, but as above I think it should be a cleanup goal rather than having feature patches fixing it bit by bit. Sure, I agree. Marco ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [FEATURE] Remove Presence Service
I don't have enough time for a very detailed review and I don't really
know telepathy, but here a few comments. Mostly nitpicks, it looks
great in general.
+BuddyIcon.__init__(self, buddy=get_owner_instance(), size=size)
Maybe assign to self._buddy here and reuse later to pass to the menu?
+if not self._buddy.props.present or \
+not self._buddy.props.current_activity:
I would align the not :)
+name = self._get_new_icon_name(self._buddy.props.current_activity)
Do we still need to use .props? I thought at some point gobject add
some magic to be able to just use properties.
+p_text = glib.markup_escape_text(self._model.bundle.get_name())
+p_icon = Icon(file=self._model.bundle.get_icon(),
Not your fault but I hate abbreviating like this... It's not
immediately clear what the variable refers to.
+item.show()
+self._invite_to_item[invite] = item
I'd /n here to make the two blocks separate.
+def set_present(self, present):
+self._present = present
+
+present = gobject.property(type=bool, default=False, getter=is_present,
+ setter=set_present)
I still think we should move away from GObject for non UI stuff :)
+if service.startswith('org.freedesktop.Telepathy.Connection.'):
+path = '/%s' % service.replace('.', '/')
+Connection(service, path, bus,
+ ready_handler=self.__connection_ready_cb)
I don't know enough about telepathy, but the path guessing here looks weird.
+logging.debug('__got_dispatch_operation_cb')
+dispatch_operation_path = kwargs['dispatch_operation_path']
Nitpicking again... In several places, I think it would be clearer to
separate the logging in its own block.
+if connection_path == '/':
+return
Why are we ignoring this? Unless it's obvious to someone that
understands telepathy, a comment would be useful here.
+#self._start_listening()
Leftover?
+if handle.invited:
+wait_loop = gobject.MainLoop()
+self._client_handler = _ClientHandler(
+self.get_bundle_id(),
+partial(self.__got_channel_cb, wait_loop))
+# The current API requires that self.shared_activity is set before
+# exiting from __init__, so we wait until we have got the shared
+# activity.
+wait_loop.run()
Ouch, quite an hack :) I'd open a bug and reference it here, it should
go away at some point.
+# Cannot call datastore.write async for creates:
+# https://dev.laptop.org/ticket/3071
+datastore.write(self._jobject)
Update the bug reference to sugarlabs.org while you are changing this?
+if handle.object_id is None and create_jobject:
+logging.debug('Creating a jobject.')
+self._jobject = datastore.create()
+title = _('%s Activity') % get_bundle_name()
+self._jobject.metadata['title'] = title
+self.set_title(self._jobject.metadata['title'])
+self._jobject.metadata['title_set_by_user'] = '0'
+self._jobject.metadata['activity'] = self.get_bundle_id()
+self._jobject.metadata['activity_id'] = self.get_id()
+self._jobject.metadata['keep'] = '0'
+self._jobject.metadata['preview'] = ''
+self._jobject.metadata['share-scope'] = SCOPE_PRIVATE
+if self.shared_activity is not None:
+icon_color = self.shared_activity.props.color
+else:
+client = gconf.client_get_default()
+icon_color = client.get_string('/desktop/sugar/user/color')
+self._jobject.metadata['icon-color'] = icon_color
Separate blocks while you are at it :) It's really hard to read.
+++ b/src/sugar/presence/util.py
Maybe a more specific name for this module? connection or something...
___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [FEATURE] Remove Presence Service
On Wed, Aug 11, 2010 at 17:26, Tomeu Vizoso tomeu.viz...@collabora.co.uk wrote: Hi, have just entered a new feature page for the removal of the Presence Service, which is being proposed as a feature for 0.90. In the page you have links to previous discussions and to the bigger plans. http://wiki.sugarlabs.org/go/Features/Remove_Presence_Service Now it is feature complete and ready for review, have rebased the clones: http://git.sugarlabs.org/projects/sugar/repos/nops http://git.sugarlabs.org/projects/sugar-toolkit/repos/nops http://git.sugarlabs.org/projects/sugar-presence-service/repos/nops Any comments are welcome. Thanks, Tomeu Regards, Tomeu ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
Re: [Sugar-devel] [FEATURE] Remove Presence Service
On 08/11/2010 05:26 PM, Tomeu Vizoso wrote: Hi, have just entered a new feature page for the removal of the Presence Service, which is being proposed as a feature for 0.90. In the page you have links to previous discussions and to the bigger plans. http://wiki.sugarlabs.org/go/Features/Remove_Presence_Service Regards, Tomeu Thanks Tomeu for providing us with this great Feature! From a maintaining point of view and in order to make Sugar even more stable this is a great enhancement. As well the possibilities for future enhancements are enormous. As this Feature is very invasive (touches three modules) and moves as well a lot of code we have to be very careful when reviewing (mind that the Feature has gone through testing and landed by next Monday the 16th of August) and of course when testing. After landing the Feature we need a lot of testing. We should list all the possible test cases in the testing section of the Feature page then. From me a definite +1 for accepting this Feature, to me all the formal criteria for inclusion are fulfilled. Regards, Simon ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
[Sugar-devel] [FEATURE] Remove Presence Service
Hi, have just entered a new feature page for the removal of the Presence Service, which is being proposed as a feature for 0.90. In the page you have links to previous discussions and to the bigger plans. http://wiki.sugarlabs.org/go/Features/Remove_Presence_Service Regards, Tomeu ___ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
