kde-plasma/kscreen: Add fix for multiscreen config persistance

Reported-by: gorg86 (forums.gentoo.org)

https://forums.gentoo.org/viewtopic-t-1046956.html

Upstream bug: https://bugs.kde.org/show_bug.cgi?id=346961

Package-Manager: portage-2.2.28
This commit is contained in:
Andreas Sturmlechner
2016-06-23 19:16:16 +02:00
committed by Michael Palimaka
parent b8abf9edc6
commit e8ecf8e327
2 changed files with 199 additions and 0 deletions

View File

@@ -0,0 +1,159 @@
From: Sebastian Kügler <sebas@kde.org>
Date: Wed, 01 Jun 2016 14:54:16 +0000
Subject: address race condition around setoperation
X-Git-Tag: v5.6.95
X-Git-Url: http://quickgit.kde.org/?p=kscreen.git&a=commitdiff&h=17199d32f292f7c44eb8cdce5b35396d3bd19eb8
---
address race condition around setoperation
Summary:
Use a timer to avoid catching configChanged signals after we set
changes.
The long version:
TL;DR: We have a race condition when the kscreen daemon starts. It looks
up a known config, then applies it and subsequently resaves the config.
The same happens on config changes, it writes, then re-reads and then
re-writes the config change.
I've managed to prevent this from happening by adding a timer that does
avoids saving the config as a direct reaction to our own config changes.
So what happens on kded5 startup after loading the kscreen2 module:
- the kscreen config is requested and received
- the kscreen daemon (KD) looks into its config directory for a suitable
config file
(a config file is identified by a combined hash of all screen
attached, so unique per connected set of outputs)
- KD usually finds a config
- KD ignores configChanged events before it starts ...
- a KScreen::SetConfigOperation to apply the "known config"
- SetConfigOperation returns after a while (say 100ms later)
- we re-enable the change monitor
- we receive a configChanged signal
- we save the new config (usually to the existing config file)
I don't think this behavior is desirable. I don't see a reason why the
daemon should save its config right after applying it. I think this
causes more problems than we want, since the startup may overwrite the
user's config. This behavior seems to be desired by the code in KD, it's
already blocking configChanged signals during the SetOperation (which,
to be honest may result in nightmarish behavior in any way, so it might
be a kludge which aims too short).
From libkscreen perspective, SetConfigOperation::finished cannot
guarantee that all configChanged signals are already fired and that it's
safe to watch for new, independent changes now. At least on X11, we
simply don't know, and what we can do is wait a bit and cross fingers
that we're not catching our own noise. The changed signal *may* come
from a re-request of the edid information, but this is a bit hard to
track down, and not too useful, anyway, since changed Edid may affect a
large number of a screen's properties.
In the Wayland backend, that's a different story and we can prevent this
behavior at an earlier stage, so this timer is "probably not needed" (I
haven't tested that).
This effectively prevents KD from catching reactions to its own changes
and does not trigger saving the config file on every login. It still
reacts to changes from libkscreen, but will avoid re-saving the config a
few times. The timer may not be the neatest of solutions for this, but
it does help narrowing down the problem and may be a last resort action.
Most importantly, it avoids the re-writing of the config on startup and
plugging/unplugging a monitor effectively.
The timer value of 100ms is also used in kwin, which should make the
behavior (which is no problem in kwin) more solid.
CCBUG:346961
CCBUG:358011
Reviewers: graesslin
Reviewed By: graesslin
Subscribers: plasma-devel, #plasma
Tags: #plasma
Differential Revision: https://phabricator.kde.org/D1730
---
--- a/kded/daemon.cpp
+++ b/kded/daemon.cpp
@@ -23,6 +23,7 @@
#include "kscreenadaptor.h"
#include "debug.h"
+#include <QElapsedTimer>
#include <QTimer>
#include <QAction>
#include <QShortcut>
@@ -52,6 +53,7 @@
, m_buttonTimer(new QTimer())
, m_saveTimer(new QTimer())
, m_lidClosedTimer(new QTimer())
+ , m_changeBlockTimer(new QElapsedTimer())
{
QMetaObject::invokeMethod(this, "requestConfig", Qt::QueuedConnection);
@@ -82,6 +84,7 @@
delete m_saveTimer;
delete m_buttonTimer;
delete m_lidClosedTimer;
+ delete m_changeBlockTimer;
Generator::destroy();
Device::destroy();
@@ -112,7 +115,6 @@
m_lidClosedTimer->setInterval(1000);
m_lidClosedTimer->setSingleShot(true);
connect(m_lidClosedTimer, &QTimer::timeout, this, &KScreenDaemon::lidClosedTimeout);
-
connect(Device::self(), &Device::lidClosedChanged, this, &KScreenDaemon::lidClosedChanged);
connect(Device::self(), &Device::resumingFromSuspend,
@@ -145,6 +147,9 @@
connect(new KScreen::SetConfigOperation(config), &KScreen::SetConfigOperation::finished,
[&]() {
qCDebug(KSCREEN_KDED) << "Config applied";
+ // We enable monitoring already, but we will ignore the first signals that come
+ // in the next 100ms, since these are likely our own changes still flushing out
+ m_changeBlockTimer->start();
setMonitorForChanges(true);
});
}
@@ -182,6 +187,12 @@
void KScreenDaemon::configChanged()
{
+ if (m_changeBlockTimer->isValid() && !m_changeBlockTimer->hasExpired(100)) {
+ m_changeBlockTimer->start();
+ qCDebug(KSCREEN_KDED) << "Change detected, but ignoring since it's our own noise";
+ return;
+ }
+ m_changeBlockTimer->invalidate();
qCDebug(KSCREEN_KDED) << "Change detected";
// Reset timer, delay the writeback
m_saveTimer->start();
--- a/kded/daemon.h
+++ b/kded/daemon.h
@@ -27,6 +27,7 @@
#include "generator.h"
+class QElapsedTimer;
class QTimer;
namespace KScreen
@@ -79,6 +80,7 @@
QTimer* m_buttonTimer;
QTimer* m_saveTimer;
QTimer* m_lidClosedTimer;
+ QElapsedTimer* m_changeBlockTimer;
};
#endif /*KSCREN_DAEMON_H*/

View File

@@ -0,0 +1,40 @@
# Copyright 1999-2016 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2
# $Id$
EAPI=6
KDE_TEST="forceoptional"
inherit kde5
DESCRIPTION="KDE Plasma screen management"
HOMEPAGE="https://projects.kde.org/projects/extragear/base/kscreen"
KEYWORDS="~amd64 ~arm ~x86"
IUSE=""
DEPEND="
$(add_frameworks_dep kconfig)
$(add_frameworks_dep kconfigwidgets)
$(add_frameworks_dep kcoreaddons)
$(add_frameworks_dep kdbusaddons)
$(add_frameworks_dep kglobalaccel)
$(add_frameworks_dep ki18n)
$(add_frameworks_dep kwidgetsaddons)
$(add_frameworks_dep kxmlgui)
$(add_plasma_dep libkscreen)
$(add_qt_dep qtdbus)
$(add_qt_dep qtdeclarative 'widgets')
$(add_qt_dep qtgui)
$(add_qt_dep qtwidgets)
"
RDEPEND="${DEPEND}
$(add_plasma_dep kde-cli-tools)
$(add_qt_dep qtgraphicaleffects)
!kde-misc/kscreen
"
PATCHES=( "${FILESDIR}/${P}-config-fix.patch" )
# bug #580440, last checked 5.6.3
RESTRICT="test"