Remove 'magic' filtering of recent apps
Needs ReviewPublic

Authored by tcanabrava on Aug 26 2019, 12:14 PM.

Details

Summary

The recent app wasn't displayed if it's recent *and* favorites.

Diff Detail

Repository
R119 Plasma Desktop
Branch
removeMagicFiltering
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15737
Build 15755: arc lint + arc unit
tcanabrava created this revision.Aug 26 2019, 12:14 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 26 2019, 12:14 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
tcanabrava requested review of this revision.Aug 26 2019, 12:14 PM
davidedmundson requested changes to this revision.Aug 26 2019, 3:18 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
applets/kicker/plugin/recentusagemodel.cpp
98

This is doing two things.

  1. It's removing any favourite in the history that returns a defunct service. i.e if dolphin is a favourite and you uninstall dolphin it will go away.
  1. removing duplicates from the favourites model.

We definitely still want 1.

2 definitely makes sense to be optional, even off by default, but simply removing it will change kicker.

Is there a reason to change kicker? If not, it needs to be a property.

This revision now requires changes to proceed.Aug 26 2019, 3:18 PM
tcanabrava updated this revision to Diff 64798.Aug 28 2019, 8:29 AM
  • Revert "Remove 'magic' filtering of recent apps"
  • Remove filtering of recent apps that are in the favorites

Is there a reason to change kicker? If not, it needs to be a property.

Is there a reason to change kicker? If not, it needs to be a property.

There was a discussion a week ago with @mart and @ngraham and the result was "yeah, looks like it's a bug dressed as a feature, it's better that we streamline this."
So we voted to change kicker as the current behavior is "unpredictable" to the user.

mart added a comment.Aug 28 2019, 9:16 AM

It does introduce a bahavior change in kicker, tough we tought that ux-wise it was a really bad behavior that ought to be "fixed"

mart added a comment.Aug 28 2019, 9:26 AM
This comment was removed by mart.
  • Revert "Remove filtering of recent apps that are in the favorites"
  • Add a new Q_PROPERTY to the Recents Model to filter out or not favorites
applets/kicker/plugin/recentusagemodel.cpp
522

If this is called before refresh model will be null, and then we don't update m_showAllRecent

This needs to be outside the if statement.

Or you can simplify the whole thing and just use refresh()

applets/kicker/plugin/recentusagemodel.h
43

We can't say "Current kicker" as people will read this in the future.

I would write it as "filterFavourites" as that explains better what the opposite of showAll is.

In general I'm not a big fan of this kind of pseudo-smart behavior. Unless it's perfect, the smartness winds up looking buggy in various cases. I'm in favor of removing the behavior entirely.

@ngraham I agree with you completely. it's basically removing a bit of code, smaller patch and the code behaves without surprises.