Add get_grid method. #2

Merged
merged 3 commits into from May 23, 2012

Conversation

Projects
None yet
2 participants
Owner

charrea6 commented May 16, 2012

get_grid returns programs split into channels, with additional metadata added to the programs by registered callbacks.

The intention is that this is used to retrieve the grid for display with the programs also including information on whether they have been scheduled for recording/are a favorite.

@charrea6 charrea6 Add get_grid function to guide.
get_grid returns programs split into channels, with additional metadata added to the programs by registered callbacks.

The intention is that this is used to retrieve the grid for display with the programs also including information on whether they have been scheduled for recording/are a favorite.
f0b9234

@jtackaberry jtackaberry and 1 other commented on an outdated diff May 16, 2012

+import kaa
+import kaa.epg
+
+
+
+def local():
+ def callback(id, meta):
+ meta.test = 'db_id : %d' % id
+ guide = kaa.epg.load('test.db')
+ guide.register_grid_callback(callback)
+
+
+@kaa.coroutine()
+def remote():
+ g = kaa.epg.connect(('localhost', 10000))
+ yield g.signals.subset('connected').any()
@jtackaberry

jtackaberry May 16, 2012

Owner

This is more direct:

yield kaa.inprogress(g.signals['connected'])
@charrea6

charrea6 May 16, 2012

Owner

Nice I tried a couple of other attempts but missed this. I'll update the code,

Owner

jtackaberry commented May 16, 2012

I'm struggling to see the value of the grid callbacks. It sounds like it's just a way for the caller to avoid iterating over the returned lists to do custom stuff. Can you help me understand the use-case?

Why use Meta instead of a simple dict? In addition to being simpler (IMO) it's also cheaper to create {} than object()

Moving the convert() function out of search() is sensible but should be a separate commit, since it's unrelated to this pull request. Also it should be done for guide.py. I think there is an opportunity to use kaa.dateutils.to_timestamp() which was recently added to kaa.base.

Lastly, we have a lot of ugly code duplication between guide.py and rpc.py. I appreciate you're just following the poor precedent set by search() but adding another function with the a lot of duplicated code just underlines the fact that this needs to be refactored.

It'd be great if that could be done before adding this get_grid() feature, but if you prefer to do it some other time, that's ok, but in that case I'd not bother duplicating the docstring in guide.py and instead put a FIXME.

Thanks!

Owner

charrea6 commented May 16, 2012

The point of the callbacks is to allow an EPG client to retrieve the grid data from a server over RPC, where the server also contains info on the favorite/schedule status. This reduces the number of RPC calls that have to be made as you don't have to do a separate call to get info on the schedule or favorites, ie one point of information, the record server.

As for the Meta rather than the dict, well personal choice I suppose and I was considering what was already in Freevo 1, ie its an attribute access to retrieve the info. I prefer the object attribute approach, 1 it's less typing to access a value and 2 it just feels more pythonic. YMMV

There was a reason for moving the convert() in this commit it was going to be duplicated code, I'd missed that it was also in guide.py. I'll take a look at the to_timestamp and see if it does the same thing....

I'll take another look at rpc.py and see what can be refactored....

Owner

jtackaberry commented May 16, 2012

Ah right, makes sense, thanks. I didn't pay enough attention to your example where it makes it clear the callback registration is intended to be done server side.

I would prefer we use a dict here instead of a Meta instance. And let's call it userdata, as it's not really metadata (i.e. it's not data about data, it's just additional data related to the Program). This feels more conventional to me.

I'd then formalize the attribute by initializing it to None in the Program class.

@jtackaberry jtackaberry commented on an outdated diff May 16, 2012

