Differential D28760

KSettings::Dialog: avoid duplicate entries due cascading $XDG_DATA_DIRS
ClosedPublic
Actions

Authored by dfaure on Apr 11 2020, 5:50 PM.

Details

Reviewers
apol
broulik
davidedmundson
kossebau
svuorela
Commits
R295:5505f6ab6f5a: KSettings::Dialog: avoid duplicate entries due cascading $XDG_DATA_DIRS
Summary

I had every toplevel entry doubled in kontact's configuration dialog
due to $XDG_DATA_DIRS=<myPrefix>/share:/usr/share
which led to parsing both
<myPrefix>/share/kontact/ksettingsdialog/knotes.setdlg
and
/usr/share/kontact/ksettingsdialog/knotes.setdlg

Diff Detail

Repository
R295 KCMUtils
Branch
fix_duplicates
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25087
Build 25105: arc lint + arc unit
dfaure created this revision.Apr 11 2020, 5:50 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 11 2020, 5:50 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Apr 11 2020, 5:50 PM
pino added a subscriber: pino.Apr 11 2020, 5:57 PM

Why not instead use a QMap<QString /* relative filename */, QString /* absolute path */ > to collect the files? This way you wouldn't need the double QStandardPaths lookup.

dfaure updated this revision to Diff 79892.Apr 11 2020, 10:16 PM
dfaure edited the summary of this revision. (Show Details)
dfaure removed a subscriber: pino.

Use QMap, thanks Pino for the idea.

svuorela accepted this revision.May 10 2020, 7:03 PM
svuorela added inline comments.
src/ksettings/dialog.cpp
317

I think I read once that whenever you used a ordered map over an unordered map, you need to justify it by talking to your manager about it. But that's also a bit from the bucket of nitpickery unless we are in a hot codepath.

326

nitpickery. but range based for and qAsConst on container?

This revision is now accepted and ready to land.May 10 2020, 7:03 PM
dfaure added inline comments.May 10 2020, 8:03 PM
src/ksettings/dialog.cpp
317

Interesting. This makes it sound like "ordering" is a feature that costs extra.
But my understanding is that the bookkeeping required for a hash table, is what costs extra -- for small quantities of data, as is the case here.
Instanciating nodes and making a few '<' comparisons are less work than creating buckets, and hashing (entire) strings.

Anyhow, for 5-10 items nothing makes a difference.

326

For a QMap? I think this iterates over pairs, so it only looks nice with C++17 structured bindings.
(or if it doesn't, then that's an incompatibility with std::map, awful).

dfaure closed this revision.May 10 2020, 8:03 PM

Revision Contents
Changeset List

PathPackages
Msrc/ksettings/dialog.cpp (8 lines)

Diff 79892

View Options

src/ksettings/dialog.cpp

Loading...