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

Do not save compare options changed on the fly. #460

Open
1 task done
SamuelPlentz opened this issue Oct 17, 2020 · 12 comments
Open
1 task done

Do not save compare options changed on the fly. #460

SamuelPlentz opened this issue Oct 17, 2020 · 12 comments

Comments

@SamuelPlentz
Copy link
Contributor

I usually want to compare everything:

  • all whitespace differences
  • all case sensitive differences
  • all empty line differences
  • all linebreak differences
  • all codepage differences
  • all comment differences

But in special cases I have to deactivate 1 or 2 rules (especially "whitespace" and "case sensitive") of that list. If I do this and restart Winmerge, the rules are still deactivated and I probably miss important differences.

I want to reset my compare options on each restart of Winmerge to my defaults. IMO other options are not affected.

What are your ideas to handle this use case? Is it even possible yet?

I propose to add an option and a button:

  • Save compare options automatically

Save compare options now

Implementing note: Some options like "compare whitespace" can be changed on the fly. They need to be reverted as well.

@SamuelPlentz
Copy link
Contributor Author

@sdottaka: I consider to create a pull request for this, but I don't want to start, before you comment on this.
I am open for other ideas facing that issue.

@sdottaka
Copy link
Member

I agree with your idea, but I prefer all options, not just the comparison option.

FYI, you can call CRegOptionsMgr::SetSerializeing(false) to prevent the settings from being saved in the registry.

@SamuelPlentz
Copy link
Contributor Author

SamuelPlentz commented Oct 19, 2020

I made a mockup, how it could look:

Winmerge

Colors and strings could be adjusted. I think it is a good idea to provide some kind of warning banner.
Do you agree with the position (section General below the Language entry) and the additional banner?

I'm not sure about the strings.

Instead of:
The option "save options automatically" is disabled. Changes are lost on restart.

It could be:
As "save options automatically" is disabled, changes are lost on restart of Winmerge.

@SamuelPlentz
Copy link
Contributor Author

SamuelPlentz commented Oct 19, 2020

Probably the banner should be yellow (warning) instead of red (error), because everything works like intended.

@sdottaka
Copy link
Member

Do you agree with the position (section General below the Language entry) and the additional banner?
I agree with you.
If the implementation is cumbersome, you can implement it by displaying a message box instead.

@mrLithe
Copy link

mrLithe commented Oct 20, 2020

I propose to make buttons presets on the toolbar for different options of user settings.

@SamuelPlentz
Copy link
Contributor Author

SamuelPlentz commented Nov 30, 2021

Mh, my motivation to implement this is somewhat reduced, as I found a neat workaround suitable for me.

I use the newly introduced winmerge.ini from #248. Thank you! By adding the attribute readonly to that file, WinMerge uses it to read the options, but not to write them. That is exactly what I wanted and this works really well!

Probably this could even be improved like this:
When starting WinMerge it first reads the file winmerge.ini and after that it reads the file winmerge readonly.ini.
When closing WinMerge it only writes the file winmerge.ini.

Every option contained in the file winmerge readonly.ini would be reverted to its default. Other options would still be changed and saved.
For example my winmerge readonly.ini would look like this.

[WinMerge]
Settings/IgnoreSpace=0
Settings/IgnoreBlankLines=0
Settings/IgnoreCase=0
Settings/IgnoreEol=0
Settings/IgnoreCodepage=0

Another name for the file could be winmerge defaultoptions.ini, winmerge defaults.ini, defaults.ini or something similar.

@SamuelPlentz
Copy link
Contributor Author

I tried to look into an implementation. The right place should be in the code inserted in pull request #750, correct?

If I understand it correctly there is no read the whole file at startup (as I assumed). Instead if an option is needed it is queried (and cached for further use) from the file or from the registry directly.

So the plan should be adjusted to:
When an option is read, one should first try to read it from winmerge readonly.ini and if that fails read it from winmerge.ini.
Write it to winmerge.ini.

So the following code should be adjusted. If a winmerge readonly.ini file exists query that file first (another call to GetPrivateProfileString and an duplicated function GetFilePath returning the winmerge readonly.ini file path). If the result is empty query winmerge.ini (as already implemented).

String CIniOptionsMgr::ReadValueFromFile(const String& name)
{
	const int size = 100;
	LPWSTR buffor = new TCHAR[size];
	DWORD result = GetPrivateProfileString(lpAppName, name.c_str(), NULL, buffor, size, GetFilePath());
	return buffor;
}

Can you please confirm that a pull request like this would be included?
Can you also clarify what name such a file should have? Currently I think defaults.ini is a good choice.

@sdottaka
Copy link
Member

sdottaka commented Dec 1, 2021

WinMerge has a command-line option /inifile that specifies the path to the INI file.

For this reason, this option also needs to be changed to accept multiple INI files.

Instead, I prefer to store separate sections in the same INI file, as shown below.

winmerge.ini

[WinMerge]
....

[Defaults]
Settings/IgnoreSpace=0
Settings/IgnoreBlankLines=0
Settings/IgnoreCase=0
Settings/IgnoreEol=0
Settings/IgnoreCodepage=0

@SamuelPlentz
Copy link
Contributor Author

Good idea to add another section to the winmerge.ini file. I had the /inifile command-line option in mind and wanted to duplicate it, but this approach is better.

I created the pull request #1071 to implement that behavior.

sdottaka pushed a commit that referenced this issue Dec 6, 2021
After reading the "WinMerge" section try to read the "Defaults" section.
Overwrite existing entries in "iniFileKeyValues" with the ones from the "Defaults" section.
This section is not written to, as it should contain default entries, that are reverted at each restart of WinMerge.
More informations see #460.
@SamuelPlentz
Copy link
Contributor Author

I just tested the new "Defaults" section of the ini file in the current beta.
Everything works as intended.

@vlakoff
Copy link
Contributor

vlakoff commented Mar 1, 2024

As the corresponding feature has been implemented, I think this issue may be closed.

I would have preferred a less ambiguous name for the INI section, like "[Startup]" or "[Overrides]", but no biggie, as it is an "hidden, advanced feature", and nevertheless it works as it is.

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

No branches or pull requests

4 participants