The problem with Reset button

[tsujan]

This issue may not be limited to the panel but it can be seen in the config dialogs of some of its plugins:

The Rest button doesn't do a complete job everywhere. Sometimes, it doesn't change the states of widgets; sometimes it doesn't cause an on-the-fly savings of settings.

EDIT: The rest of this comment was a wrong description of the cause and created confusion. Sorry for that!

In some places, when the user changes a setting, the new value is immediately saved. Apart from the problem that this can cause in the case of widgets like spinbox (the user turns the mouse wheel and the config file is overwritten constantly), after it happens, the reset button may lose its functionality because it reads the config file and shows the new values instead of the old ones.

In all config dialogs, the new values should be saved only when their OK or Apply buttons are pressed — or, at least, when their Close buttons are pressed, in case there's no OK/Apply button. Only in that way, the Reset button makes sense. "Reset" means "Restore Old Values".

[ghost]

I think having the Apply/OK button pairs available uniformly would be great. The best semantics I feel are: 'OK' applies changes and closes dialog, 'Apply' applies changes and doesn't close dialog, and settings are otherwise not applied automatically. Then 'Reset' simply changes the values back, but still would require pressing 'OK'/'Apply' to apply.

[ghost]

The only issue I see with uniform 'OK'/'Apply' is - what happens when someone closes the window with settings values different from what's applied? Do they get a message box asking them to save? This could clutter the GUI.

[tsujan]

See LXQt Appearance Configuration.

Here the important thing is the Reset button. If it does its job as expected, there will be no need to Apply/OK in small dialogs. But, for example, the Panel config dialog still needs Apply/OK, like in LXQt Appearance Configuration. I'll fix all of them.

[ghost]

Yes, I agree that for settings dialogs with minimal values Apply/OK isn't necessary. I believe even Windows does this - it only has OK for small dialogs.

EDIT: * OK/Cancel for small dialogs.

[tsujan]

Yes, "OK" may make more sense than "Close" when "Reset" exists.

Anyway, let's keep the "Close" buttons for small dialogs and talk about "OK" after the "Reset" buttons are fixed.

[palinek]

When I first used the LXQt (or maybe even Razor), the on-the-fly applying of styling/configuration was the most impressive thing, that I enjoyed. This was the feature, which was making the DE unique. I'm missing it in config and I will miss it also here.

Just my 2 cents ...

[tsujan]

Just my 2 cents ...

I accept it because you were here long before I was.

For widgets like spinbox and combobox, there's a simple way of avoiding constant writing to the config file (when the mouse wheel is used) and respecting on-the-fly applying at the same time.

As for the Reset button, I should start to read the codes and see why it doesn't have effect in some.

[elviosak]

When I first used the LXQt (or maybe even Razor), the on-the-fly applying of styling/configuration was the most impressive thing

I agree, i really dislike having to "Apply" everything on KDE whenever i use it... Changing stuff on-the fly is much better IMO.
On customcommand the dialog stores the config values when dialog opens and remains unchanged, used only to restore the values if "Reset" is clicked, everything is applied on change..

As for the Reset button, I should start to read the codes and see why it doesn't have effect in some.

Last i checked, on some plugins the reset button was not connected and there was no handler method, and those that implement differ from one another (some apply on close and some on-the-fly).

[tsujan]

