[Differential] [Commented On] D3166: make sure containment actions config is up to date
mart added a comment. In https://phabricator.kde.org/D3166#59071, @davidedmundson wrote: > It's still a bodge round broken user code. it is, but the thing is, that's the way is supported by the api right now, and there are users for it, so it has to work, regardless if it's quite ugly. i can't really see much other ways, either that or put a watcher on the config update... REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3166 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, davidedmundson, #plasma Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3166: make sure containment actions config is up to date
davidedmundson added a comment. It's still a bodge round broken user code. ConfigGroup.write() can't be expected to update anything on the fly, that's not how config works, and it's not how any of the rest of the scripting works. Config API even allows you to open a different file that might not even be part of plasmashell. Updating just this one weirdly specific action group on script closure makes no design sense. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3166 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, davidedmundson, #plasma Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3166: make sure containment actions config is up to date
mart added a comment. in this version, i'm rebuilding everything on scriptengine teardown, this should make work correctly even if one adds a new plugin, or removes one. REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3166 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, davidedmundson, #plasma Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3166: make sure containment actions config is up to date
mart added a comment. In https://phabricator.kde.org/D3166#58841, @davidedmundson wrote: > Also this script is making an optimistic assumption that in the script where you might be modding containment configs, you'd create a containment instance. That's not always true (like from the evaluate script DBus calls). yeah, if no containment wrapper instances exist, this may not get executed. I think it should probably be attempted when tearing down the whole scripting engine stuff instead. in order to make adding actions (or changing existing ones) working, a loop of Containment::setContainmentActions should be executed looping trought the config that just has been written > Maybe do this for 5.8, but you really need to expose a new method (or even Actions object) to do this properly - or to delay the loading. I would really like to avoid creating any new api with this qtscript stuff, given the state of qtscript, but I think is feasible to make the current manual config poking fully work. existing scripts in 5.8 has to work, so yeah. i don't see possible delaying the loading, as the desktop containment has to already exist by the time the script is executed (see the long story of fixes of scripting during the early 5.7) > BTW: The script in the bug report has a bug, it's only going to configure the actions on containment 0; even though you'll be reconfiguring the "correct" one. no, that group is not the containment id, it is the containment type, so [ActionPlugins][0] means plugins for desktop containments, [ActionPlugins][1] for panel containments (enum ContainmentType in plasma.h) so that script correctly changes actions for "desktop" containments REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3166 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma, davidedmundson Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
[Differential] [Commented On] D3166: make sure containment actions config is up to date
davidedmundson added a comment. > If the containment actions plugins have been configured in the startup script, their config must be reloaded when the script ends, That part makes sensebut why is this code in the containment destructor? REPOSITORY rPLASMAWORKSPACE Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D3166 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
