D23461: Remove 'magic' filtering of recent apps

[Tomaz Canabrava]

tcanabrava added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: mart, ngraham, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D23461: Remove 'magic' filtering of recent apps

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: mart, ngraham, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D23461: Remove 'magic' filtering of recent apps

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> recentusagemodel.cpp:522
> +if (auto model = 
> qobject_cast(sourceModel())) {
> +model->setShowAllRecents(m_showAllRecents);
> +m_showAllRecents = recents;

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()

> recentusagemodel.h:43
>  Q_OBJECT
> +/* current kicker filters out recent apps that are also favorites, set 
> this to true to bypass that */
> +Q_PROPERTY(bool showAllRecents READ showAllRecents WRITE 
> setShowAllRecents NOTIFY showAllRecentsChanged)

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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: mart, ngraham, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D23461: Remove 'magic' filtering of recent apps

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 64818.
tcanabrava added a comment.


  - 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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23461?vs=64798=64818

BRANCH
  removeMagicFiltering

REVISION DETAIL
  https://phabricator.kde.org/D23461

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp
  applets/kicker/plugin/recentusagemodel.h

To: tcanabrava, hein, davidedmundson
Cc: mart, ngraham, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D23461: Remove 'magic' filtering of recent apps

[Marco Martin]

mart added a comment.


  but good to split the patch and push #1 first and discuss separatedly #2

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: mart, ngraham, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D23461: Remove 'magic' filtering of recent apps

[Marco Martin]

mart added a comment.


  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"

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: mart, ngraham, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D23461: Remove 'magic' filtering of recent apps

[Tomaz Canabrava]

tcanabrava added subscribers: ngraham, mart.
tcanabrava added a comment.


  In D23461#520710 , @davidedmundson 
wrote:
  
  > > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: mart, ngraham, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D23461: Remove 'magic' filtering of recent apps

[David Edmundson]

davidedmundson added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D23461: Remove 'magic' filtering of recent apps

[Tomaz Canabrava]

tcanabrava updated this revision to Diff 64798.
tcanabrava added a comment.


  - Revert "Remove 'magic' filtering of recent apps"
  - Remove filtering of recent apps that are in the favorites

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23461?vs=64654=64798

BRANCH
  removeMagicFiltering

REVISION DETAIL
  https://phabricator.kde.org/D23461

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp
  applets/kicker/plugin/recentusagemodel.h

To: tcanabrava, hein, davidedmundson
Cc: davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D23461: Remove 'magic' filtering of recent apps

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> recentusagemodel.cpp:98
> -
> -return (service && (!favoritesModel || 
> !favoritesModel->isFavorite(service->storageId(;
> -}

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.

2. 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D23461

To: tcanabrava, hein, davidedmundson
Cc: davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D23461: Remove 'magic' filtering of recent apps

[Tomaz Canabrava]

tcanabrava created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
tcanabrava requested review of this revision.

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

REPOSITORY
  R119 Plasma Desktop

BRANCH
  removeMagicFiltering

REVISION DETAIL
  https://phabricator.kde.org/D23461

AFFECTED FILES
  applets/kicker/plugin/recentusagemodel.cpp
  applets/kicker/plugin/recentusagemodel.h

To: tcanabrava
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart