Merge lp:~osomon/moovida/fix_uri_quoting into lp:moovida

Proposed by Olivier Tilloy
Status: Merged
Merged at revision: not available
Proposed branch: lp:~osomon/moovida/fix_uri_quoting
Merge into: lp:moovida
Diff against target: 131 lines (+83/-8)
3 files modified
elisa-plugins/elisa/plugins/gstreamer/decodebin2_pipeline.py (+24/-2)
elisa-plugins/elisa/plugins/gstreamer/old_pipeline.py (+24/-2)
elisa-plugins/elisa/plugins/poblesec/player_video.py (+35/-4)
To merge this branch: bzr merge lp:~osomon/moovida/fix_uri_quoting
Reviewer Review Type Date Requested Status
Guillaume Emont code functional Approve
Review via email: mp+17013@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Olivier Tilloy (osomon) wrote :

This patch fixes the symptoms of bug #491874.
It is a refinement of Fernando's patch (see http://bazaar.launchpad.net/~moovida-developers/moovida/relook/revision/1599): only "file://" URLs are escaped. URLs for distant resources thus remain unchanged.

As extensively explained in the comments I added to the code, this patch is by no means a proper fix to the underlying issue, which is that "file://" URIs are not correctly built in the first place. It is merely a quick fix, as unintrusive as possible.

The careful reviewer will make sure that the changes are relevant and that the comments make sense. He will also verify that bug #491874 is fixed in a variety of situations [1], and that indexing, thumbnailing and playback of local audio and video files containing reserved characters work fine. In particular, there shouldn't be regressions on bug #374305 and bug #296517. As noted by Fernando in the first patch, indexing and thumbnailing of pictures containing reserved characters should also work, but displaying them in the slideshow player will fail. This can be considered as a separate issue.

Thanks for all the shoes!

Olivier

[1] Tests can be done using the arte plugin using e.g. a French proxy or the grooveshark plugin. Note that for the latter, another issue prevents playback, but a patched egg is attached to bug #451125.

Revision history for this message
Guillaume Emont (guijemont) wrote :

worksforme(tm), and I'm ok with the changes.
Did test on ubuntu hardy and vista. I didn't test with chinese characters, but considering the changes, if there is no regression with the # character, there shouldn't be any regression with chinese characters either.
I guess the comment could be shorter (ideally), but I have nothing better to propose, so I'm OK with that.

