search

lxqt / qterminal

A lightweight Qt-based terminal emulator
https://lxqt.github.io
GNU General Public License v2.0
586 stars 150 forks source link

Resolved conflicts when settings are changed by multiple instances #1146

Open tsujan opened 1 week ago

tsujan commented 1 week ago

Please note that changing of settings in one instance isn't reflected immediately by the windows of others, because that might result in problems.

Fixes https://github.com/lxqt/qterminal/issues/1145

isf63 commented 1 week ago

Wouldn't the easiest solution be to not save settings on exit, and only upon Ok/Apply in settings?

tsujan commented 1 week ago

Wouldn't the easiest solution be to not...

That would result in bugs. Some settings are changed directly from the main window. Moreover, it wouldn't make any difference as far as the conflict is concerned because the settings dialogs of different instances might show different settings.

stefonarch commented 1 week ago

Just rechecked in master git: there it works fine. My guess: it's the only setting which can't be applied instantly.

tsujan commented 1 week ago

Thanks for finding the problem. I think there should be 7 other settings with the same behavior and that they come from another part of the code that isn't compatible with this change, namely PropertiesDialog::apply(). Will look into it.

tsujan commented 1 week ago

@stefonarch, I added a commit; please try again.

Among other issues, changes made to shortcuts by another instance would be lost if its window was closed before the current one. That's fixed now. Moreover, as soon as the preferences dialog of the current instance is shown, the new shortcuts will also be applied to its window; otherwise, they will be applied to new windows.

I didn't find another problem by checking the code, but you might find one in practice ;)

tsujan commented 1 week ago

Didn't find any issues anymore now.

I also did my best to find an issue, both in the PR and in practice, but only found a few small issues in the original code instead. Might address them later, in separate PRs.