Differential D10890

Don't skip certain recent documents in kicker and taskmanager
ClosedPublic
Actions

Authored by aleksejshilin on Feb 27 2018, 3:21 PM.

Details

Reviewers
broulik
hein
Group Reviewers
Plasma
Commits
R119:ca96f850efbf: Don't skip certain recent documents in kicker and taskmanager
R120:e64f95feb2fe: Don't skip certain recent documents in kicker and taskmanager
Summary

Due to KRecentDocument::add() bug, application name '<app>' could
be stored for recent documents instead of its desktop entry name
'org.kde.<app>'. This issue mostly affects items opened from apps
themselves, while items opened from e.g. Dolphin are not affected.
(The bug is fixed by D10863, but we depend on Frameworks 5.42 which
doesn't include it.)

Kicker and taskmanager used to query for '<app>' agent only, thus
excluding items with correct 'org.kde.<app>' agent.

This commit makes kicker and taskmanager query for both '<app>' and
'org.kde.<app>' agents in order to get all recent documents for the
application.

Test Plan
  1. Create 'test1' and 'test2' text files.
  2. Open 'test1' in Kate from Dolphin.
  3. Open 'test2' from Kate itself.
  4. Right-click Kate in Kicker/Kickoff and Task Manager and check that both files are present there.

Diff Detail

Repository
R119 Plasma Desktop
Branch
prepare_for_krecentdocument_fix
Lint
No Linters Available
Unit
No Unit Test Coverage
aleksejshilin created this revision.Feb 27 2018, 3:21 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 27 2018, 3:21 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
aleksejshilin requested review of this revision.Feb 27 2018, 3:21 PM
anthonyfieroni added a subscriber: anthonyfieroni.Feb 28 2018, 7:37 AM

So why you called hack, why no just add conditional compilation against 5.44

aleksejshilin added a comment.Mar 2 2018, 11:50 AM
In D10890#215479, @anthonyfieroni wrote:

So why you called hack, why no just add conditional compilation against 5.44

Alright, I re-read the summary and admit that it's somewhat confusing. Sorry for that. I'll try to make it clear:

  • This is a workaround for a bug when items opened from e.g. Dolphin don't show up in kicker's and taskmanager's application context menus. It is caused by:
    1. KIO KRecentDocument bug, which led to application name (<app>) being stored in recent documents DB instead of its desktop entry name (org.kde.<app>). It mostly affects items which are opened from applications themselves. (The bug is fixed in D10863, but not landed yet.)
    2. Kicker and taskmanager cutting out 'org.kde.' prefix (in order to work around the bug above, I believe).
    3. Items opened with KRun (which is what Dolphin is using) having correct org.kde.<app> record stored in the DB. As a result, they don't match recent documents query by kicker and taskmanager and hence don't show up there.
  • The workaround is needed for Plasma 5.12 only as it depends on Frameworks >= 5.42 i.e. we don't know if the version we're running on does contain D10863.
    • If it doesn't, we need to query for both <app> and org.kde.<app>.
    • If it does, we only need to query for org.kde.<app>. However, querying for both will do no harm either.
  • We need to fix Plasma first because otherwise kicker's and taskmanager's recent documents feature will break completely when Frameworks containing D10863 is released.

So the plan (as discussed with @broulik on IRC) is to work around the issue in Plasma/5.12, then land D10863, then remove the hack with cutting out 'org.kde.' in master (I'll do it in a separate revision).

aleksejshilin updated this revision to Diff 28397.Mar 2 2018, 11:53 AM

Rebase onto Plasma/5.12

aleksejshilin updated this revision to Diff 28398.Mar 2 2018, 12:16 PM

Diff against Plasma/5.12

aleksejshilin updated this revision to Diff 28626.Mar 4 2018, 7:55 PM
  • Clean up and better comment the code
aleksejshilin retitled this revision from Prepare for KIO KRecentDocument fix (D10863) to Don't skip certain recent documents in kicker and taskmanager.Mar 4 2018, 7:55 PM
aleksejshilin edited the summary of this revision. (Show Details)
aleksejshilin edited the test plan for this revision. (Show Details)
anthonyfieroni added a comment.Mar 4 2018, 8:07 PM

It looks good to me

aleksejshilin updated this revision to Diff 28639.Mar 4 2018, 9:11 PM
  • Make sure to forget all documents, too
hein added a comment.Mar 5 2018, 6:19 AM

Looks good to me, Kai?

broulik added a comment.Mar 5 2018, 8:31 AM

lgtm

applets/kicker/plugin/actionlist.cpp
344

I would prefer mid(8)

aleksejshilin updated this revision to Diff 28681.Mar 5 2018, 9:39 AM
aleksejshilin marked an inline comment as done.
  • Use QString::mid()
aleksejshilin edited the test plan for this revision. (Show Details)Mar 5 2018, 9:45 AM
hein accepted this revision.Mar 8 2018, 7:48 AM

lgtm

This revision is now accepted and ready to land.Mar 8 2018, 7:48 AM
Closed by commit R119:ca96f850efbf: Don't skip certain recent documents in kicker and taskmanager (authored by aleksejshilin). · Explain WhyMar 8 2018, 11:42 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28398

View Options

applets/kicker/plugin/actionlist.cpp

Loading...
View Options

applets/taskmanager/plugin/backend.cpp

Loading...