parse the desktop file 2 times
ClosedPublic

Authored by mart on Jan 11 2017, 12:46 PM.

Details

Summary

Search for the ServiceTypes key in the desktop file before
parsing it "for real", because how it's parsed depends from
the service type definition, so if the servicetype is defined
at the bottom of the file or after keys dependent from the type,
those keys would be parsed incorrectly

Test Plan

things using plugins like plasmashell still start correctly,
dropping on the desktop a text file now proposes to create a notes widget.
The notes widget has ServiceTypes defined *after* X-Plasma-DropMimetypes
which is a stringlist, that would be misinterpreted as a string
otherwise

Diff Detail

Repository
R244 KCoreAddons
Branch
phab/prse2servicetypes
Lint
No Linters Available
Unit
No Unit Test Coverage
mart updated this revision to Diff 10039.Jan 11 2017, 12:46 PM
mart retitled this revision from to parse the desktop file 2 times.
mart updated this object.
mart edited the test plan for this revision. (Show Details)
mart added a reviewer: Plasma.
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptJan 11 2017, 12:46 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
mart added inline comments.
src/lib/plugin/desktopfileparser.cpp
237

maybe instead a bool it could have error codes to more easily assure the behavior is 100% the same as before

davidedmundson added inline comments.Jan 11 2017, 1:31 PM
src/lib/plugin/desktopfileparser.cpp
537

We don't need to close

readUntilDesktopEntryGroup()
auto pos = dr.pos();
while() {
}
dr.seek(pos);
while() {
}

Also you can remove the lines

} else if (key == QByteArrayLiteral("X-KDE-ServiceTypes") || key == QByteArrayLiteral("ServiceTypes")) {
    const auto services = deserializeList(value);
  • for(const auto &service : services) {
  • // some .desktop files still use the legacy ServiceTypes= key
  • QString fileName = service.toLower().replace(QLatin1Char('/'), QLatin1Char('-'))+QStringLiteral(".desktop");
  • serviceTypes.addFile(fileName);
  • }

from inside DesktopFileParser::convertToJson as we'll hae already done that

(I had written https://paste.kde.org/pipfub7bu last night, but wanted to split that tokenisation out that you've done)

mart updated this revision to Diff 10044.Jan 11 2017, 1:55 PM
mart edited edge metadata.
  • don't close/reopen the file
mart updated this revision to Diff 10045.Jan 11 2017, 2:02 PM
mart marked an inline comment as done.
  • don't load servicetypes a second time
apol added a subscriber: apol.Jan 11 2017, 2:18 PM

How about adding a test?

Other than that, the patch looks good.

It's horrible that we need to do so, but I guess it's the price of backwards compatibility. There's the possibility of doing the processing in two steps (desktop to pairs, pairs to json), but we agreed that this code shouldn't be optimized anyway (as it's transitional and what we support is json).

apol accepted this revision.Jan 11 2017, 2:59 PM
apol added a reviewer: apol.
This revision is now accepted and ready to land.Jan 11 2017, 2:59 PM
mart updated this revision to Diff 10054.Jan 11 2017, 3:04 PM
mart edited edge metadata.
  • add an autotest for a list item before the servicetypes definition
This revision was automatically updated to reflect the committed changes.