Merge lp:~aacid/unity/remove_qsortfilterproxymodelqml_get into lp:unity/phablet

Proposed by Albert Astals Cid
Status: Merged
Approved by: Michał Sawicz
Approved revision: no longer in the source branch.
Merged at revision: 666
Proposed branch: lp:~aacid/unity/remove_qsortfilterproxymodelqml_get
Merge into: lp:unity/phablet
Diff against target: 181 lines (+23/-48)
7 files modified
Dash/Dash.qml (+0/-16)
Dash/DashHome.qml (+3/-4)
Dash/DashVideos.qml (+13/-5)
Dash/Video/VideoPreview.qml (+3/-3)
Dash/Video/VideosFilterGrid.qml (+4/-2)
plugins/Utils/qsortfilterproxymodelqml.cpp (+0/-17)
plugins/Utils/qsortfilterproxymodelqml.h (+0/-1)
To merge this branch: bzr merge lp:~aacid/unity/remove_qsortfilterproxymodelqml_get
Reviewer Review Type Date Requested Status
Michał Sawicz Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+162955@code.launchpad.net

Commit message

Remove ::get() in qsortfilterproxymodelqml

Using ::get() means that all the data of the model for a given row is obtained which in most of the cases is unnecessary and in some could mean getting expensive data. This data should be somewhere in the QML already so we should use that and not re-query it again

Description of the change

The new if in DashVideos.qml is a bit ugly, but if we don't want that it'll require a bit of surgery at the Carousel level, that given that we plan to use the Listview'ed one "soon" i'd prefer not to do at the moment.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

Please fix the comment re: the function - it did work, but wasn't called in correct places. Since we're switching to smart scopes soon, this will no longer be there anyway. So maybe we just drop it altogether already?

