Merge lp:~aacid/unity8/decodeAudioURIs into lp:unity8

Proposed by Albert Astals Cid
Status: Work in progress
Proposed branch: lp:~aacid/unity8/decodeAudioURIs
Merge into: lp:unity8
Diff against target: 65 lines (+6/-6)
3 files modified
plugins/Dash/CardCreator.js (+2/-2)
qml/Dash/Previews/PreviewAudioPlayback.qml (+1/-1)
tests/plugins/Dash/cardcreator/9.res (+3/-3)
To merge this branch: bzr merge lp:~aacid/unity8/decodeAudioURIs
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Unity8 CI Bot continuous-integration Needs Fixing
Unity Team Pending
Review via email: mp+283226@code.launchpad.net

Commit message

decode music URIs since they are sent percent encoded

Description of the change

 * Are there any related MPs required for this MP to build/function as expected?
No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * Did you make sure that your branch does not contain spurious tags?
Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

 * If you changed the UI, has there been a design review?
N/A

To post a comment you must log in.
Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:2137
https://unity8-jenkins.ubuntu.com/job/lp-unity8-1-ci/141/
Executed test runs:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-1-ci/141/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2137
http://jenkins.qa.ubuntu.com/job/unity8-ci/7105/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6081
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-xenial-touch/520/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/1810
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity8-qmluitest-xenial-amd64/513
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/1705
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/1705
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-xenial-amd64-ci/512
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-xenial-i386-ci/511
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/4695
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6092
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6092/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26805
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-xenial-touch/258/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-xenial-armhf/518
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-xenial-armhf/518/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26806

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7105/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

Test please?

Revision history for this message
Albert Astals Cid (aacid) wrote :

I'm not sure what you want to test here.

In my opinion the only valid test would be a full stack test that creates a file with [ on the filename, gets the mediascanner to parse it, waits for the music scope to show it, then we send that url to the qt audio player and check the url it says it is playing is the same.

And i don't see how to do that with our test facilities.

Using mocks for any of the steps above basically means that if any of the other actors change their behaviours the test will still work but the real thing will fail, making the test rather useless.

lp:~aacid/unity8/decodeAudioURIs updated
2138. By Albert Astals Cid

decodeURI on the test

Revision history for this message
Unity8 CI Bot (unity8-ci-bot) wrote :

FAILED: Continuous integration, rev:2138
https://unity8-jenkins.ubuntu.com/job/lp-unity8-1-ci/145/
Executed test runs:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-1-ci/145/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:2138
http://jenkins.qa.ubuntu.com/job/unity8-ci/7108/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/6088
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-xenial-touch/523/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/1813
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-qmluitest-xenial-amd64/516
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/1708
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/1708
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-xenial-amd64-ci/515
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-xenial-i386-ci/514
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/4699
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6099
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/6099/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26824
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-xenial-touch/260/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-xenial-armhf/521
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-xenial-armhf/521/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26823

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/7108/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

Is decodeURI really the right choice here? The result won't necessarily be a valid URI, which the property would be expecting.

As I understand it, we've got two pieces of code generating URLs for files on disk that choose to escape different sets of characters. Just decoding the escapes in URLs from one source won't necessarily get all equivalent URLs to match. Instead, it should probably be using a normalisation transform as discussed in https://tools.ietf.org/html/rfc3986#section-6

Revision history for this message
Albert Astals Cid (aacid) wrote :

"The result won't necessarily be a valid URI"

Why not?

Revision history for this message
James Henstridge (jamesh) wrote :

> "The result won't necessarily be a valid URI"
>
> Why not?

Sorry, missed your question. Looking at the docs, it seems it won't decode certain reserved characters, so it probably won't create an invalid URI.

The result won't necessarily refer to the same file though, which is a problem. For example:

  >> decodeURI("file:///hello%253f.mp3")
  "file:///hello%3f.mp3"

Here the input refers to a file "hello%3f.mp3", and the result to a file "hello?.mp3".

Revision history for this message
Albert Astals Cid (aacid) wrote :

So you're escaping the url twice?

Revision history for this message
James Henstridge (jamesh) wrote :

No. I'm giving an example of a URI that will refer to a different file if you run it through decodeURI.

If you want to match equivalent URIs, then you want something that does the kind of normalisation described in the link I gave in the previous comment. I don't know if Qt/QML provides such a function, but decodeURI() isn't it.

Revision history for this message
Albert Astals Cid (aacid) wrote :

FWIW decodeURI() is not Qt/QML, it's Javascript

Revision history for this message
Michi Henning (michihenning) wrote :

All of section 6 talks basically about the difficulty of figuring out whether two URLs denote the same resource or not. A string comparison is inadequate because there are many URLs that all represent the same resource, such as file:///home/michi vs file:///%86ome/michi

It's similar to canonical path names. If we have something that depends on figuring out whether the resource denoted by a URL is the same as the resource denoted by some other URL, both URLs need to be converted into an unambiguous representation before making the equality comparison.

I apologise if I'm teaching my grandmother to suck eggs. (That's an Australianism...)

Revision history for this message
Paweł Stołowski (stolowski) wrote :

