Summary: | When the slideshow plugin changes the wallpaper, the newly selected wallpaper is not updated in the configuration file or DBUS API | ||
---|---|---|---|
Product: | [Plasma] plasmashell | Reporter: | Oded Arbel <oded> |
Component: | Image Wallpaper | Assignee: | Plasma Bugs List <plasma-bugs> |
Status: | RESOLVED FIXED | ||
Severity: | minor | CC: | nate, notmart, qydwhotmail |
Priority: | NOR | Keywords: | qt6 |
Version: | master | ||
Target Milestone: | 1.0 | ||
Platform: | Other | ||
OS: | Linux | ||
Latest Commit: | https://invent.kde.org/plasma/plasma-workspace/-/commit/c940d214b794d0ca4d822aa40cb64199101e76cf | Version Fixed In: | 6.0 |
Sentry Crash Report: |
Description
Oded Arbel
2024-02-24 00:31:52 UTC
KConfigPropertyMapPrivate::writeConfigValue doesn't sync the config immediately (In reply to Fushan Wen from comment #1) > KConfigPropertyMapPrivate::writeConfigValue doesn't sync the config > immediately Can it? Should it? I understand performance reasons because disk IO sth sth, but: 1. Will it ever sync to the files? Is there an external (DBUS) API to request the sync? I failed to find one and failed to wait long enough for the sync to occur spontaneously, including when plasmashell is closing. 2. What about the data in the DBUS `org.kde.PlasmaShell.wallpaper response? it shouldn't be a performance issue to have that data available in memory for a DBUS request. I tried to do this horrible thing: diff --git a/wallpapers/image/plugin/imagebackend.cpp b/wallpapers/image/plugin/imagebackend.cpp index e8d51ba2bf..2ab21e1f54 100644 --- a/wallpapers/image/plugin/imagebackend.cpp +++ b/wallpapers/image/plugin/imagebackend.cpp @@ -23,6 +23,7 @@ #include <QStandardPaths> #include <KLocalizedString> +#include <KConfigPropertyMap> #include "model/imageproxymodel.h" #include "slidefiltermodel.h" @@ -189,6 +190,7 @@ void ImageBackend::saveCurrentWallpaper() } QMetaObject::invokeMethod(this, "writeImageConfig", Qt::QueuedConnection, Q_ARG(QString, m_image.toString())); + static_cast<KConfigPropertyMap*>(m_configMap.data())->writeConfig(); } QAbstractItemModel *ImageBackend::slideFilterModel() const And now it updates the configuration - but with the previous wallpaper image?!? (i.e. if showing a.jpeg, then saveCurrentWallpaper() is called to save "b.jpeg", after that the configuration DBUS and file will show "a.jpeg") This works better: diff --git a/wallpapers/image/plugin/imagebackend.cpp b/wallpapers/image/plugin/imagebackend.cpp index e8d51ba2bf..754fb7077b 100644 --- a/wallpapers/image/plugin/imagebackend.cpp +++ b/wallpapers/image/plugin/imagebackend.cpp @@ -21,10 +21,11 @@ #include <QMimeDatabase> #include <QScreen> #include <QStandardPaths> #include <KLocalizedString> +#include <KConfigPropertyMap> #include "model/imageproxymodel.h" #include "slidefiltermodel.h" #include "slidemodel.h" @@ -356,10 +357,13 @@ void ImageBackend::setConfigMap(QQmlPropertyMap *configMap) connect(m_configMap, &QQmlPropertyMap::valueChanged, this, [this](const QString &key, const QVariant & /* value */) { if (key == QStringLiteral("Image")) { Q_EMIT configMapChanged(); } + if (!m_configMap.isNull()) { + static_cast<KConfigPropertyMap*>(m_configMap.data())->writeConfig(); + } }); saveCurrentWallpaper(); } Still horrible, but at least gets the job done. As a general concept - if the API was sane - would that be acceptable? Depends on how much we want to prioritize avoiding writes to the config file, I guess. If the user does something silly like change their wallpaper every 5 seconds, we could end up doing a lot of writes. So it depends at least partially on how much we care about this use case. (In reply to Nate Graham from comment #4) > Depends on how much we want to prioritize avoiding writes to the config > file, I guess. If the user does something silly like change their wallpaper > every 5 seconds, we could end up doing a lot of writes. Is writing the config file every 5 seconds an issue? The config file is synced for a lot of other things - such as moving widgets. When someone is setting up a new widget on their desktop, the config file will be updated multiple times during that process. If the user does RMB -> next wallpaper, again and again - it is user interaction and updating the config file in response seems like an appropriate measure. > So it depends at least partially on how much we care about this use case. Well, obviously _I_ care about this use case 😅 I have a lot of wallpapers and a bunch of automation tools that help me manage them and they interact with the plasma shell using DBus and shell scripting - I'm not actually reading the config file, but both DBus and shell scripting use the same mechanism that - at least with the current code AFAIK - won't update unless we do `KConfigPropertyMap::writeConfig()`. Some of those tools are presented as desktop widgets, so when the wallpaper changes (due to either timer or actually doing "Next wallpaper") I'd expect the widget to reflect the current wallpaper. Sure, that's why I marked it as CONFIRMED. :) My point is that we need to consider as a part of this how to avoid excessive config file writes. In the worst-case scenario where the user has the wallpaper changing every 5 seconds and keeps the system on 24/7, that's over 17,000 writes daily--far more than the writes that take place when you move widgets around. So either we decide we care about optimizing that, or we decide we don't and tell people "don't change your wallpaper every 5 seconds". Maybe we should even reduce the minimum wallpaper transition time to 1 minute. (In reply to Nate Graham from comment #6) > In the worst-case scenario where the user has the wallpaper changing every 5 > seconds and keeps the system on 24/7, that's over 17,000 writes daily--far > more than the writes that take place when you move widgets around. I can add a simple heuristic that has a minimum time between writes and a burst budget, so with - lets say 1 minute and burst 10 - would let you make 10 writes quickly but on average not more than 1 write a minute. Shouldn't be more than a dozen LoC. > So either we decide we care about optimizing that, or we decide we don't and > tell people "don't change your wallpaper every 5 seconds". Maybe we should > even reduce the minimum wallpaper transition time to 1 minute. On MS-Windows you can't set the slideshow to more frequent than 1 minute, but most other desktops let you go down to 1 second - and I kind of don't want to degrade the current capabilities. Thinking about it a bit more - as I've mentioned, I don't actually care about the contents of the on-disk file, I just want the DBus APIs (direct responses and shell scripting) to see the changes. I wonder if there's another way to sync the changes to they are visible to the external APIs without actually opening files and moving bits to storage. Your plan sounds great! A possibly relevant merge request was started @ https://invent.kde.org/plasma/plasma-workspace/-/merge_requests/3987 I pushed an MR with what I was doing and I would appreciate any feedback. Git commit 37d6e6b5043cd2a44e19d668b113c8d2ebeedb73 by Fushan Wen. Committed on 09/03/2024 at 02:01. Pushed by fusionfuture into branch 'master'. shell: read wallpaper config from plugin instead of local file The local config is not always up-to-date. FIXED-IN: 6.0 M +11 -15 shell/shellcorona.cpp https://invent.kde.org/plasma/plasma-workspace/-/commit/37d6e6b5043cd2a44e19d668b113c8d2ebeedb73 Git commit c940d214b794d0ca4d822aa40cb64199101e76cf by Fushan Wen. Committed on 09/03/2024 at 02:22. Pushed by fusionfuture into branch 'Plasma/6.0'. shell: read wallpaper config from plugin instead of local file The local config is not always up-to-date. FIXED-IN: 6.0 (cherry picked from commit 37d6e6b5043cd2a44e19d668b113c8d2ebeedb73) M +11 -15 shell/shellcorona.cpp https://invent.kde.org/plasma/plasma-workspace/-/commit/c940d214b794d0ca4d822aa40cb64199101e76cf Thank you Fushan Wen. With the current version of plasma-workspace in Neon testing, I can see that the D-Bus API response for `org.kde.plasmashell /PlasmaShell org.kde.plasmashell.wallpaper` does indeed return the current wallpaper. My remaining concern if with the desktop scripting API (https://develop.kde.org/docs/plasma/scripting/). In Plasma 5 I could run something like this (using the interactive console, or the D-Bus API `org.kde.PlasmaShell.evaluateScript`): for (let d of desktopsForActivity('114ebbc8-f947-493d-bc5e-3762eab23c9a')) { if (d.screen != 1) continue; if (d.wallpaperPlugin != 'org.kde.slideshow') continue; // for (let h in d) // print("Arg: " + h + " => " + d[h] + "\n"); d.currentConfigGroup = ['Wallpaper',d.wallpaperPlugin,'General']; print(d.readConfig('Image') + "\n"); } And that would print the current wallpaper image selected by the slideshow. With Plasma 6, without force syncing the configuration files (as the slideshow plugin hack was doing), this scripting code prints the file name of some very old selected image (I think from the last log out, but I'm not sure). I wonder if there's a way to get the desktop scripting API to also see the configuration change. My current workaround - with the bug fix - is to extract the image file using the command `qdbus org.kde.plasmashell /PlasmaShell org.kde.PlasmaShell.wallpaper 1 | awk '$1=="Image:"{print$2}' | sed s,file://,,` The main problem with that is that it isn't activity aware - so I can only retrieve the current image file name for the current activity. It is OK for *most* of what I'm doing, but I think a solution that fixes desktop scripting will be a good idea. That indicates there is a deeper problem in plasmashell |