Please also add a comment to the commit message as to why we're removing the ::get().

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Dash/Dash.qml'
--- Dash/Dash.qml 2013-02-27 17:36:32 +0000
+++ Dash/Dash.qml 2013-05-08 13:31:29 +0000
@@ -30,21 +30,6 @@
30 width: units.gu(40)30 width: units.gu(40)
31 height: units.gu(71)31 height: units.gu(71)
3232
33 function updateLensesViewType(currentLensIndex, homeLensSelected) {
34 /* Lens.viewType need to be set for all lenses when switching lenses.
35 If showing the home lens, viewType must be set to "HomeView",
36 otherwise viewType must be set to "Hidden" for the non current lenses.
37 */
38 for (var i=0; i<filteredLenses.count; i++) {
39 var lens = filteredLenses.get(i)
40 if (homeLensSelected) {
41 lens.viewType = (i === currentLensIndex) ? Lens.LensView : Lens.HomeView
42 } else {
43 lens.viewType = (i === currentLensIndex) ? Lens.LensView : Lens.Hidden
44 }
45 }
46 }
47
48 function setCurrentLens(lensId, animate, reset) {33 function setCurrentLens(lensId, animate, reset) {
49 var lensIndex = filteredLenses.findFirst(Lenses.RoleId, lensId)34 var lensIndex = filteredLenses.findFirst(Lenses.RoleId, lensId)
5035
@@ -58,7 +43,6 @@
58 return43 return
59 }44 }
6045
61 updateLensesViewType(lensIndex, (lensId == "home.lens"))
62 dashContent.setCurrentLensAtIndex(lensIndex, animate, reset)46 dashContent.setCurrentLensAtIndex(lensIndex, animate, reset)
63 }47 }
6448
6549
=== modified file 'Dash/DashHome.qml'
--- Dash/DashHome.qml 2013-04-16 09:32:38 +0000
+++ Dash/DashHome.qml 2013-05-08 13:31:29 +0000
@@ -156,10 +156,9 @@
156 //FIXME: workaround for lack of previews for videos in Home lens.156 //FIXME: workaround for lack of previews for videos in Home lens.
157 //Need to connect to the clicked() signal here and act upon it here instead.157 //Need to connect to the clicked() signal here and act upon it here instead.
158 if (component === "VideosGrid") {158 if (component === "VideosGrid") {
159 function playVideo(index) {159 function playVideo(index, data) {
160 var video = item.model.get(index);160 if (data.fileUri) {
161 if (video && video.fileName) {161 shell.activateApplication('/usr/share/applications/media-player.desktop', "/usr/share/demo-assets/videos/" + data.fileUri);
162 shell.activateApplication('/usr/share/applications/media-player.desktop', "/usr/share/demo-assets/videos/" + video.fileName);
163 }162 }
164 }163 }
165164
166165
=== modified file 'Dash/DashVideos.qml'
--- Dash/DashVideos.qml 2013-04-29 23:47:43 +0000
+++ Dash/DashVideos.qml 2013-05-08 13:31:29 +0000
@@ -117,9 +117,17 @@
117 Connections {117 Connections {
118 target: loader.item118 target: loader.item
119 onClicked: {119 onClicked: {
120 var item = loader.item.model.get(index)120 var dataItem;
121 if (item.column_5 != "") {121 // VideosCarousel and VideosFilterGrid have different
122 previewLoader.videoItem = loader.item.model.get(index);122 // clicked signals, accomodate for that
123 if (categoryId == 0) {
124 var fileUri = delegateItem.model.column_0.replace(/^[^:]+:/, "")
125 dataItem = {fileUri: fileUri, nfoUri: delegateItem.model.column_5}
126 } else {
127 dataItem = data;
128 }
129 if (dataItem.nfoUri != "") {
130 previewLoader.videoData = dataItem;
123 previewLoader.open = true;131 previewLoader.open = true;
124 effect.positionPx = mapToItem(categoryView, 0, itemY).y;132 effect.positionPx = mapToItem(categoryView, 0, itemY).y;
125 }133 }
@@ -156,7 +164,7 @@
156164
157 property bool open: false165 property bool open: false
158 property bool onScreen: false166 property bool onScreen: false
159 property var videoItem167 property var videoData
160168
161169
162 onOpenChanged: {170 onOpenChanged: {
@@ -166,7 +174,7 @@
166 }174 }
167175
168 onLoaded: {176 onLoaded: {
169 item.item = videoItem;177 item.item = videoData;
170 }178 }
171 }179 }
172180
173181
=== modified file 'Dash/Video/VideoPreview.qml'
--- Dash/Video/VideoPreview.qml 2013-02-27 17:20:00 +0000
+++ Dash/Video/VideoPreview.qml 2013-05-08 13:31:29 +0000
@@ -24,15 +24,15 @@
2424
25 property var item25 property var item
26 property alias ready: nfo.ready26 property alias ready: nfo.ready
27 property url fileUri: item ? item.column_0.replace(/^[^:]+:/, "") : ""27 readonly property url fileUri: item ? item.fileUri : ""
2828
29 VideoInfo {29 VideoInfo {
30 id: nfo30 id: nfo
31 source: item ? item.column_5 : ""31 source: item ? item.nfoUri : ""
32 }32 }
3333
34 title: nfo.ready ? nfo.video.title : ""34 title: nfo.ready ? nfo.video.title : ""
35 url: item ? item.column_5.replace(/\.nfo$/, "-fanart.jpg") : ""35 url: item ? item.nfoUri.replace(/\.nfo$/, "-fanart.jpg") : ""
36 previewWidthRatio: 0.636 previewWidthRatio: 0.6
37 playable: fileUri != ""37 playable: fileUri != ""
3838
3939
=== modified file 'Dash/Video/VideosFilterGrid.qml'
--- Dash/Video/VideosFilterGrid.qml 2013-04-22 10:10:47 +0000
+++ Dash/Video/VideosFilterGrid.qml 2013-05-08 13:31:29 +0000
@@ -30,7 +30,7 @@
30 readonly property int iconWidth: (width / columns) * 0.830 readonly property int iconWidth: (width / columns) * 0.8
31 readonly property int iconHeight: iconWidth * 16 / 1131 readonly property int iconHeight: iconWidth * 16 / 11
3232
33 signal clicked(int index, real itemY)33 signal clicked(int index, var data, real itemY)
3434
35 delegate: Tile {35 delegate: Tile {
36 id: tile36 id: tile
@@ -43,7 +43,9 @@
43 source: model.column_143 source: model.column_1
44 fillMode: Image.PreserveAspectCrop44 fillMode: Image.PreserveAspectCrop
45 onClicked: {45 onClicked: {
46 filtergrid.clicked(index, tile.y);46 var fileUri = model.column_0.replace(/^[^:]+:/, "")
47 var data = {fileUri: fileUri, nfoUri: model.column_5}
48 filtergrid.clicked(index, data, tile.y);
47 }49 }
48 }50 }
49}51}
5052
=== modified file 'plugins/Utils/qsortfilterproxymodelqml.cpp'
--- plugins/Utils/qsortfilterproxymodelqml.cpp 2013-05-08 13:24:15 +0000
+++ plugins/Utils/qsortfilterproxymodelqml.cpp 2013-05-08 13:31:29 +0000
@@ -57,23 +57,6 @@
57 }57 }
58}58}
5959
60QVariantMap
61QSortFilterProxyModelQML::get(int row)
62{
63 if (sourceModel() == NULL) {
64 return QVariantMap();
65 }
66
67 const QModelIndex modelIndex = index(row, 0);
68 QVariantMap result;
69 QHashIterator<int, QByteArray> i(roleNames());
70 while (i.hasNext()) {
71 i.next();
72 result[i.value()] = modelIndex.data(i.key());
73 }
74 return result;
75}
76
77int60int
78QSortFilterProxyModelQML::totalCount() const61QSortFilterProxyModelQML::totalCount() const
79{62{
8063
=== modified file 'plugins/Utils/qsortfilterproxymodelqml.h'
--- plugins/Utils/qsortfilterproxymodelqml.h 2013-04-16 15:54:41 +0000
+++ plugins/Utils/qsortfilterproxymodelqml.h 2013-05-08 13:31:29 +0000
@@ -31,7 +31,6 @@
31public:31public:
32 explicit QSortFilterProxyModelQML(QObject *parent = 0);32 explicit QSortFilterProxyModelQML(QObject *parent = 0);
3333
34 Q_INVOKABLE QVariantMap get(int row);
35 Q_INVOKABLE int count();34 Q_INVOKABLE int count();
36 Q_INVOKABLE int findFirst(int role, const QVariant& value) const;35 Q_INVOKABLE int findFirst(int role, const QVariant& value) const;
37 virtual bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const;36 virtual bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const;

Subscribers

People subscribed via source and target branches