broulik added a comment.
Nice findings, feel free to ignore the stylistic changes I commented, except
the `qDeleteAll` one, and do unrelated further cleanup in a separate patch
INLINE COMMENTS
> waylandoutput.cpp:64
> + auto it = std::find(m_modeIdMap.cbegin(), m_modeIdMap.cend(),
> kwaylandmodeid);
> + if (it == m_modeIdMap.cend()) {
> qCWarning(KSCREEN_WAYLAND) << "Invalid kwayland mode id:" <<
> kwaylandmodeid << m_modeIdMap;
I think we typically use `const...()` instead of `c...()` but since this method
is `const`, shouldn't be neccessary to begin with
> qscreenconfig.cpp:47
> {
> - foreach(auto output, m_outputMap.values()) {
> + foreach(auto output, m_outputMap) {
> delete output;
`qDeleteAll(m_outputMap);`
> qscreenconfig.cpp:62
> {
> QList<int> ids;
> + foreach(auto output, m_outputMap) {
This seems unused
> output.cpp:101
> {
> - if (before.keys() != after.keys()) {
> + if (before.size() != after.size()) {
> return false;
`count()`
> waylandconfigreader.cpp:117
> + if (mode.contains(QStringLiteral("refreshRate"))) {
> m0.refreshRate =
> qRound(mode[QStringLiteral("refreshRate")].toReal() * 1000); // config has it
> in Hz
> }
We do a double lookup here, `contains()` and then `operator[]` afterwards,
should be combined to a single `find()`
REPOSITORY
R110 KScreen Library
REVISION DETAIL
https://phabricator.kde.org/D15946
To: volkov, #plasma
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
