Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a unified settings model which will eventually remove the need for QucsSettings #533

Merged
merged 19 commits into from
May 3, 2024

Conversation

iwbnwif
Copy link
Contributor

@iwbnwif iwbnwif commented Feb 6, 2024

This is the first stage of refactoring the settings implementation. Currently the only change is to access the underlying QSettings engine via the new settings class.

The motivation behind this change is to support the implementation of the shortcut key feature #368.

The main advantage of this is that it is simpler to create new settings items. If you want to write a new setting, just add _settings::Get().setItem<T>("key", value); To read it back, it's just _settings::Get().item<T>("key"). If you want to give it a default, just add it in settings::initDefaults.

There is also a mechanism to have aliases for old settings names if we decide to update the names in the future.

Ultimately, it shouldn't be necessary to have QucsSettings at all - use _settings::Get().item<T>("key") directly in place. I haven't done this part yet though because it means changing a lot of files and I think it's best to exercise the new model a bit first.

This change implements a QucsSingleton class that can be used to
make any class singleton. It also provides a new settingsManager
class which uses QucsSingleton to provide a single, static access
point to the standard QSettings functionality. A shortcut is provided
and a new method for providing a default value if the requested key
is not set.
The recent docs list is stored as a * separated string and needs to be
converted to a string list when reading from the settings file.
Replace almost all calls in main::saveApplSettings to the new unified
settings model.
@zergud
Copy link
Collaborator

zergud commented Feb 7, 2024

thnx!
as stated earlier, we have decided to postpone settings refactoring to next release..
its need to be designed together with ActionManager and, may be, ShortcutManager..
there will be a lot of changes in many files

so, I suggest not changing anything now

@ra3xdh ra3xdh added this to the 24.2.0 milestone Feb 7, 2024
@ra3xdh
Copy link
Owner

ra3xdh commented Feb 7, 2024

Thanks for the contribution. The removing of QucsSettings structure would be useful. The existing settings implementation uses a global variable that is against C++ rules. The proposed implementation looks much more better.

But I propose to shift the massive refactoring PRs to the next release. I am planning to prepare the v24.1.0 package within next two weeks. After the release will be done, we can review and merge this PR. I have assigned a 24.2.0 milestone for this PR.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Feb 7, 2024

Thank you and that is no problem at all. Of course can certainly save it for the next release and I would be happy to look at the shortcut key closer to the time if you like.

qucs/settings.cpp Outdated Show resolved Hide resolved
@ra3xdh
Copy link
Owner

ra3xdh commented Apr 8, 2024

Please check if this PR is compatible with the recent changes. I have added a QucsSettings.RFLayoutExecutable to hold the RFLayout tool location. @wawuwo has changed working directory setting in #658. Rebase this PR if necessary. Then I will merge this.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Apr 9, 2024

I have merged the current branch with this PR branch but still need to go through #658. I only pushed the latest commit to see if there are other build issues, so please don't start to review yet.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Apr 19, 2024

I have finished merging in the changes from #658 for now and I think that this can PR can be merged.

There are a few things to bear in mind:

  1. The final objective is to completely remove QucsSettings.
  2. The reason is that QucsSettings effectively duplicates the Qt settings object which means that every time a setting is added or changed then the new / changed setting has to be duplicated in QucsSettings.
  3. It should be possible to simply use _settings::Get().setItem<T>() and _settings::Get().item<T>() everywhere. I would encourage all new code to use these new methods.
  4. The new methods and QucsSettings both work with the underlying settings object, but only synchronise when saveApplSettings and loadSettings in main.cpp are called.
  5. If a setting should have a default value, please add it to the m_Defaults map in settings.cpp, settingsManager::initDefaults.
  6. There are still one or two shortcomings for edge case uses of QucsSettings - mostly these are to do with constructing one setting's default from another setting.

The reason for merging this PR now is to avoid a too big set of files to merge later. As can be expected, the settings items permeate almost all classes in the code, so moving completely to the new scheme will touch a lot of files. My thought is that it would be better to do one area at a time, but of course this is only a suggestion!

@ra3xdh
Copy link
Owner

ra3xdh commented Apr 20, 2024

@iwbnwif Thanks! I will try to test and merge this within the next 1-2 weeks.

@ra3xdh
Copy link
Owner

ra3xdh commented Apr 25, 2024

I have tested this PR and found one major issue. The window position and window size resets to default on every application start. The previous version was able to remember window size and position. At least it would be sufficient to always open the main window maximized.

@ra3xdh
Copy link
Owner

ra3xdh commented Apr 25, 2024

@zergud , Could you check the operation of the Windows version using new settings model provided by this PR?

@ra3xdh ra3xdh mentioned this pull request Apr 25, 2024
@zergud
Copy link
Collaborator

zergud commented Apr 25, 2024

@zergud , Could you check the operation of the Windows version using new settings model provided by this PR?
ок

I thing we should migrate to new settings at one time, without leaving for later implementation

@zergud
Copy link
Collaborator

zergud commented Apr 25, 2024

@iwbnwif does your solution support setting like "gui/showMenuBar" ?

@ra3xdh
Copy link
Owner

ra3xdh commented Apr 25, 2024

@zergud

does your solution support setting like "gui/showMenuBar" ?

This feature was never presented in application before and should be a separate PR. This PR doesn't introduce this feature. This PR is about refactoring of the existing settings representation.

@zergud
Copy link
Collaborator

zergud commented Apr 25, 2024

@zergud

does your solution support setting like "gui/showMenuBar" ?

This feature was never presented in application before and should be a separate PR. This PR doesn't introduce this feature. This PR is about refactoring of the existing settings representation.

this feature supports by QSettings out of box
https://doc.qt.io/qt-6/qsettings.html

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Apr 25, 2024

I have tested this PR and found one major issue. The window position and window size resets to default on every application start. The previous version was able to remember window size and position. At least it would be sufficient to always open the main window maximized.

This should be simple to fix. Qt has a function to do exactly this behaviour. I will look into it and update the PR but it might be a day or so.

https://doc.qt.io/qt-6/qsettings.html#restoring-the-state-of-a-gui-application

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Apr 25, 2024

@iwbnwif does your solution support setting like "gui/showMenuBar" ?

At the moment I have only tried to replicate settings that are already there. The idea is to replace the QucsSettings object with the inbuilt QSettings. This should allow more flexibility in future and easier to add new settings items.

@ra3xdh
Copy link
Owner

ra3xdh commented Apr 26, 2024

At the moment I have only tried to replicate settings that are already there.

Yes, let's restrict by only replication of the existing features in this PR. New features should go in a separate patch.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Apr 28, 2024

The previous version was able to remember window size and position.

Window size and position should now be remembered. In fact, it should be better than previously because also the screen will be remembered in a multi-screen setup, and the maximized / minimized flags should be set in accordance with the state when the previous execution was closed.

I have only been able to test this on Linux, but I can't see any particular problems on other platforms because only standard Qt calls are used.

@ra3xdh ra3xdh merged commit d59b5e7 into ra3xdh:current May 3, 2024
2 checks passed
@ra3xdh
Copy link
Owner

ra3xdh commented May 3, 2024

@iwbnwif I have merged this PR. Thanks for the contribution. One improvement could be added. The application window should be started maximized on the first start (check the former QucsSetting.firstRun).. This may be implemented in the future. As far as I can understand the second part of the settings refactoring will follow soon.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented May 3, 2024

Thank you.

One improvement could be added. The application window should be started maximized on the first start (check the former QucsSetting.firstRun).

This should be trivial I think. For example, add showMaximized(); on line 118 of qucs.cpp. I will leave as a note here and incorporate to a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants