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

ISOProperties: Rename "Edit Config" and "Show Defaults" #6299

Merged
merged 1 commit into from Jan 12, 2018

Conversation

7 participants
@Ebola16
Member

Ebola16 commented Jan 11, 2018

Fixes https://bugs.dolphin-emu.org/issues/10775

These were very confusing to me due to the reasons outlined in the link above.

@JosJuice

This is likely going to be made redundant by the PR that makes us put the default INIs in a zip file, but I'm fine with merging this for now.

@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Jan 11, 2018

Contributor

By the way, to explain why the text was the way it was: The default INIs are actually read-only sometimes, depending on the operating system and the install method. They're never read-only in Windows development builds, but they might be if you install a stable build (I haven't checked), and I think they're typically read-only on Linux. With that in mind, I think replacing the "read-only" part with a "please don't edit them" part is fine, since Windows development builds are a common use case after all.

Contributor

JosJuice commented Jan 11, 2018

By the way, to explain why the text was the way it was: The default INIs are actually read-only sometimes, depending on the operating system and the install method. They're never read-only in Windows development builds, but they might be if you install a stable build (I haven't checked), and I think they're typically read-only on Linux. With that in mind, I think replacing the "read-only" part with a "please don't edit them" part is fine, since Windows development builds are a common use case after all.

@MayImilae

This comment has been minimized.

Show comment
Hide comment
@MayImilae

MayImilae Jan 11, 2018

Contributor

I'm fine with this, but...

The Default SYS INIs should not be editable, on any operating system, without the user manually undoing the read-only block! I think we should look into fixing that before we adjust the naming of the setting. EDIT: If the default INIs in a zip file fixes this, maaaybe we should do that first, then adjust the option names?

Contributor

MayImilae commented Jan 11, 2018

I'm fine with this, but...

The Default SYS INIs should not be editable, on any operating system, without the user manually undoing the read-only block! I think we should look into fixing that before we adjust the naming of the setting. EDIT: If the default INIs in a zip file fixes this, maaaybe we should do that first, then adjust the option names?

@Ebola16

This comment has been minimized.

Show comment
Hide comment
@Ebola16

Ebola16 Jan 11, 2018

Member

Ok thanks, that makes more sense. Even if default config files aren't editable in a ZIP, the phrasing of this PR remains correct.

@MayImilae, I need to edit the default SYS INIs as some important options on Android aren't implemented in the UI yet. I understand wanting to prevent users from messing with those but it will make clean installs of Dolphin much more annoying to test.

Member

Ebola16 commented Jan 11, 2018

Ok thanks, that makes more sense. Even if default config files aren't editable in a ZIP, the phrasing of this PR remains correct.

@MayImilae, I need to edit the default SYS INIs as some important options on Android aren't implemented in the UI yet. I understand wanting to prevent users from messing with those but it will make clean installs of Dolphin much more annoying to test.

@MayImilae

This comment has been minimized.

Show comment
Hide comment
@MayImilae

MayImilae Jan 11, 2018

Contributor

I need to edit the default SYS INIs as some important options on Android aren't implemented in the UI yet.

But shouldn't that be addressed by allowing the creation of user INIs on android, not encouraging users edit the file with settings we know work and don't want them to touch?

I don't agree with holding back improvements to the main version to preserve a work around for something that's broken and we should be fixing in Android!

Contributor

MayImilae commented Jan 11, 2018

I need to edit the default SYS INIs as some important options on Android aren't implemented in the UI yet.

But shouldn't that be addressed by allowing the creation of user INIs on android, not encouraging users edit the file with settings we know work and don't want them to touch?

I don't agree with holding back improvements to the main version to preserve a work around for something that's broken and we should be fixing in Android!

@Ebola16

This comment has been minimized.

Show comment
Hide comment
@Ebola16

Ebola16 Jan 11, 2018

Member

Oh I see. A default ZIP of SYS INIs along with user-editable INIs wouldn't be a bad thing. I didn't think user SYS INI corruption was a big issue but I may be wrong on that.

Regardless, that probably shouldn't block this PR as the changed buttons don't open the SYS INIs and the phrasing works for both zipped and non-zipped default INIs. Let me know if I'm misunderstanding something as I didn't look at the zipped INI PR in great detail.

For my future reference, the ZIP INI PR is here: #6178

Member

Ebola16 commented Jan 11, 2018

Oh I see. A default ZIP of SYS INIs along with user-editable INIs wouldn't be a bad thing. I didn't think user SYS INI corruption was a big issue but I may be wrong on that.

Regardless, that probably shouldn't block this PR as the changed buttons don't open the SYS INIs and the phrasing works for both zipped and non-zipped default INIs. Let me know if I'm misunderstanding something as I didn't look at the zipped INI PR in great detail.

For my future reference, the ZIP INI PR is here: #6178

@MayImilae

This comment has been minimized.

Show comment
Hide comment
@MayImilae

MayImilae Jan 11, 2018

Contributor

True, it is worded well. It's vaguer than I'd like, but that kind of reflects the current vague status of it at the moment. I'll approve the change, in a totally just writing and not coding approval!

EDIT: Wait, actually, I have nitpicks first.

Still, I don't like the vagueness, and when situation changes the wording should change too. @delroth could you make sure that whenever #6178 is merged that the View Default Config button tooltip indicates that it is read only? Since it will properly be read only in your PR!

Contributor

MayImilae commented Jan 11, 2018

True, it is worded well. It's vaguer than I'd like, but that kind of reflects the current vague status of it at the moment. I'll approve the change, in a totally just writing and not coding approval!

EDIT: Wait, actually, I have nitpicks first.

Still, I don't like the vagueness, and when situation changes the wording should change too. @delroth could you make sure that whenever #6178 is merged that the View Default Config button tooltip indicates that it is read only? Since it will properly be read only in your PR!

@delroth

This comment has been minimized.

Show comment
Hide comment
@delroth

delroth Jan 11, 2018

Member

Pretty sure I just removed the button in my PR anyway.

Member

delroth commented Jan 11, 2018

Pretty sure I just removed the button in my PR anyway.

@MayImilae

This comment has been minimized.

Show comment
Hide comment
@MayImilae

MayImilae Jan 11, 2018

Contributor

@delroth Whoops you mentioned that in the PR. Well, works for me!

Contributor

MayImilae commented Jan 11, 2018

@delroth Whoops you mentioned that in the PR. Well, works for me!

@Zexaron

This comment has been minimized.

Show comment
Hide comment
@Zexaron

Zexaron Jan 11, 2018

Contributor

I agree having the system default INIs treated as read-only (whether or not in ZIP), in terms that you can't edit them through the GUI, whether or not you have a filesystem read-only flag, either way anything shipped is editable if the user really wants to.

The user's INI settings would either be diffs to the defaults or a full user's copy which wouldn't be called a "user default" it would just be "user ini config" or something like that, it would just be a user setting and as long as the user uses his edits that would be saved into user config just as any other user config and would in practice work as a user default for a future session.

There could be a tickbox to switch between using user's INIs vs. system default INIs.

That said, if it was up to me, I would be in the favor of android having it's own android default INI defaults, so the user customization would be based on that, and not from desktop versions.

Contributor

Zexaron commented Jan 11, 2018

I agree having the system default INIs treated as read-only (whether or not in ZIP), in terms that you can't edit them through the GUI, whether or not you have a filesystem read-only flag, either way anything shipped is editable if the user really wants to.

The user's INI settings would either be diffs to the defaults or a full user's copy which wouldn't be called a "user default" it would just be "user ini config" or something like that, it would just be a user setting and as long as the user uses his edits that would be saved into user config just as any other user config and would in practice work as a user default for a future session.

There could be a tickbox to switch between using user's INIs vs. system default INIs.

That said, if it was up to me, I would be in the favor of android having it's own android default INI defaults, so the user customization would be based on that, and not from desktop versions.

@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Jan 11, 2018

Contributor

The user's INI settings would either be diffs to the defaults

That has already been the case for many years.

That said, if it was up to me, I would be in the favor of android having it's own android default INI defaults, so the user customization would be based on that, and not from desktop versions.

I don't really think there are any situations that that would be useful for.

