Differential D8178

Fix editing items in SimpleFavoritesModel
ClosedPublic
Actions

Authored by fvogt on Oct 7 2017, 12:32 PM.

Details

Reviewers
davidedmundson
Group Reviewers
Plasma
Commits
R120:065a0fcfb035: Fix editing items in SimpleFavoritesModel
R119:06f01ed291ca: Fix editing items in SimpleFavoritesModel
Summary

tools.js is supposed to support both KaStatsFavoritesModel (used for
favorite applications) and SimpleFavoritesModel (used for system actions,
like poweroff/reboot/logout). The latter did not work though, as tools.js
unconditionally calls methods only present in KaStatsFavoritesModel.
This commit makes KaStatsFavoritesModel API compatible to SimpleFavoritesModel
and changes the relevant code in tools.js to support both.

Additionally, this syncs applets/kickoff/package/contents/code/tools.js again.

BUG: 385463

Test Plan

Before this patch it wasn't possible to remove system actions from
the favorites bar.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fvogt created this revision.Oct 7 2017, 12:32 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 7 2017, 12:32 PM
fvogt added a comment.Oct 7 2017, 1:31 PM

Someone needs to have a look at the kickoff tools.js diff - it got out of sync apparently.

rikmills added a comment.Oct 8 2017, 6:56 AM

Built with the above patch, and on quick testing it seems to resolve the issue for me. I am not a user of activities, so my testing there to make sure existing behaviour on the standard favourites was still the same, may not be as thorough as desirable.

davidedmundson accepted this revision.Oct 10 2017, 12:11 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
applets/kickoff/package/contents/code/tools.js
32 ↗(On Diff #20437)

I don't understand the comment; are you explaining why you're using unshift instead of concat?

177 ↗(On Diff #20437)

?

This revision is now accepted and ready to land.Oct 10 2017, 12:11 PM
hein added a subscriber: hein.Oct 10 2017, 12:45 PM

Please make sure the spam is gone (console.log), etc.

The "this crashes Qt" comment": That was written by Ivan originally.

Please make sure and test that this patch works with both kinds of favorites.

I'm travelling and am indisposed currently.

fvogt added a comment.Oct 10 2017, 12:47 PM

re. the comments: I did not write any of that, it simply got added by the tools.js sync. If this is supposed to be removed, please tell Ivan.
I did test this patch for all types of favorites, but not in relation to activities as I'm not too familiar with them.

Closed by commit R119:06f01ed291ca: Fix editing items in SimpleFavoritesModel (authored by fvogt). · Explain WhyOct 10 2017, 4:53 PM
This revision was automatically updated to reflect the committed changes.
ivan added a subscriber: ivan.Oct 11 2017, 8:28 PM
ivan added inline comments.
applets/kicker/package/contents/code/tools.js
185 ↗(On Diff #20437)

I don't like this change. Would rather have this stated explicitly than rely on the implementation in the favmodel. Can you make this part also aware of which model is behind the favoriteModel variable?

ivan added a comment.Oct 11 2017, 8:28 PM

Somehow I didn't submit the comment when I wrote it. See above.

fvogt added inline comments.Oct 12 2017, 4:26 AM
applets/kicker/package/contents/code/tools.js
185 ↗(On Diff #20437)

With yet another `_kicker_favorite_remove_from_any``` method? I did a search for removeFavorite and it's not used outside of this file, so the API should be as minimal as possible IMO.

Revision Contents
Changeset List

Diff 20579

View Options

applets/kicker/package/contents/code/tools.js

Loading...
View Options

applets/kicker/plugin/kastatsfavoritesmodel.cpp

Loading...
View Options

applets/kickoff/package/contents/code/tools.js

Loading...