Support multiple values in XDG_CURRENT_DESKTOP
ClosedPublic

Authored by dfaure on Jan 24 2020, 6:46 PM.

Details

Summary

The Desktop Entry spec says "If $XDG_CURRENT_DESKTOP is set then it contains a colon-separated list of strings."
I found this bug (in a 2014 commit of mine) by simply reading this code and wondering why there was a QStringList.

Test Plan

Unittest updated, fails without the fix

Diff Detail

Repository
R309 KService
Branch
xdg_current_deskto
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21643
Build 21661: arc lint + arc unit
dfaure created this revision.Jan 24 2020, 6:46 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 24 2020, 6:46 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Jan 24 2020, 6:46 PM
apol added inline comments.Jan 24 2020, 10:46 PM
src/services/kservice.cpp
658–660

You can use QVector<QStringRef> here, it would save us constructing a bunch of strings for no reason. Maybe not in the general case of everyone having just the one desktop though.

dfaure updated this revision to Diff 74337.Jan 24 2020, 11:31 PM

use splitRef and QVector<QStringRef>

apol accepted this revision.Jan 26 2020, 1:05 PM

The patch looks good to me.

src/services/kservice.cpp
658–660

This adds a "KDE" allocation on every which isn't ideal either. I don't know, maybe it doesn't matter either way...

This revision is now accepted and ready to land.Jan 26 2020, 1:05 PM
dfaure added inline comments.Jan 26 2020, 5:21 PM
src/services/kservice.cpp
658–660

Well, it's a QStringLiteral, AFAIK the QString just points to pre-allocated UTF-16 data. Not sure if it's zero-allocation but it's pretty cheap.

dfaure closed this revision.Jan 26 2020, 5:21 PM