Contributor

JosJuice commented Jan 11, 2018

The user's INI settings would either be diffs to the defaults

That has already been the case for many years.

That said, if it was up to me, I would be in the favor of android having it's own android default INI defaults, so the user customization would be based on that, and not from desktop versions.

I don't really think there are any situations that that would be useful for.

@JMC47

This comment has been minimized.

Show comment
Hide comment
@JMC47

JMC47 Jan 11, 2018

Contributor

I'd go a step further and say we're being negligent to have different GameINI defaults between Android and Desktop.

Contributor

JMC47 commented Jan 11, 2018

I'd go a step further and say we're being negligent to have different GameINI defaults between Android and Desktop.

@Zexaron

This comment has been minimized.

Show comment
Hide comment
@Zexaron

Zexaron Jan 11, 2018

Contributor

@JosJuice Someone mentioned differences on android platform, ... allright reading that again it seems like a minor temporary issue.

Contributor

Zexaron commented Jan 11, 2018

@JosJuice Someone mentioned differences on android platform, ... allright reading that again it seems like a minor temporary issue.

@JosJuice

This comment has been minimized.

Show comment
Hide comment
@JosJuice

JosJuice Jan 11, 2018

Contributor

Someone mentioned differences on android platform, ... allright reading that again it seems like a minor temporary issue.

Ebola16 said some settings weren't implemented in the UI. What the UI supports has no effect on what the emulation core supports, so this is irrelevant for the default INIs that we ship.

Contributor

JosJuice commented Jan 11, 2018

Someone mentioned differences on android platform, ... allright reading that again it seems like a minor temporary issue.

Ebola16 said some settings weren't implemented in the UI. What the UI supports has no effect on what the emulation core supports, so this is irrelevant for the default INIs that we ship.

@Ebola16

This comment has been minimized.

Show comment
Hide comment
@Ebola16

Ebola16 Jan 11, 2018

Member

Yeah @delroth proposes to remove the "Show defaults" button. Do we really want to remove it though? It's an easy way for users to check that they aren't overriding something that they shouldn't and you don't need to go searching through a massive zip in the future. I've never used the button but I do see a use case for it.

@MayImilae holding off on further edits to this PR until this is decided.

Member

Ebola16 commented Jan 11, 2018

Yeah @delroth proposes to remove the "Show defaults" button. Do we really want to remove it though? It's an easy way for users to check that they aren't overriding something that they shouldn't and you don't need to go searching through a massive zip in the future. I've never used the button but I do see a use case for it.

@MayImilae holding off on further edits to this PR until this is decided.

@Ebola16

This comment has been minimized.

Show comment
Hide comment
@Ebola16

Ebola16 Jan 11, 2018

Member

After actually reading the comments of #6178 I now think "Show Defaults" should be dropped eventually.

This PR will probably be ready to go before #6178 and is better than the current button names so this should probably just be merged after review. @delroth can then remove the newly-named "View Default Config" button again.

@MayImilae I incorporated your suggestions with the intention of losing "View Default Config" eventually. I added the "Settings in the user config INI override default config INI settings." sentence to actually answer the question of how one should properly edit values set in the default config INI.

@ZexaronS I think @JosJuice addressed most points but I wanted to add that anything that involves users disabling all user configs shouldn't even be considered until the default configs are read-only.

Member

Ebola16 commented Jan 11, 2018

After actually reading the comments of #6178 I now think "Show Defaults" should be dropped eventually.

This PR will probably be ready to go before #6178 and is better than the current button names so this should probably just be merged after review. @delroth can then remove the newly-named "View Default Config" button again.

@MayImilae I incorporated your suggestions with the intention of losing "View Default Config" eventually. I added the "Settings in the user config INI override default config INI settings." sentence to actually answer the question of how one should properly edit values set in the default config INI.

@ZexaronS I think @JosJuice addressed most points but I wanted to add that anything that involves users disabling all user configs shouldn't even be considered until the default configs are read-only.

@MayImilae

The new wording looks good to me!

@leoetlino leoetlino merged commit 955214c into dolphin-emu:master Jan 12, 2018

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment