Review Request 126027: Port Kickoff to the Kicker backend

Eike Hein hein at kde.org
Wed Nov 11 15:27:11 UTC 2015 


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/computermodel.cpp, line 264
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416142#file416142line264>
> >
> >     why do this? trigger is virtual

But it's not const, while QModelIndex::model() is, and I don't like using const casts.


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/computermodel.cpp, line 41
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416142#file416142line41>
> >
> >     this is now the default

I prefer making things explicit, but I dropped it.


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/computermodel.cpp, line 173
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416142#file416142line173>
> >
> >     this is unused?

No, it's used by AppEntry.


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/computermodel.cpp, line 213
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416142#file416142line213>
> >
> >     url.toString(QUrl::PreferLocalFile)

Changed.


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/recentusagemodel.cpp, line 60
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416152#file416152line60>
> >
> >     sourceModel->setParent(this) in the ctor might be better?
> >     
> >     It would ensure sourceModel exists during the  QSortFitlerProxy destructor, which I assume is why you're doing this setSourceModel(0)

Very good idea, done.


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/recentusagemodel.cpp, line 112
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416152#file416152line112>
> >
> >     this method can be just
> >     
> >     sourceModel()->data(...)
> >     
> >     if sourceModel happens to be a GroupSortProxyModel it will forward the data to the right index itself.

QSortFilterProxyModel cares about the model pointer checking out, so it needs to be an index created from the GroupSortProxyModel rather than the ForwardingModel.


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/recentusagemodel.cpp, line 253
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416152#file416152line253>
> >
> >     why only for this KRun job, not the others?

Dunno, ask Martin why it's needed for apps.


> On Nov. 11, 2015, 2:38 p.m., David Edmundson wrote:
> > applets/kicker/plugin/computermodel.cpp, line 83
> > <https://git.reviewboard.kde.org/r/126027/diff/1/?file=416142#file416142line83>
> >
> >     return QSortFilterProxyModel::lessThan()
> >     
> >     I dont't think you can rely on it following the source model's order.
> >     I had a look at qsortfilterproxy model code; sort() uses stable sort so it implicitly will, but the insert methods don't seem to.
> >     
> >     Also without this your explicit setSortRole will do nothing.

I flipped back and forth on this because I'd prefer retaining the order and this at least makes sure the initial load follows user places order, but it's true it's not very good. I'll go with alphabetic for now and address sorting further after this is merged.


- Eike


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126027/#review88252
-----------------------------------------------------------


