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

[Olivier Tilloy]

[Olivier Tilloy (osomon)]

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.

[Guillaume Emont (guijemont)]

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)

=== modified file 'elisa-plugins/elisa/plugins/gstreamer/decodebin2_pipeline.py'
--- elisa-plugins/elisa/plugins/gstreamer/decodebin2_pipeline.py	2009-11-13 18:24:48 +0000
+++ elisa-plugins/elisa/plugins/gstreamer/decodebin2_pipeline.py	2010-01-08 12:24:30 +0000
@@ -675,8 +675,30 @@
         self.debug('unknown pad %s, caps %s' % (pad, caps))
 
     def _plug_src(self, uri):
-        quoted_uri = quote(uri, '/:')
-        src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
+        """
+        @type uri: C{str}
+        """
+        if uri.startswith('file://'):
+            # Hack to workaround the messy handling of file paths using MediaUri
+            # objects. In various places in the code, file paths are not
+            # URL-escaped when building a MediaUri. It results in invalid URLs
+            # if the file path contains reserved characters
+            # (see http://tools.ietf.org/html/rfc3986#section-2.2).
+            # See https://bugs.launchpad.net/moovida/+bug/374305 and
+            # https://bugs.launchpad.net/moovida/+bug/296517 for displays of
+            # this issue.
+            # Note that while we use MediaUri objects, the clean way to fix this
+            # mess is to make sure that file paths are correctly URL-escaped and
+            # unescaped where needed (but that would be a tedious, invasive and
+            # risky patch).
+            quoted_uri = quote(uri, ':/')
+            src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
+        else:
+            # We quote only file:// URLs. Other MediaUri objects for distant
+            # resources are assumed to be built correctly
+            # (i.e. already URL-escaped).
+            src = gst.element_make_from_uri(gst.URI_SRC, uri)
+
         # FIXME: workaround for jpegdec that does a gst_buffer_join for each
         # gst_pad_chain.
         src.props.blocksize = 1 * 1024 * 1024
 
=== modified file 'elisa-plugins/elisa/plugins/gstreamer/old_pipeline.py'
--- elisa-plugins/elisa/plugins/gstreamer/old_pipeline.py	2009-11-13 18:24:48 +0000
+++ elisa-plugins/elisa/plugins/gstreamer/old_pipeline.py	2010-01-08 12:24:30 +0000
@@ -541,8 +541,30 @@
         self.debug('unknown pad %s, caps %s' % (pad, caps))
 
     def _plug_src(self, uri):
-        quoted_uri = quote(uri, '/:')
-        src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
+        """
+        @type uri: C{str}
+        """
+        if uri.startswith('file://'):
+            # Hack to workaround the messy handling of file paths using MediaUri
+            # objects. In various places in the code, file paths are not
+            # URL-escaped when building a MediaUri. It results in invalid URLs
+            # if the file path contains reserved characters
+            # (see http://tools.ietf.org/html/rfc3986#section-2.2).
+            # See https://bugs.launchpad.net/moovida/+bug/374305 and
+            # https://bugs.launchpad.net/moovida/+bug/296517 for displays of
+            # this issue.
+            # Note that while we use MediaUri objects, the clean way to fix this
+            # mess is to make sure that file paths are correctly URL-escaped and
+            # unescaped where needed (but that would be a tedious, invasive and
+            # risky patch).
+            quoted_uri = quote(uri, ':/')
+            src = gst.element_make_from_uri(gst.URI_SRC, quoted_uri)
+        else:
+            # We quote only file:// URLs. Other MediaUri objects for distant
+            # resources are assumed to be built correctly
+            # (i.e. already URL-escaped).
+            src = gst.element_make_from_uri(gst.URI_SRC, uri)
+
         # FIXME: workaround for jpegdec that does a gst_buffer_join for each
         # gst_pad_chain.
         src.props.blocksize = 1 * 1024 * 1024
 
=== modified file 'elisa-plugins/elisa/plugins/poblesec/player_video.py'
--- elisa-plugins/elisa/plugins/poblesec/player_video.py	2009-12-01 12:16:26 +0000
+++ elisa-plugins/elisa/plugins/poblesec/player_video.py	2010-01-08 12:24:30 +0000
@@ -695,9 +695,22 @@
                     unicode_uri = unicode(file_uri)
                     sub_uri = \
                         unicode_uri.encode(locale_helper.gst_file_encoding())
-                    quoted_sub_uri = quote(sub_uri, '/:')
+                    # Hack to workaround the messy handling of file paths using
+                    # MediaUri objects. In various places in the code, file
+                    # paths are not URL-escaped when building a MediaUri. It
+                    # results in invalid URLs if the file path contains reserved
+                    # characters
+                    # (see http://tools.ietf.org/html/rfc3986#section-2.2).
+                    # See https://bugs.launchpad.net/moovida/+bug/374305 and
+                    # https://bugs.launchpad.net/moovida/+bug/296517 for
+                    # displays of this issue.
+                    # Note that while we use MediaUri objects, the clean way to
+                    # fix this mess is to make sure that file paths are
+                    # correctly URL-escaped and unescaped where needed (but that
+                    # would be a tedious, invasive and risky patch).
+                    quoted_sub_uri = quote(sub_uri, ':/')
                     self.pipeline.set_property('suburi', quoted_sub_uri)
-                    self.info("Loaded subtitles at %r", sub_uri)
+                    self.info("Loaded subtitles at %r", quoted_sub_uri)
                     found = True
                     break
         if not found:
@@ -808,8 +821,26 @@
         self.filename = model.title or model.uri.filename
         self.stop()
         uri = unicode(model.uri).encode(locale_helper.gst_file_encoding())
-        quoted_uri = quote(uri, '/:')
-        self.pipeline.set_property('uri', quoted_uri)
+        if uri.startswith('file://'):
+            # Hack to workaround the messy handling of file paths using MediaUri
+            # objects. In various places in the code, file paths are not
+            # URL-escaped when building a MediaUri. It results in invalid URLs
+            # if the file path contains reserved characters
+            # (see http://tools.ietf.org/html/rfc3986#section-2.2).
+            # See https://bugs.launchpad.net/moovida/+bug/374305 and
+            # https://bugs.launchpad.net/moovida/+bug/296517 for displays of
+            # this issue.
+            # Note that while we use MediaUri objects, the clean way to fix this
+            # mess is to make sure that file paths are correctly URL-escaped and
+            # unescaped where needed (but that would be a tedious, invasive and
+            # risky patch).
+            quoted_uri = quote(uri, ':/')
+            self.pipeline.set_property('uri', quoted_uri)
+        else:
+            # We quote only file:// URLs. Other MediaUri objects for distant
+            # resources are assumed to be built correctly
+            # (i.e. already URL-escaped).
+            self.pipeline.set_property('uri', uri)
         self._load_subs(model.uri)
 
         self.update_visualization()