+ matching the search criteria.
+
+ The return :class:`~kaa.epg.Program` objects will have an additional
+ 'meta' attribute that will include any additional information supplied
+ by the registered grid callback functions.
+ """
+ programs = yield self.search(channel=channels, time=(start_time,end_time), cls=None)
+ results = []
+ for c in channels:
+ results.append([])
+ channel = None
+ i = 0
+ for row in programs:
+ # Populate the meta object
+ meta = Meta()
+ for callback in self._grid_callbacks:
@jtackaberry

jtackaberry May 16, 2012

Owner

Should not bother creating a Meta object (or dict, as the case may be) unless _grid_callbacks is not empty.

Owner

charrea6 commented May 16, 2012

I'll change the meta object to a dict as although it's longer to type known attributes it's not so easy to do the 'default setting if not available'.

As for the name I don't like userdata, far too vague were as I feel meta is correct as it is data about data, ie in this case its data about the program object, if it is schedule to record, whether is a favorite etc. Its not additional data on the program which should be additional attributes.

Owner

jtackaberry commented May 16, 2012

The term "userdata" is commonly used in libraries and has a clear meaning (if not formal, definitely de facto): it's data that's not generated from within the library, rather it's opaque data provided by the caller elsewhere and merely passed along by the library.

I don't think stuff like "is this program scheduled to be recorded" constitutes metadata. If you define "data" in this context to be the program and "metadata" to be "data about the program" then even the program title and description would qualify as metadata under that definition. I think whether the program is a favourite is in the same category as genre and rating, which are attributes of the Program object. To me, metadata in this context would be stuff like the source of the program data (schedulesdirect.org, say), or when it was last updated from the server, etc.

So IMO "userdata" is much less vague than "metadata" because at least has some descriptive value.

Owner

charrea6 commented May 17, 2012

Another reason I don't like userdata is that it suggests its being added by the caller somehow or is some sort of callback data (originating from the user of the API), which it isn't.
The server is providing it from another part of the server (ignoring the local case here I know), therefore its not user data, it may however be additional data. So how about a compromise additional_info rather that userdata or meta.

Owner

jtackaberry commented May 18, 2012

Maybe I'm missing something important here. I thought the actual data is being originated by the user of the API? It seems to me that the client/server distinction is a red herring. The recordserver is a user of the kaa.epg API, extending the data passed back to the client via the grid callback.

So from kaa.epg's perspective, this is userdata. I think your objection is that from the client side, this doesn't smell at all like userdata because the server provided that data, and it doesn't necessarily know or appreciate the fact that the server actually extended kaa.epg to provide that extra info.

Does that seem like a fair statement?

I usually try to avoid attribute names with underscores if possible. So maybe extrainfo? Another option we might consider is whether it makes sense to just have Client.get_grid() merge the extrainfo dict straight into the program object's __dict__. If the goal is to be able to extend the Program object in a way that the client shouldn't have to know or care about, this might be acceptable (although it's not without its downside). What do you think?

I don't want to wear you down on this stuff, because I do appreciate your contribution. I've voiced my opinion and provided a couple other options, so go ahead and do what you think is right. :)

Owner

charrea6 commented May 18, 2012

Yes and Yes, I beginning to think about this not best way to do things but am struggling to see a better solution.
I've got a solution currently just for Freevo 1 where this call is in the recordserver RPC context but this is ugly and doesn't give properly created EPG Program objects back, channel for one is probably duplicated rather than a ref to the client context channel.

extrainfo sounds fine but, and here's a doh moment, what about gridinfo?
I think keeping it separate is best as it is obvious that these extra bits are only available when you call get_grid.

Don't worry about wearing me down, I'm used to working in a place where everything is code reviewed (down to single character changes) so this isn't new to me, plus sometimes it the best way to get a good API.

Owner

jtackaberry commented May 18, 2012

gridinfo is fine with me if you think this approach of having callbacks attach additional data to Program objects is going to stay unique to get_grid()? Do you think there might be other times where Program objects would be augmented with external data?

Maybe it makes sense to make this completely generic, so that any time a Program object is returned by the API, it invokes user specified callbacks to attach new data? In that case it could be reasonable to merge the extrainfo dict into the program object's __dict__ because those attributes would be available in all cases.

Owner

charrea6 commented May 18, 2012

That's an interesting point as it would conceivable make sense for search() to return the same sort of information as get_grid(), ie transient information such as scheduled/favorite status. You'd want to know that information in the UI so you could modify the behaviour appropriately.

Perhaps the best thing to do therefore is to pass this extra information to the Program class on creation so that it can either add it to the object as attributes or as a single attribute containing a dictionary. For the standard Program class it gets added to the object dictionary.

Ok, I'll go with that and expand the behaviour to search as well.

@charrea6 charrea6 Remove get_grid and change search to call the callbacks instead.
This way all queries of the database will have access to the additional information. Also remove the meta object and instead apply the returned properties to the Program object as attributes.

 Modifiy the grid test program to separate the programs itself and then check the test value as before.
97c9fb9

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

results = []
- channel = None
- for row in query_data:
- if not channel or row['parent_id'] != channel.db_id:
- if row['parent_id'] not in self._channels_by_db_id:
- continue
- channel = self._channels_by_db_id[row['parent_id']]
- results.append(cls(channel, row))
- yield results
+ for i,row in enumerate(query_data):
+ channel = self._channels_by_db_id[row['parent_id']]
@jtackaberry

jtackaberry May 22, 2012

Owner

You seem to have removed some of the verifications done on channel and row['parent_id']. Did you determine that those checks are unnecessary?

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

@@ -302,3 +327,9 @@ def get_genres(self, associated=None, prefix=None):
@property
def num_programs(self):
return self._num_programs
+
+def to_timestamp(dt):
+ 'Converts a time to a unix timestamp (seconds since epoch UTC)'
@jtackaberry

jtackaberry May 22, 2012

Owner

Different docstring formatting. As a matter of style, kaa uses """ everywhere (with """ always being on dedicated lines). (Ok, this looks like it was part of the original inline function. My bad :))

Owner

jtackaberry commented May 22, 2012

I prefer this now, much more general purpose.

Looking at the register/unregister callback stuff, I now wonder if we shouldn't instead us a Signal. This is just what signals are: something happens and we want to give the user a way to attach/detach callbacks to do stuff when that thing happens. In this case, Guide would subclass kaa.Object and define a __kaasignals__ attribute with something like a new-program signal. (You can see sockets.py in kaa.base for a good example.)

By reusing the Signal machinery, this should simplify the patch even more, and make it more kaa-like.

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

results = []
- channel = None
- for row in query_data:
- if not channel or row['parent_id'] != channel.db_id:
- if row['parent_id'] not in self._channels_by_db_id:
- continue
- channel = self._channels_by_db_id[row['parent_id']]
- results.append(cls(channel, row))
- yield results
+ for i,row in enumerate(query_data):
+ channel = self._channels_by_db_id[row['parent_id']]
+ if extra_data is None:
+ extra_info = None
+ else:
+ extra_info = extra_data[i]
+ results.append(cls(channel, row, extra_info))
@jtackaberry

jtackaberry May 22, 2012

Owner

extra_info vs extra_data is somewhat confusing to read. Might just avoid extra_info altogether:

results.append(cls(channel, row, extra_data[i] if extra_data else None))

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

else:
- time = convert(time)
- query_data = yield self.channel.rpc('search', channel, time, None, **kwargs)
+ time = to_timestamp(time)
+ query_data,extra_data = yield self.channel.rpc('search', channel, time, None, **kwargs)
+ if cls is None:
+ yield query_data,extra_data
@jtackaberry

jtackaberry May 22, 2012

Owner

Missing space after comma (twice in the previous 3 lines).

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

@@ -211,20 +201,55 @@ def convert(dt):
def combine_attrs(row):
return [ row.get(a) for a in attrs ]
[ combine_attrs(row) for row in query_data ]
+ extra_data = None
+ if self._program_callbacks:
+ extra_data = []
+ for row in query_data:
+ extra_info = {}
+ for callback in self._program_callbacks:
+ callback(row['id'], extra_info)
+ extra_data.append(extra_info)
@jtackaberry

jtackaberry May 22, 2012

Owner

Initialize extra_data to None in an else instead of unconditionally. Reads better.

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

@@ -211,20 +201,55 @@ def convert(dt):
def combine_attrs(row):
return [ row.get(a) for a in attrs ]
[ combine_attrs(row) for row in query_data ]
+ extra_data = None
+ if self._program_callbacks:
+ extra_data = []
+ for row in query_data:
+ extra_info = {}
+ for callback in self._program_callbacks:
+ callback(row['id'], extra_info)
@jtackaberry

jtackaberry May 22, 2012

Owner

Why not pass the whole row object? It would give the callbacks more flexibility as to what they can do.

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

@@ -219,6 +203,10 @@ def search(self, channel, time, cls, **kwargs):
return self.guide.search(channel, time, cls, **kwargs)
@kaa.rpc.expose()
+ def get_grid(self, channels, start_time, end_time, cls):
@jtackaberry

jtackaberry May 22, 2012

Owner

get_grid should be removed now.

@jtackaberry jtackaberry commented on an outdated diff May 22, 2012

@@ -247,3 +235,4 @@ def client_closed(self, client):
"""
log.info('Client disconnected: %s', client)
self._clients.remove(client)
+
@jtackaberry

jtackaberry May 22, 2012

Owner

Spurious whitespace.

@jtackaberry jtackaberry added a commit that referenced this pull request May 23, 2012

@jtackaberry jtackaberry Merge pull request #2 from charrea6/master
Add program-retrieved signal to Guide to allow custom Program attributes
872dd9d

@jtackaberry jtackaberry merged commit 872dd9d into freevo:master May 23, 2012

Owner

jtackaberry commented May 23, 2012

Looks great, Adam. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment