[Sugar-devel] [PATCH] Playlist for Jukebox
Sascha Silbe
sascha-ml-reply-to-2011-3 at silbe.org
Mon Sep 26 16:27:50 EDT 2011
Excerpts from Manuel Quiñones's message of 2011-09-16 07:02:18 +0200:
[ControlToolbar.py]
> class Control(gobject.GObject):
> """Class to create the Control (play) toolbar"""
> diff --git a/jukeboxactivity.py b/jukeboxactivity.py
> index 0ba3f4c..98acdd4 100644
> --- a/jukeboxactivity.py
> +++ b/jukeboxactivity.py
> @@ -24,6 +24,7 @@
>
> import logging
> from gettext import gettext as _
> +import cjson
You might want to consider using the Python 2.6 json module with
fallback to simplejson (which is the same thing for Python < 2.6)
instead:
try:
import json
except ImportError:
import simplejson as json
cjson is buggy [1]; simplejson has been used by Sugar since 0.79.2, so
should be available on all systems.
> + def __size_allocate_cb(self, widget, allocation):
> + canvas_size = self.bin.get_allocation()
> + self.playlist_widget.set_size_request(canvas_size.width/3, 0)
This looks like a dirty hack, but I can't find a better way. Maybe one
of our GTK experts can point out a good solution?
> @@ -310,17 +325,57 @@ class JukeboxActivity(activity.Activity):
> jobject = chooser.get_selected_object()
> if jobject and jobject.file_path:
> self.jobjectlist.append(jobject)
> - self._start(jobject.file_path)
> + title = jobject.metadata.get('title', None)
None is the default for dict.get(), so ...get('title') should suffice.
> + def _get_data_from_file_path(self, file_path):
> + fd = open(file_path, 'r')
> + try:
> + data = fd.read()
> + finally:
> + fd.close()
> + return data
You can just do "return fd.read()", finally will still work. And since
you're only reading a single file, letting the garbage collector close
the file should work well enough, so you can simplify this to:
def _get_data_from_file_path(self, file_path):
return open(file_path).read()
Since the code is now shorter than the name of the function, you could
even just use it inline. ;)
> def read_file(self, file_path):
> - self.uri = os.path.abspath(file_path)
> - if os.path.islink(self.uri):
> - self.uri = os.path.realpath(self.uri)
> - gobject.idle_add(self._start, self.uri)
> + def deserialize_playlist(data):
> + return cjson.decode(data)
> +
> + def get_uri_title(file_path):
> + """Return the media URI and the title to show in the playlist."""
> + uri = os.path.abspath(file_path)
> + title = os.path.basename(file_path)
> + if os.path.islink(uri):
> + uri = os.path.realpath(uri)
> + return uri, title
> +
> + if self.metadata['mime_type'] == 'text/plain':
> + logging.debug("reading playlist from file...")
> + data = self._get_data_from_file_path(file_path)
> + playlist = deserialize_playlist(data)
With json / simplejson you can deserialise directly from a file:
playlist = json.load(open(file_path))
> + for elem in playlist:
> + gobject.idle_add(self._start, elem['url'], elem['title'])
> +
> + else:
> + logging.debug("reading one media from file...")
> + self.uri, title = get_uri_title(file_path)
> + gobject.idle_add(self._start, self.uri, title)
> +
> + def write_file(self, file_path):
> + def serialize_playlist(playlist):
> + return cjson.encode(playlist)
> +
> + logging.debug("writing playlist to file...")
> + self.metadata['mime_type'] = 'text/plain'
> + playlist_str = serialize_playlist(self.playlist)
> + f = open(file_path, 'w')
> + try:
> + f.write(playlist_str)
> + finally:
> + f.close()
Similarly, you can serialise directly to the file. Since you are writing
(not reading) the explicit close() call is required, though:
f = open(file_path, 'w')
try:
json.dump(f, self.playlist)
finally:
f.close()
It could be argued that you only care about the data being available to
other processes (which is what close() ensures) if there was no error,
in which case you could get rid of the try/finally (but keep the
f.close() after json.dump()). The try/finally doesn't hurt, though.
[_start()]
> if uri.endswith(".m3u") or uri.endswith(".m3u8"):
> - self.playlist.extend(self.getplaylist([line.strip() for line in open(uri).readlines()]))
> + for line in open(uri).readlines():
A file object is an iterator that returns each line, so
for line in open(uri):
should suffice.
> + url = line.strip()
> + self.playlist.extend({'url': url, 'title': title})
Are you sure this does what you want it to do? Did you mean append
instead of extend?
> elif uri.endswith('.pls'):
> try:
> cf.readfp(open(uri))
> x = 1
> while True:
> - self.playlist.append(cf.get("playlist",'File'+str(x)))
> + self.playlist.append({'url': cf.get("playlist",'File'+str(x)),
> + 'title': title})
> x += 1
> except:
> #read complete
> pass
How about:
try:
cf.read([uri])
except ConfigParser.Error:
logging.exception('Could not read playlist %r', uri)
for idx in range(cf.get('playlist', 'NumberOfEntries'):
if not cf.has_option('playlist', 'File' + str(idx)):
continue
url = cf.get('playlist', 'File' + str(idx))
if cf.has_option('playlist', 'Title' + str(idx)):
cur_title = cf.get('playlist', 'Title' + str(idx))
else:
cur_title = title
self.playlist.append({'url': url, 'title': cur_title})
Or if you want to cope with files that don't have NumberOfEntries:
items = sorted(cf.items('playlist'))
urls = [path for name, url in items if name.startswith('File')]
titles = [titl for name, titl in items if name.startswith('Title')]
if len(titles) != len(urls):
titles = [title] * len(urls)
self.playlist += [{'uri': url, 'title': cur_title}
for url, cur_title in zip(paths, titles)]
> + def __toggle_playlist_cb(self, toolbar, active):
> + if active:
> + self.playlist_widget.show_all()
> + else:
> + self.playlist_widget.hide()
> + self.bin.queue_draw()
Are you sure queue_draw() is necessary? Shouldn't show() / hide()
already trigger a redraw?
[widgets.py]
> @@ -0,0 +1,87 @@
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
Please consider using LGPL v3 (or even GPLv3 - I don't see why you'd
want proprietary code to be able to use widgets.py), following the
recent referendum on licenses for Sugar Labs projects. [3]
This is a recommendation only - as a contributor, it's your choice how
you license your code (and the maintainers choice whether to accept
contributions under a given license).
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> +# USA
As a side effect, switching to the recommended (L)GPLv3 boilerplate text
would get rid of the outdated FSF address.
> +import logging
> +from gettext import gettext as _
> +
> +import pygtk
> +pygtk.require('2.0')
It doesn't hurt to do this, but every system running Sugar is using
GTK 2, so no need to check for it.
Note: I'm just pointing out a few pieces of code that I think could be
done in a better way. Whether or not the maintainer of Jukebox agrees
with my point of view and would accept the changes I suggested I do not
know.
Sascha
[1] https://bugs.sugarlabs.org/ticket/1553
[2] http://lists.laptop.org/pipermail/devel/2011-July/032529.html
--
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110926/edcdea25/attachment-0001.pgp>
More information about the Sugar-devel
mailing list