I don't think we're getting any closer to fixing the problem. I recon the problem is convoluted, but I think we need something fast even if not perfect (I just found out it affects inline playback badly when testing some of the changes requested for the upcoming OTA).

I admit don't understand all the intricate details of URI decoding, but can we accept Albert's workaround if it works with typical cases we have (this needs to be tested and confirmed yet), and potentially log a bug to make it more robust?

Revision history for this message
James Henstridge (jamesh) wrote :

Perhaps the following algorithm would handle the majority of cases as a normalisation algorithm:

1. Check the URI scheme. If it isn't "file", then the original URI is the normalised URI.
2. Otherwise, convert to a local file name with QUrl::toLocalFile
3. Optionally try to canonicalise the file name (this might not be needed).
4. Convert that filename to the normalised URI with QUrl::fromLocalFile

That won't help with HTTP URIs, but should ensure that file URIs for the same path are identical.

I'm not sure you can do all this from QML, so it might need to be implemented in C++ then exposed.

Revision history for this message
Michi Henning (michihenning) wrote :

For file URLs, all paths should be absolute anyway. boost has a nice canonical() function that turns an absolute path name into an unambiguous one (no ".", "..", unravels symbolic links, etc.)

Revision history for this message
Albert Astals Cid (aacid) wrote :

I'm going to declare this blocked until https://bugs.launchpad.net/canonical-devices-system-image/+bug/1449790 gets fixed because i need the media-hub to get fixed so that the layer on top of it can get fixed otherwise i'm not sure if i'm doing it right, wrong or workarounding media-hub bugs themselves.

Unmerged revisions

2138. By Albert Astals Cid

decodeURI on the test

2137. By Albert Astals Cid

decode music URIs since they are sent percent encoded

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/Dash/CardCreator.js'
2--- plugins/Dash/CardCreator.js 2015-12-18 15:48:32 +0000
3+++ plugins/Dash/CardCreator.js 2016-01-20 15:10:36 +0000
4@@ -144,7 +144,7 @@
5 anchors.fill: %1; \n\
6 width: %2; \n\
7 height: %3; \n\
8- readonly property url source: (cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || ""; \n\
9+ readonly property url source: decodeURI((cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || ""); \n\
10 UbuntuShape { \n\
11 anchors.fill: parent; \n\
12 visible: parent.pressed; \n\
13@@ -359,7 +359,7 @@
14 var kAudioProgressBarCode = 'CardAudioProgress { \n\
15 id: audioProgressBar; \n\
16 duration: (cardData["quickPreviewData"] && cardData["quickPreviewData"]["duration"]) || 0; \n\
17- source: (cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || ""; \n\
18+ source: decodeURI((cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || ""); \n\
19 anchors { \n\
20 bottom: %1; \n\
21 left: %2; \n\
22
23=== modified file 'qml/Dash/Previews/PreviewAudioPlayback.qml'
24--- qml/Dash/Previews/PreviewAudioPlayback.qml 2015-12-01 14:02:21 +0000
25+++ qml/Dash/Previews/PreviewAudioPlayback.qml 2016-01-20 15:10:36 +0000
26@@ -49,7 +49,7 @@
27 id: trackItem
28 objectName: "trackItem" + index
29
30- readonly property url sourceUrl: modelData["source"]
31+ readonly property url sourceUrl: decodeURI(modelData["source"])
32 readonly property bool isPlayingItem: AudioUrlComparer.compare(sourceUrl, DashAudioPlayer.currentSource)
33
34 anchors { left: parent.left; right: parent.right }
35
36=== modified file 'tests/plugins/Dash/cardcreator/9.res'
37--- tests/plugins/Dash/cardcreator/9.res 2016-01-11 15:40:59 +0000
38+++ tests/plugins/Dash/cardcreator/9.res 2016-01-20 15:10:36 +0000
39@@ -3,7 +3,7 @@
40 property var components;
41 property var cardData;
42 property string artShapeStyle: "inset";
43- property string backgroundShapeStyle: "inset";
44+ property string backgroundShapeStyle: "inset";
45 property real fontScale: 1.0;
46 property var scopeStyle: null;
47 property int titleAlignment: Text.AlignLeft;
48@@ -61,7 +61,7 @@
49 CardAudioProgress {
50 id: audioProgressBar;
51 duration: (cardData["quickPreviewData"] && cardData["quickPreviewData"]["duration"]) || 0;
52- source: (cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || "";
53+ source: decodeURI((cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || "");
54 anchors {
55 bottom: audioButton.bottom;
56 left: audioButton.right;
57@@ -74,7 +74,7 @@
58 anchors.fill: undefined;
59 width: height;
60 height: (root.fixedHeaderHeight > 0 ? root.fixedHeaderHeight : headerHeight) + 2 * units.gu(1);
61- readonly property url source: (cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || "";
62+ readonly property url source: decodeURI((cardData["quickPreviewData"] && cardData["quickPreviewData"]["uri"]) || "");
63 UbuntuShape {
64 anchors.fill: parent;
65 visible: parent.pressed;

Subscribers

People subscribed via source and target branches