Merge lp:~vthompson/mediascanner2/add-case-insensitive-sorting into lp:mediascanner2

Proposed by Victor Thompson on 2014-11-10
Status: Needs review
Proposed branch: lp:~vthompson/mediascanner2/add-case-insensitive-sorting
Merge into: lp:mediascanner2
Diff against target: 70 lines (+8/-3)
1 file modified
src/mediascanner/MediaStore.cc (+8/-3)
To merge this branch: bzr merge lp:~vthompson/mediascanner2/add-case-insensitive-sorting
Reviewer Review Type Date Requested Status
James Henstridge Needs Fixing on 2017-02-24
unity-api-1-bot continuous-integration 2014-11-10 Approve on 2017-02-23
Review via email: mp+241230@code.launchpad.net

Commit message

Add case insensitive sorting.

Description of the change

This adds case insensitive sorting.

However, from the Music app side of things there is still another issue. Currently we use the SDK's SortFilterModel to sort the SongsModel on the Songs tab and the AlbumsModel on the Albums tab since both of these models are sorted by artist by default. Unfortunately, the SortFilterModel also has the same A-Za-z sorting issue, so we'd like to be able to drop it and instead have the model sorted by song title in the Songs tab (SongsModel) and by album name in the Albums tab (AlbumsModel). Do we need to file a bug for this, or do we just need assistance in getting the model properly sorted?

To post a comment you must log in.
unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:
https://jenkins.canonical.com/unity-api-1/job/lp-mediascanner2-ci/18/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/1716
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/1723
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1498
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/1498/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1498
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=zesty/1498/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1498
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/1498/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1498
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=zesty/1498/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1498
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/1498/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1498
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=zesty/1498/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-mediascanner2-ci/18/rebuild

review: Approve (continuous-integration)
dobey (dobey) wrote :

I'm not sure what the test coverage is like for this code, but would like to see some regression tests for this as well if possible.

James Henstridge (jamesh) wrote :

I remember discussing this with Victor at the time on IRC, and I think the main issues were:

(a) SQLite's "nocase" collation only helped with characters in the 7-bit ASCII range.

(b) The change means we're no longer taking advantage of the indexes to sort results.

And just looking at the diff now, I can see a third problem:

(c) the "COLLATE $collation" modifier binds to the individual terms of the ORDER BY clause rather than being an independent clause itself. This means that for the places where we sort by multiple columns, we're only doing a nocase collation on the last term.

For the sorting issue, it is possible to create indexes like "CREATE INDEX idx_name ON table (column COLLATE nocase)" which can be used for a nocase sorted result set. But I can't seem to avoid the sort when combined with an indexed WHERE filter.

review: Needs Fixing

Unmerged revisions

289. By Victor Thompson on 2014-11-10

* Add case insensitive sorting

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/mediascanner/MediaStore.cc'
2--- src/mediascanner/MediaStore.cc 2014-09-24 09:39:06 +0000
3+++ src/mediascanner/MediaStore.cc 2014-11-10 02:41:25 +0000
4@@ -555,7 +555,7 @@
5 }
6 break;
7 case MediaOrder::Title:
8- qs += " ORDER BY title";
9+ qs += " ORDER BY title COLLATE NOCASE";
10 if (filter.getReverse()) {
11 qs += " DESC";
12 }
13@@ -610,7 +610,7 @@
14 switch (filter.getOrder()) {
15 case MediaOrder::Default:
16 case MediaOrder::Title:
17- qs += " ORDER BY album";
18+ qs += " ORDER BY album COLLATE NOCASE";
19 if (filter.getReverse()) {
20 qs += " DESC";
21 }
22@@ -645,7 +645,7 @@
23 switch (filter.getOrder()) {
24 case MediaOrder::Default:
25 case MediaOrder::Title:
26- qs += " ORDER BY artist";
27+ qs += " ORDER BY artist COLLATE NOCASE";
28 if (filter.getReverse()) {
29 qs += " DESC";
30 }
31@@ -716,6 +716,7 @@
32 }
33 qs += R"(
34 ORDER BY album_artist, album, disc_number, track_number, title
35+COLLATE NOCASE
36 LIMIT ? OFFSET ?
37 )";
38 Statement query(db, qs.c_str());
39@@ -756,6 +757,7 @@
40 qs += R"(
41 GROUP BY album, album_artist
42 ORDER BY album_artist, album
43+COLLATE NOCASE
44 LIMIT ? OFFSET ?
45 )";
46 Statement query(db, qs.c_str());
47@@ -787,6 +789,7 @@
48 qs += R"(
49 GROUP BY artist
50 ORDER BY artist
51+ COLLATE NOCASE
52 LIMIT ? OFFSET ?
53 )";
54 Statement query(db, qs.c_str());
55@@ -816,6 +819,7 @@
56 qs += R"(
57 GROUP BY album_artist
58 ORDER BY album_artist
59+ COLLATE NOCASE
60 LIMIT ? OFFSET ?
61 )";
62 Statement query(db, qs.c_str());
63@@ -840,6 +844,7 @@
64 WHERE type = ?
65 GROUP BY genre
66 ORDER BY genre
67+ COLLATE NOCASE
68 LIMIT ? OFFSET ?
69 )");
70 query.bind(1, (int)AudioMedia);

Subscribers

People subscribed via source and target branches