review: Approve (code functional)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'elisa-plugins/elisa/plugins/gstreamer/decodebin2_pipeline.py'
2--- elisa-plugins/elisa/plugins/gstreamer/decodebin2_pipeline.py 2009-11-13 18:24:48 +0000
3+++ elisa-plugins/elisa/plugins/gstreamer/decodebin2_pipeline.py 2010-01-08 12:24:30 +0000
4@@ -675,8 +675,30 @@
5 self.debug('unknown pad %s, caps %s' % (pad, caps))
6
7 def _plug_src(self, uri):
8- quoted_uri = quote(uri, '/:')
9- src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
10+ """
11+ @type uri: C{str}
12+ """
13+ if uri.startswith('file://'):
14+ # Hack to workaround the messy handling of file paths using MediaUri
15+ # objects. In various places in the code, file paths are not
16+ # URL-escaped when building a MediaUri. It results in invalid URLs
17+ # if the file path contains reserved characters
18+ # (see http://tools.ietf.org/html/rfc3986#section-2.2).
19+ # See https://bugs.launchpad.net/moovida/+bug/374305 and
20+ # https://bugs.launchpad.net/moovida/+bug/296517 for displays of
21+ # this issue.
22+ # Note that while we use MediaUri objects, the clean way to fix this
23+ # mess is to make sure that file paths are correctly URL-escaped and
24+ # unescaped where needed (but that would be a tedious, invasive and
25+ # risky patch).
26+ quoted_uri = quote(uri, ':/')
27+ src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
28+ else:
29+ # We quote only file:// URLs. Other MediaUri objects for distant
30+ # resources are assumed to be built correctly
31+ # (i.e. already URL-escaped).
32+ src = gst.element_make_from_uri(gst.URI_SRC, uri)
33+
34 # FIXME: workaround for jpegdec that does a gst_buffer_join for each
35 # gst_pad_chain.
36 src.props.blocksize = 1 * 1024 * 1024
37
38=== modified file 'elisa-plugins/elisa/plugins/gstreamer/old_pipeline.py'
39--- elisa-plugins/elisa/plugins/gstreamer/old_pipeline.py 2009-11-13 18:24:48 +0000
40+++ elisa-plugins/elisa/plugins/gstreamer/old_pipeline.py 2010-01-08 12:24:30 +0000
41@@ -541,8 +541,30 @@
42 self.debug('unknown pad %s, caps %s' % (pad, caps))
43
44 def _plug_src(self, uri):
45- quoted_uri = quote(uri, '/:')
46- src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
47+ """
48+ @type uri: C{str}
49+ """
50+ if uri.startswith('file://'):
51+ # Hack to workaround the messy handling of file paths using MediaUri
52+ # objects. In various places in the code, file paths are not
53+ # URL-escaped when building a MediaUri. It results in invalid URLs
54+ # if the file path contains reserved characters
55+ # (see http://tools.ietf.org/html/rfc3986#section-2.2).
56+ # See https://bugs.launchpad.net/moovida/+bug/374305 and
57+ # https://bugs.launchpad.net/moovida/+bug/296517 for displays of
58+ # this issue.
59+ # Note that while we use MediaUri objects, the clean way to fix this
60+ # mess is to make sure that file paths are correctly URL-escaped and
61+ # unescaped where needed (but that would be a tedious, invasive and
62+ # risky patch).
63+ quoted_uri = quote(uri, ':/')
64+ src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
65+ else:
66+ # We quote only file:// URLs. Other MediaUri objects for distant
67+ # resources are assumed to be built correctly
68+ # (i.e. already URL-escaped).
69+ src = gst.element_make_from_uri(gst.URI_SRC, uri)
70+
71 # FIXME: workaround for jpegdec that does a gst_buffer_join for each
72 # gst_pad_chain.
73 src.props.blocksize = 1 * 1024 * 1024
74
75=== modified file 'elisa-plugins/elisa/plugins/poblesec/player_video.py'
76--- elisa-plugins/elisa/plugins/poblesec/player_video.py 2009-12-01 12:16:26 +0000
77+++ elisa-plugins/elisa/plugins/poblesec/player_video.py 2010-01-08 12:24:30 +0000
78@@ -695,9 +695,22 @@
79 unicode_uri = unicode(file_uri)
80 sub_uri = \
81 unicode_uri.encode(locale_helper.gst_file_encoding())
82- quoted_sub_uri = quote(sub_uri, '/:')
83+ # Hack to workaround the messy handling of file paths using
84+ # MediaUri objects. In various places in the code, file
85+ # paths are not URL-escaped when building a MediaUri. It
86+ # results in invalid URLs if the file path contains reserved
87+ # characters
88+ # (see http://tools.ietf.org/html/rfc3986#section-2.2).
89+ # See https://bugs.launchpad.net/moovida/+bug/374305 and
90+ # https://bugs.launchpad.net/moovida/+bug/296517 for
91+ # displays of this issue.
92+ # Note that while we use MediaUri objects, the clean way to
93+ # fix this mess is to make sure that file paths are
94+ # correctly URL-escaped and unescaped where needed (but that
95+ # would be a tedious, invasive and risky patch).
96+ quoted_sub_uri = quote(sub_uri, ':/')
97 self.pipeline.set_property('suburi', quoted_sub_uri)
98- self.info("Loaded subtitles at %r", sub_uri)
99+ self.info("Loaded subtitles at %r", quoted_sub_uri)
100 found = True
101 break
102 if not found:
103@@ -808,8 +821,26 @@
104 self.filename = model.title or model.uri.filename
105 self.stop()
106 uri = unicode(model.uri).encode(locale_helper.gst_file_encoding())
107- quoted_uri = quote(uri, '/:')
108- self.pipeline.set_property('uri', quoted_uri)
109+ if uri.startswith('file://'):
110+ # Hack to workaround the messy handling of file paths using MediaUri
111+ # objects. In various places in the code, file paths are not
112+ # URL-escaped when building a MediaUri. It results in invalid URLs
113+ # if the file path contains reserved characters
114+ # (see http://tools.ietf.org/html/rfc3986#section-2.2).
115+ # See https://bugs.launchpad.net/moovida/+bug/374305 and
116+ # https://bugs.launchpad.net/moovida/+bug/296517 for displays of
117+ # this issue.
118+ # Note that while we use MediaUri objects, the clean way to fix this
119+ # mess is to make sure that file paths are correctly URL-escaped and
120+ # unescaped where needed (but that would be a tedious, invasive and
121+ # risky patch).
122+ quoted_uri = quote(uri, ':/')
123+ self.pipeline.set_property('uri', quoted_uri)
124+ else:
125+ # We quote only file:// URLs. Other MediaUri objects for distant
126+ # resources are assumed to be built correctly
127+ # (i.e. already URL-escaped).
128+ self.pipeline.set_property('uri', uri)
129 self._load_subs(model.uri)
130
131 self.update_visualization()

Subscribers

People subscribed via source and target branches