On Nov. 11, 2015, 12:27 p.m., Eike Hein wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126027/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2015, 12:27 p.m.)
> 
> 
> Review request for Plasma and Ivan Čukić.
> 
> 
> Repository: plasma-desktop
> 
> 
> Description
> -------
> 
> This is a roll-up of the various Kicker backend and ultimately Kickoff changes needed to obsolete Kickoff's old backend in favor of Kicker's. It raises Kickoff to Kicker's feature level (much more fleshed-out context menus with many additional features, pervasive favoriting, etc.) along with addressing numerous Kickoff bugs (e.g. sorting on the Computer page) and removing some cruft.
> 
> Migration from kickoffrc is done using a script and supports both favorites and ISV-customized system apps for the Computer page.
> 
> I suggest using the hein/kickeroff branch for review instead, as things are nicely split out into separate commits there.
> 
> 
> Diffs
> -----
> 
>   applets/kicker/CMakeLists.txt bdb6419 
>   applets/kicker/plugin/abstractentry.h e09b3cf 
>   applets/kicker/plugin/abstractentry.cpp 846a548 
>   applets/kicker/plugin/abstractmodel.h 142de27 
>   applets/kicker/plugin/abstractmodel.cpp 18eed03 
>   applets/kicker/plugin/actionlist.h 4aaa1d5 
>   applets/kicker/plugin/appentry.h 3d0b1bd 
>   applets/kicker/plugin/appentry.cpp 34bab3d 
>   applets/kicker/plugin/appsmodel.h 3cd0577 
>   applets/kicker/plugin/appsmodel.cpp 821ae88 
>   applets/kicker/plugin/computermodel.h PRE-CREATION 
>   applets/kicker/plugin/computermodel.cpp PRE-CREATION 
>   applets/kicker/plugin/favoritesmodel.cpp 4931a16 
>   applets/kicker/plugin/fileentry.h cd4e54a 
>   applets/kicker/plugin/fileentry.cpp 693816d 
>   applets/kicker/plugin/kickerplugin.cpp d31043a 
>   applets/kicker/plugin/recentappsmodel.h 6ee55c1 
>   applets/kicker/plugin/recentappsmodel.cpp 3d167c5 
>   applets/kicker/plugin/recentdocsmodel.h df12d1b 
>   applets/kicker/plugin/recentdocsmodel.cpp ae7306f 
>   applets/kicker/plugin/recentusagemodel.h PRE-CREATION 
>   applets/kicker/plugin/recentusagemodel.cpp PRE-CREATION 
>   applets/kicker/plugin/rootmodel.h 5acdb8e 
>   applets/kicker/plugin/rootmodel.cpp fde16fd 
>   applets/kicker/plugin/runnermatchesmodel.cpp d375fd7 
>   applets/kicker/plugin/runnermodel.h d827e6d 
>   applets/kicker/plugin/runnermodel.cpp 4eb6ccc 
>   applets/kicker/plugin/systementry.h df0712b 
>   applets/kicker/plugin/systementry.cpp ec527da 
>   applets/kicker/plugin/systemmodel.h 1b3eab7 
>   applets/kicker/plugin/systemmodel.cpp ef02892 
>   applets/kickoff/CMakeLists.txt 339316d 
>   applets/kickoff/STATUS-TODO f837793 
>   applets/kickoff/core/applicationmodel.h cc28656 
>   applets/kickoff/core/applicationmodel.cpp a292130 
>   applets/kickoff/core/favoritesmodel.h 7a5f225 
>   applets/kickoff/core/favoritesmodel.cpp be47347 
>   applets/kickoff/core/itemhandlers.h 8f05490 
>   applets/kickoff/core/itemhandlers.cpp d0cd423 
>   applets/kickoff/core/kickoffabstractmodel.h be5ce21 
>   applets/kickoff/core/kickoffabstractmodel.cpp 43afb1b 
>   applets/kickoff/core/kickoffmodel.h dd91f9b 
>   applets/kickoff/core/kickoffmodel.cpp fd5bb62 
>   applets/kickoff/core/kickoffplugin.h de839b3 
>   applets/kickoff/core/kickoffplugin.cpp ef6cbdd 
>   applets/kickoff/core/kickoffproxymodel.h f1af640 
>   applets/kickoff/core/kickoffproxymodel.cpp 8c58d2b 
>   applets/kickoff/core/krunnermodel.h 3916829 
>   applets/kickoff/core/krunnermodel.cpp f8c031b 
>   applets/kickoff/core/leavemodel.h f95a271 
>   applets/kickoff/core/leavemodel.cpp 154eb1c 
>   applets/kickoff/core/models.h 30e80be 
>   applets/kickoff/core/models.cpp a63d363 
>   applets/kickoff/core/org.kde.kickoff.recent.xml d013f62 
>   applets/kickoff/core/org.kde.kickoff.xml b0f5d09 
>   applets/kickoff/core/processrunner.h 5d836eb 
>   applets/kickoff/core/processrunner.cpp 6572386 
>   applets/kickoff/core/qmldir d454d1b 
>   applets/kickoff/core/recentapplications.h b73d526 
>   applets/kickoff/core/recentapplications.cpp 2dd6781 
>   applets/kickoff/core/recentlyusedmodel.h 2530fb4 
>   applets/kickoff/core/recentlyusedmodel.cpp afeed23 
>   applets/kickoff/core/systemmodel.h 5da49ad 
>   applets/kickoff/core/systemmodel.cpp 461a2ed 
>   applets/kickoff/core/urlitemlauncher.h 05c414d 
>   applets/kickoff/core/urlitemlauncher.cpp c350680 
>   applets/kickoff/package/contents/code/tools.js PRE-CREATION 
>   applets/kickoff/package/contents/config/config.qml fe059b8 
>   applets/kickoff/package/contents/config/main.xml 00dfdf9 
>   applets/kickoff/package/contents/ui/ActionMenu.qml PRE-CREATION 
>   applets/kickoff/package/contents/ui/ApplicationsView.qml ad5272a 
>   applets/kickoff/package/contents/ui/BaseView.qml e628614 
>   applets/kickoff/package/contents/ui/Breadcrumb.qml 016cf25 
>   applets/kickoff/package/contents/ui/ComputerView.qml PRE-CREATION 
>   applets/kickoff/package/contents/ui/ConfigGeneral.qml PRE-CREATION 
>   applets/kickoff/package/contents/ui/ContextMenu.qml e8f681e 
>   applets/kickoff/package/contents/ui/FavoritesView.qml f286592 
>   applets/kickoff/package/contents/ui/Footer.qml dcc762f 
>   applets/kickoff/package/contents/ui/FullRepresentation.qml a7559ac 
>   applets/kickoff/package/contents/ui/Kickoff.qml 33b49eb 
>   applets/kickoff/package/contents/ui/KickoffButton.qml ac1e96a 
>   applets/kickoff/package/contents/ui/KickoffItem.qml f7038cc 
>   applets/kickoff/package/contents/ui/KickoffLeaveItem.qml 3b06395 
>   applets/kickoff/package/contents/ui/LeaveView.qml 00e758e 
>   applets/kickoff/package/contents/ui/RecentlyUsedView.qml df25db2 
>   applets/kickoff/package/contents/ui/SearchView.qml f75a7ec 
>   applets/kickoff/package/contents/ui/SystemView.qml e0093f0 
>   applets/kickoff/package/contents/ui/configGeneral.qml d754c34 
>   desktoppackage/contents/updates/obsolete_kickoffrc.js PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126027/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Eike Hein
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20151111/77e7628a/attachment-0001.html>

More information about the Plasma-devel mailing list