panel/plugin: Fix settings storing/restoring #1745

palinek · 2022-03-14T10:54:50Z

Add emitting settingsChanged() into PluginSettings::loadFromCache() to
notify plugin/users, that settings were updated. Previously the signal
was probably emitted by the underlying object detecting the settings
file changed, but this is not the case any more (probably enhancement
in the underlying Qt's QSettings optimization).
Add PlutingSettings::storeToCache() for the plugin configure dialog to
be able to store to cache on closing the dialog -> after re-showing or
showing freshly created one, we expect to be able to reset to the
values shown initialy in this dialog.
Delete plugin's config dialog before deleting the plugin object
itself, as the config dialog can rely/use other plugin objects and
deleting/closing the dialog late after plugin object destroyed can
cause problems (invalid memory access).

tsujan · 2022-03-14T11:05:39Z

This isn't needed. It complicates the code unnecessarily. Please read my replies at #1744

tsujan · 2022-03-14T11:09:22Z

panel/lxqtpanelpluginconfigdialog.cpp

+    mSettings.storeToCache();
+    return QDialog::closeEvent(event);
+}
+


We don't want only to save settings. We also want to reflect the changes that the Rest button causes in the GUI. There's no guarantee that it happens. Each plugin needs its own handling and that makes this change redundant.

We also want to reflect the changes that the Rest button causes in the GUI.

The Reset button just restores the values from cache (pre-existing) into the settings object and calls locadSettings() which must each plugin's config dialog implement. And that's exactly, what you described, it should do.

Apart from not being needed, this is also at odd with the on-the-fly applying of settings change.

@palinek, please forget about what I wrote at #1741 (comment). As I said at #1741 (comment), I remembered everything later.

Apart from not being needed, this is also at odd with the on-the-fly applying of settings change.

?
Aren't you misreading the code? This does:

when the config dialog is closed, the current (and applied) settings are stored into "old values cache"

when the config dialog is opened next time, the reset button will be (optionally) resetting to values "from previous close" (or from the plugin creation if no config showed yet)

The close logic does nothing to the currently used configuration values.

OK but it isn't needed. We don't need to be worried about the cache; it'll be correct if Reset does its job.

