From kde-panel-devel Tue May 12 09:32:15 2015 From: "Konrad Materka" Date: Tue, 12 May 2015 09:32:15 +0000 To: kde-panel-devel Subject: Re: Review Request 123426: Kickoff is not mounting removable devices and shows "Invalid URL" Message-Id: <20150512093215.8613.48403 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-panel-devel&m=143142315214267 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============8305393666196389054==" --===============8305393666196389054== Content-Type: multipart/alternative; boundary="===============5262109013940834283==" --===============5262109013940834283== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123426/ ----------------------------------------------------------- (Updated May 12, 2015, 9:32 a.m.) Status ------ This change has been marked as submitted. Review request for Plasma. Changes ------- Submitted with commit 49a1a2827639b3099a2f16fc3c8eef6130e4d95d by Marco Martin to branch master. Bugs: 346002 None Description ------- Fixed version of /r/123350/ Why VisualDelegateModel? It may be workaround/hack/you name it but it was the only way I found acceptable. You need to pass QModelIndex to openItem and I don't know how to get it using plain ListView and SystemModel. VisualDelegateModel has a convenience method modelIndex which is already used in KickoffItem.qml (but only if ApplicationsView tab is active). I changed BaseView to use VisualDelegateModel to have modelIndex method and to be more consistent with ApplicaitonView. "this change seems uneeded; we're using modelIndex fine in the old code, see line 40 in KickoffItem.qml in the old code. (unless maybe that didn't work ?) My understanding is setting the model to a QAIM, internally just creates a VDM anyway." It didn't work. modelIndex is used only if flag hasModelChildren is set to true. That can happen only for ApplicationView, as only there children exist. Also ApplicationView uses VisualDataModel explicitly, so method modelIndex is available. In SystemView model is set directly, VisualDataModel is not created implicitly (or even if it is it is not accessible). In other words listItem.ListView.view.model returns SystemModel object which is QAbstractProxyModel/QAbstractItemModel implementation. SystemModel does not have any slot that would be usefull in this case. Passing "model" variable is not an option. It would be a good idea as it contains all data related to current item but this is internal QML/Qt Quick class. In KDE 4 it was not a problem as it was easy to get QModelIndex from event (lines 860 - 878): https://projects.kde.org/projects/kde/kde-workspace/repository/entry/plasma/desktop/applets/kickoff/ui/launcher.cpp?rev=KDE%2F4.9 Diffs ----- Diff: https://git.reviewboard.kde.org/r/123426/diff/ Testing ------- I was able to reproduce bug using latest GIT code. Screenshots, first shows not mounted removable device, second is error message (in Polish): https://www.dropbox.com/s/8vcokx17kmn3onw/zrzut%20ekranu1.png?dl=0&s=sl https://www.dropbox.com/s/sqkci1z1fhb3h3p/zrzut%20ekranu2.png?dl=0&s=sl In console I can find this message: Opening item with URL "" It looks that KFilePlacesModel->url(index) returns empty string for not mounted devices. I have 5.9.0 version of libkf5kiocore5 installed. Patch fixed mounting. I also tested all tabs, all items works as expected. File Attachments ---------------- Use VisualDataModel->modelIndex in openItem https://git.reviewboard.kde.org/media/uploaded/files/2015/04/19/4a0d0c4d-0f76-429b-8c4a-6f9ba9856816__Bug-346002.patch Thanks, Konrad Materka --===============5262109013940834283== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123426/

This change has been marked as submitted.


Review request for Plasma.
By Konrad Materka.

Updated May 12, 2015, 9:32 a.m.

Changes

Submitted with commit 49a1a2827639b3099a2f16fc3c8eef6130e4d95d by Marco Martin to branch master.
Bugs: 346002

Description

Fixed version of /r/123350/

Why VisualDelegateModel? It may be workaround/hack/you name it but it was the only way I found acceptable. You need to pass QModelIndex to openItem and I don't know how to get it using plain ListView and SystemModel. VisualDelegateModel has a convenience method modelIndex which is already used in KickoffItem.qml (but only if ApplicationsView tab is active). I changed BaseView to use VisualDelegateModel to have modelIndex method and to be more consistent with ApplicaitonView.

"this change seems uneeded; we're using modelIndex fine in the old code, see line 40 in KickoffItem.qml in the old code. (unless maybe that didn't work ?) My understanding is setting the model to a QAIM, internally just creates a VDM anyway."

It didn't work. modelIndex is used only if flag hasModelChildren is set to true. That can happen only for ApplicationView, as only there children exist. Also ApplicationView uses VisualDataModel explicitly, so method modelIndex is available. In SystemView model is set directly, VisualDataModel is not created implicitly (or even if it is it is not accessible). In other words listItem.ListView.view.model returns SystemModel object which is QAbstractProxyModel/QAbstractItemModel implementation. SystemModel does not have any slot that would be usefull in this case. Passing "model" variable is not an option. It would be a good idea as it contains all data related to current item but this is internal QML/Qt Quick class.

In KDE 4 it was not a problem as it was easy to get QModelIndex from event (lines 860 - 878): https://projects.kde.org/projects/kde/kde-workspace/repository/entry/plasma/desktop/applets/kickoff/ui/launcher.cpp?rev=KDE%2F4.9

Testing

I was able to reproduce bug using latest GIT code. Screenshots, first shows not mounted removable device, second is error message (in Polish): https://www.dropbox.com/s/8vcokx17kmn3onw/zrzut%20ekranu1.png?dl=0&s=sl https://www.dropbox.com/s/sqkci1z1fhb3h3p/zrzut%20ekranu2.png?dl=0&s=sl

In console I can find this message: Opening item with URL ""

It looks that KFilePlacesModel->url(index) returns empty string for not mounted devices. I have 5.9.0 version of libkf5kiocore5 installed.

Patch fixed mounting. I also tested all tabs, all items works as expected.

Diffs

View Diff

File Attachments

  • Use VisualDataModel->modelIndex in openItem
  • --===============5262109013940834283==-- --===============8305393666196389054== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KUGxhc21hLWRl dmVsIG1haWxpbmcgbGlzdApQbGFzbWEtZGV2ZWxAa2RlLm9yZwpodHRwczovL21haWwua2RlLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL3BsYXNtYS1kZXZlbAo= --===============8305393666196389054==--