To me, both on-the-fly and KDE style are acceptable, although I prefer a third method (which I've used in my own apps). Alf preferred the KDE style; so, I thought it was accepted by other LXQt members too.

Anyway, this issue is about the boring job of fixing the Reset buttons. I accepted @palinek's view and won't change the on-the-fly behavior.

[stefonarch]

We had an issue about that: #1581

I agree on avoiding apply buttons where they are not necessary, "Ok" is clear. For example in appearance dialog "apply" makes sense for palette colors testing ecc.

I really don't like this "you have unsaved changes" when switching tabs or items in systemsettings.

[ghost]

I like on-the-fly configuration, it does add to code complexity though. I think automatically updating from the config files is a step too far. (And has performance implications).

Maybe this issue could be also opened in other sub-projects. lxqt-config-appearance has Reset/Apply/Close, lxqt-admin-time has OK/Cancel, pcmanfm-qt Desktop Preferences's has OK/Cancel/Apply. I propose having a unified non-dynamic settings with [Reset]/Apply/OK - for certain GUIs. Which ones is unclear, and perhaps some could become dynamic.

[tsujan]

We had an issue about that: #1581

I closed it only because you said "this works apparently. So maybe we can close this." Only a few days ago, I found out that Reset buttons didn't work in most config dialogs (they do in some).

I like on-the-fly configuration

As I said, I won't touch it. This issue will be only for fixing the Reset buttons.

[tsujan]

Maybe this issue could be also opened in other sub-projects

No, please! There is no issue about Apply/OK and there is no rule carved in stone.

[tsujan]

This is the first PR: #1742. Nothing has changed in it except for fixing the Reset button.

Will wait for each PR to be approved and merged before making the next one. The applying behavior won't change in any of them.

[elviosak]

@tsujan i just found there’s a SettingsCache in PluginSettings, so adding settings.sync(); here

https://github.com/lxqt/lxqt-panel/blob/master/panel/pluginsettings.cpp#L39

should make Reset work for some (or most?) plugins, tested mount and works as expected, volume does not, i'm trying to find why.
After that still need to test and check each plugin if they're implementing something else for reset btn handling.

[tsujan]

@slidinghotdog
I saw the code and remembered everything. See the first PR.

[elviosak]

volume is woking fine, it's just not updating the value in the disabled checkbox.

Saw the PR.. So settings.sync() is not needed? mount and volume only works after changing it, and i don't see what’s different in config handling from taskbar

[tsujan]

... and i don't see what’s different in config handling from taskbar

I'm proceeding step by step; haven't reached it yet. Right now, I fixed Status Notifier. It needed an extra method because of its visibility list. So, each plugin may require a special handling.

I should sleep now; otherwise I won't be alive tomorrow.

[tsujan]

volume is woking fine, it's just not updating the value in the disabled checkbox.

I reached it now ;) As I suspected, each plugin may need a different handling. In the case of Volume, all signals also work when the widget states are changed programmatically (I dislike that because of constant savings but that's beside the point in this series of PRs). Therefore, saving is done automatically whenever loadSettings() is called by the Reset button. Only a small change is needed (a condition is misplaced in the code), which is exactly about the disabled checkbox.

[palinek]

Apart from the problem that this can cause in the case of widgets like spinbox (the user turns the mouse wheel and the config file is overwritten constantly), after it happens, the reset button may lose its functionality

Can you, please, describe this step-by-step? After submitting the #1745, I tried the existing master and don't observe this "loosing functionality" (seems like the description about the settingsChanged in the mentioned PR is wrong)

[tsujan]

@palinek, I think you didn't read my comments in #1745 and #1744.

[tsujan]

and don't observe this "loosing functionality"

Nothing was lost in the first place; nothing was "loosing" either.

[palinek]

Nothing was lost in the first place; nothing was "loosing" either.

? It is in your OP: "...after it happens, the reset button may lose its functionality". Why are you now saying nothing is lost, loosing, etc. ?

[tsujan]

Why are you now saying nothing is lost...

When I started to report the issue, I didn't have the code of lxqt-panel in front of me, and I made a mistake in describing its cause. It was just that: my wrong description.

[tsujan]

Let me explain it here too:

There's no problem in the main code and no extra signal is needed. The problems are in the codes of some plugins.

We can't enforce a rule that, for example, the signal QAbstractButton::clicked shouldn't be used for saving settings in a plugin. That signal isn't emitted when the check state of a button is changed by loadSettings() (to say nothing of the possibility that loadSettings() may not even do it). There's nothing wrong with that.

There can also be very different codes, like that of Status Notifier's visibility list.

Therefore, each plugin needs to be handled separately.

[palinek]

Please, show me the wrong behavior of taskbar plugin before your fix in #1742.

[tsujan]

In lxqttaskbarconfiguration.cpp, QAbstractButton::clicked is used for saving settings, and that's OK. The Reset button correctly called LXQtTaskbarConfiguration::loadSettings and that method set the states of check buttons correctly. But, of course, nothing was saved; why should it be saved?

If, instead of QAbstractButton::clicked, QAbstractButton::toggled was used, settings would be saved, but that's somehow beside the point.

[palinek]

I still don't get the wrong behavior there. What steps do I need to follow to replicate it?

[tsujan]

What steps do I need to follow to replicate it?

Before the patch: Click Reset and keep the dialog open. Nothing will be saved. Therefore, you'll see no real change.

After the patch: Click Reset and keep the dialog open. Old settings will be saved. Therefore, you'll see the changes in the panel.

[palinek]

Before the patch: Click Reset and keep the dialog open. Nothing will be saved. Therefore, you'll see no real change.

This is not the case here. Reset works correctly as it should. And every setting is instantly applied in the plugin.

[tsujan]

And every setting is instantly applied in the plugin.

In the dialog, yes. In the real plugin, on the panel, no.

[palinek]

In the dialog, yes. In the real plugin, on the panel, no.

I know what I see. Everything is applied in the configuration dialog and in panel's widget.

[tsujan]

I know what I see.

As I did ;)

Please tell me why the changes should be applied, by referring to the code of lxqttaskbarconfiguration.cpp before the patch.

[tsujan]

Take the example of groupingGB. setChecked() is called for it in loadSettings(). That just affects the dialog because its clicked signal is connected to saveSettings(), not its toggled signal.

[palinek]

Take the example of groupingGB. setChecked() is called for it in loadSettings(). That just affects the dialog because its clicked signal is connected to saveSettings(), not its toggled signal.

And what special does the saveSettings()? It just sets the value into the settings object. The very same as the loading from cache does. The difference is only emitting the settingsChange() signal.

[palinek]

FYI ... Finally I simulated the wrong behavior. The culprit is my testing env... I'm testing the panel with the config file specified on command line and with this everything is running smooth. But with the default configs used (no command line option), I see your described behavior.

EDIT: and the #1745 fixes it

[tsujan]

EDIT: and the #1745 fixes it

Of course it does, in the case of Taskbar. But it isn't needed at all.

[tsujan]

As I said before, each plugin needs to be handled separately:

• Some config dialogs use signals that are also emitted by loadSettings. They already behave correctly and need no change. panel/plugin: Fix settings storing/restoring #1745 is redundant for them
• Some config dialogs reflect the effect of the Reset button in themselves but not in Panel. panel/plugin: Fix settings storing/restoring #1745 may fix them.
• Some config dialogs do not even reflect the changes completely.

Again, as I implied elsewhere, we can't enforce a rule on contributors' works. Each plugin should do the job correctly.

[palinek]

Of course it does, in the case of Taskbar.

And for the comparison with #1745 the LXQtTaskBar::settingsChanged() is called once on Reset and with #1741 it is called 16 times.