- Add emitting settingsChanged() into PluginSettings::loadFromCache() to notify plugin/users, that settings were updated. Previously the signal was probably emitted by the underlying object detecting the settings file changed, but this is not the case any more (probably enhancement in the underlying Qt's QSettings optimization). - Add PlutingSettings::storeToCache() for the plugin configure dialog to be able to store to cache on closing the dialog -> after re-showing or showing freshly created one, we expect to be able to reset to the values shown initialy in this dialog. - Delete plugin's config dialog before deleting the plugin object itself, as the config dialog can rely/use other plugin objects and deleting/closing the dialog late after plugin object destroyed can cause problems (invalid memory access).

palinek · 2022-03-14T12:15:02Z

Updated... re-added emitting the settingsChanged() again... as this does the trick in case lxqt-panel is used with "default" configuration (no configuration file specified in command line options).

palinek · 2022-03-14T12:34:07Z

Heh.... after next testing -> the settings cache is messing up the config file. There is some deeper problem, here is the difference between pre-existing config file and after the reset button is pressed:

--- ../test_panel.conf  2022-03-14 13:32:47.044289979 +0100
+++ ../test1.conf       2022-03-14 13:32:10.396083054 +0100
@@ -4,8 +4,35 @@
 panels=panelTest
 
 [mainmenuTest]
+__userfile__=true
 alignment=Left
-ownIcon=false
+iconTheme=
+mainmenuTest\alignment=Left
+mainmenuTest\ownIcon=true
+mainmenuTest\showText=true
+mainmenuTest\type=mainmenu
+ownIcon=true
+panelTest\alignment=0
+panelTest\animation-duration=500
+panelTest\background-color=@Variant(\0\0\0\x43\0\xff\xff\0\0\0\0\0\0\0\0)
+panelTest\background-image=
+panelTest\desktop=0
+panelTest\font-color=@Variant(\0\0\0\x43\0\xff\xff\0\0\0\0\0\0\0\0)
+panelTest\hidable=false
+panelTest\hide-on-overlap=false
+panelTest\iconSize=32
+panelTest\lineCount=2
+panelTest\lockPanel=false
+panelTest\opacity=100
+panelTest\panelSize=80
+panelTest\plugins=mainmenuTest
+panelTest\position=Right
+panelTest\reserve-space=false
+panelTest\show-delay=500
+panelTest\visible-margin=false
+panelTest\width=80
+panelTest\width-percent=true
+panels=panelTest
 showText=true
 type=mainmenu

tsujan · 2022-03-14T12:46:19Z

here is the difference between pre-existing config file and after the reset button is pressed

Yes, there is a problem, without this patch too.

tsujan · 2022-03-14T12:49:59Z

The problem doesn't affect the functionality but makes a mess in ~/.config/lxqt/panel.conf.

tsujan · 2022-03-14T12:57:34Z

It's in PluginSettings::loadFromCache().

tsujan · 2022-03-14T13:54:55Z

The bug was in liblxqt. → lxqt/liblxqt#306

Without calling the beginGroup() on parent settings object, we got the cache object constructed with all keys in the parent object (not only those belonging to our group). Then doing loadFromCache() flushed all keys from config and messed the configuration file.

palinek · 2022-03-14T15:05:30Z

The bug was in liblxqt. → lxqt/liblxqt#306

Not only there... also in the PluginSettingsPrivate c-tor. It is fixed here.

tsujan · 2022-03-14T15:07:38Z

How? I can't reproduce the bug with lxqt/liblxqt#306 — and without this patch.

tsujan · 2022-03-14T15:10:32Z

The logic was very simple. SettingsCache::loadFromSettings needed to clear the cache first. Nothing more was needed (sorry that I repeat "not needed" but I mean it).

palinek · 2022-03-14T15:11:06Z

Yes, because there is the PluginSettings::sync() called between PluginSettings creation and PluginSettings::locadFromCache() - this reloads the cache. But for future usages there was a "sleeping" bug waiting...

tsujan · 2022-03-14T15:13:43Z

But for future usages there was a "sleeping" bug waiting...

Could you please be more specific about it? Which "sleeping bug"?

palinek · 2022-03-14T15:21:19Z

Try to create PluginSettings object, call PluginSettings::loadFromCache(). This should make no difference in the underneath config file. But the SettingsCache is constructed wrongly in PluginSettings construction time (there are all the keys in from the config file stored into the cache), which will result in flushing all config keys under our plugin section ...

tsujan · 2022-03-14T15:24:24Z

which will result in flushing all config keys under our plugin section

Yes, but

1, It has no effect (after lxqt/liblxqt#306) and
2. Why should loading from cache save settings? After all, it's just about loading. Also, in a future plugin, the coder may not want to save settings at this stage, for whatever reason, e.g., he may want/need to add Apply and OK buttons.

palinek · 2022-03-14T15:28:08Z

Why should loading from cache save settings?

That's the only point of the "cache". Restore the previously stored configuration snapshot.

palinek · 2022-03-14T15:29:12Z

1, It has no effect (after lxqt/liblxqt#306) and

Yes, because there is a sync() call luckily in between (as I said previously).

tsujan · 2022-03-14T15:34:54Z

Restore the previously stored configuration snapshot.

If there's no OK/Apply button, yes, because the user knows settings should be applied on-the-fly. With OK and Apply buttons, we don't want to save anything before pressing them. Are we prohibiting the usage of OK/Apply buttons here? Sorry if I've misread the code — I'm very busy with other codes now — but I ask because I want to be sure.

palinek · 2022-03-14T15:38:10Z

With OK and Apply buttons, we don't want to save anything before pressing them. Are we prohibiting the usage of OK/Apply buttons here?

No we're not prohibiting anything, but encouraging the unified on-they-fly updating. If the plugin wants to force ok/apply/cancel it needs to implement it by itself.

tsujan · 2022-03-14T15:46:04Z

Yes, because there is a sync() call luckily in between (as I said previously).

Sorry, however I look at it, I can't find a problem with sync(). Maybe I should come to this later...

tsujan · 2022-03-14T16:55:57Z

OK, coming back to this — free from other codes.

It doesn't prevent a "sleeping bug" but fixes an existing bug, which isn't related to what is fixed in lxqt/liblxqt#306. Previously (a few hours ago), it also worked around the problem that's now fixed in lxqt/liblxqt#306. That was the source of my confusion: the workaround isn't needed anymore but the fix is.

What does it fix? The settings cache might not be correct the second time a config dialog was opened. This patch guarantees a correct cache. This wasn't a future bug but an existing one ;)

It addition, it discourages Apply/OK buttons (I don't like that but it's beside the point). Three changes in a single PR (actually 4)!

Feel free to merge it as it is. I'll need it to be merged for fixing #1741 and changing its previous fixes according to it (perhaps, in a single PR).

palinek · 2022-03-14T17:02:32Z

Three changes in a single PR (actually 4)!

I know, this is a messy PR (shame on me :) ).... I was adding commits here as I was diving deeper into it and finding smelly pieces of code.

tsujan · 2022-03-14T17:05:23Z

It's OK. I'll follow you in future ;) Who has time for several PRs?

palinek · 2022-03-14T17:08:01Z

Should I wait for stefonarch to test this before merging?

tsujan · 2022-03-14T17:09:58Z

Should I wait for stefonarch to test this before merging?

I don't think he'll find a difference, because of the PRs that have been already merged (and I should change them after this is merged).

palinek · 2022-03-14T17:14:07Z

OK... merged. Thanks for reviewing.

stefonarch · 2022-03-14T17:51:54Z

i've not followed this, but looking at my panel,conf I wonder why it has 6000 lines and a size of 290kb.

tsujan · 2022-03-14T17:58:49Z

I wonder why it has 6000 lines and a size of 290kb.

Because of the bug that's fixed in lxqt/liblxqt#306. Fortunately, most users didn't notice it.

EDIT: Or one can say, "Unfortunately....".

stefonarch · 2022-03-14T18:01:09Z

I always was wondering about this file, I think it kept also stuff from testing panels but looking at it is impossible.
I think I'll start from scratch, saving custumcommands settings.

tsujan · 2022-03-14T18:05:04Z

I'd seen the problem before but, because of it, I backed up my panel's conf file in its best state. That, in turn, made me forget about the problem (I restored the backup without taking a look at the config file).

stefonarch · 2022-03-14T18:26:13Z

Reconfigured all, now it makes sense looking at it. Excerpt from the old file:

mount2\type=mount
mount2\volume\alignment=Right
mount2\volume\device=0
mount2\volume\type=volume
mount2\worldclock\alignment=Right
mount2\worldclock\autoRotate=true
mount2\worldclock\customFormat="dddd, d MMM |<b><big> H:mm</big> </b>"
mount2\worldclock\dateFormatType=custom
mount2\worldclock\dateLongNames=false
mount2\worldclock\datePadDay=false
mount2\worldclock\datePosition=below
mount2\worldclock\dateShowDoW=true
mount2\worldclock\dateShowYear=false
mount2\worldclock\defaultTimeZone=Europe/Rome

``

tsujan · 2022-03-14T18:30:29Z

Yes, that's prevented both in lxqt/liblxqt#306 and here. The main usefulness of this PR was elsewhere.

tsujan · 2022-03-14T18:34:08Z

It's hard to describe but, in short, the PR guarantees remembering of the old settings the next time you open the config dialog (of Taskbar). The rest isn't so visible to the user.

palinek mentioned this pull request Mar 14, 2022

The problem with Reset button #1741

Closed

tsujan reviewed Mar 14, 2022

View reviewed changes

palinek force-pushed the config-reset branch from 7114426 to 5a669b1 Compare March 14, 2022 11:23

palinek force-pushed the config-reset branch from 5a669b1 to b62ed29 Compare March 14, 2022 12:14

palinek force-pushed the config-reset branch from 535d9e0 to f3580d1 Compare March 14, 2022 13:28

palinek force-pushed the config-reset branch from f3580d1 to 51b3132 Compare March 14, 2022 14:45

taskbar: Use "static" last resort default values

56efcad

tsujan approved these changes Mar 14, 2022

View reviewed changes

palinek merged commit b5aa597 into master Mar 14, 2022

palinek deleted the config-reset branch March 14, 2022 17:13

tsujan mentioned this pull request Mar 14, 2022

Fixed Reset button in Clock config dialog #1744

Closed

panel/plugin: Fix settings storing/restoring #1745

panel/plugin: Fix settings storing/restoring #1745

Conversation

palinek commented Mar 14, 2022 • edited Loading

tsujan commented Mar 14, 2022

tsujan Mar 14, 2022

Choose a reason for hiding this comment

palinek Mar 14, 2022

Choose a reason for hiding this comment

tsujan Mar 14, 2022

Choose a reason for hiding this comment

tsujan Mar 14, 2022

Choose a reason for hiding this comment

palinek Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

tsujan Mar 14, 2022

Choose a reason for hiding this comment

palinek commented Mar 14, 2022

palinek commented Mar 14, 2022

tsujan commented Mar 14, 2022 • edited Loading

tsujan commented Mar 14, 2022

tsujan commented Mar 14, 2022

tsujan commented Mar 14, 2022

palinek commented Mar 14, 2022

tsujan commented Mar 14, 2022

tsujan commented Mar 14, 2022

palinek commented Mar 14, 2022

tsujan commented Mar 14, 2022

palinek commented Mar 14, 2022

tsujan commented Mar 14, 2022

palinek commented Mar 14, 2022

palinek commented Mar 14, 2022 • edited Loading

tsujan commented Mar 14, 2022 • edited Loading

palinek commented Mar 14, 2022

tsujan commented Mar 14, 2022

tsujan commented Mar 14, 2022 • edited Loading

palinek commented Mar 14, 2022 • edited Loading

tsujan commented Mar 14, 2022

palinek commented Mar 14, 2022

tsujan commented Mar 14, 2022

palinek commented Mar 14, 2022

stefonarch commented Mar 14, 2022

tsujan commented Mar 14, 2022 • edited Loading

stefonarch commented Mar 14, 2022 • edited Loading

tsujan commented Mar 14, 2022

stefonarch commented Mar 14, 2022

tsujan commented Mar 14, 2022

tsujan commented Mar 14, 2022

palinek commented Mar 14, 2022 •

edited

Loading

palinek Mar 14, 2022 •

edited

Loading

tsujan commented Mar 14, 2022 •

edited

Loading

palinek commented Mar 14, 2022 •

edited

Loading

tsujan commented Mar 14, 2022 •

edited

Loading

tsujan commented Mar 14, 2022 •

edited

Loading

palinek commented Mar 14, 2022 •

edited

Loading

tsujan commented Mar 14, 2022 •

edited

Loading

stefonarch commented Mar 14, 2022 •

edited

Loading