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

Possibility to store settings in INI file (#248) #750

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

grzesie
Copy link
Contributor

@grzesie grzesie commented Apr 24, 2021

Hi.

Here is first code that allows to store setting in INI file. According to issue #248 it checks if INI file exist - if it exist, uses it. Otherwise use previous register mechanism.

@sdottaka
Copy link
Member

Thank you!

I will release version 2.16.12 this month, so I will include this Pull Request after the release.

After applying this Pull Request, I may change the following parts later.

  • If using std::filesystem causes an unacceptable increase in binary size or does not work on Windows XP, I will change it to use the Poco library or Win32 API instead.

  • Since the behavior has changed due to comment out (ToDo: part), I want to restore it.

  • I want to rename the class of IniOptionsMgr to CIniOptionsMgr to match CRegOptionsMgr.

  • Since there is a concern that performance will deteriorate using INI file instead of the registry, I would like to introduce an asynchronous writing mechanism.

@grzesie
Copy link
Contributor Author

grzesie commented Apr 24, 2021

  • Since the behavior has changed due to comment out (ToDo: part), I want to restore it.

Maybe it should be changed to this:
if (typeid(*pOptions) == typeid(CRegOptionsMgr)) { static_cast<CRegOptionsMgr *>(GetOptionsMgr())->CloseKeys(); }

Should i change this and class names?

@sdottaka
Copy link
Member

If you have time, I would appreciate it if you could make that changes.

changing name to CIniOptionsMgr
@grzesie
Copy link
Contributor Author

grzesie commented Apr 24, 2021

Name changed, also removed this TODO and wrapped it in IF. However 1 comment here:
This is in OnIdle method - it is executing many times. I don't think closing keys should be executed so many times during program execution. But maybe i am not aware of something and it is must have...

@sdottaka
Copy link
Member

Thank you for the update.

There is an environment where it takes a long time to open the registry key, so keeping the registry key open to speed up continuous access to the registry.
However, I don't want to keep the registry key open all the time, so closing the registry key when WinMerge is in the Idle state.
CloseKeys() is called many times, but if the registry key is already closed, nothing will happen, so I think it's okay. Of course, Pull Requests are welcome if there is another good way.

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

Successfully merging this pull request may close these issues.

2 participants