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

citra_qt: Settings (configuration) default value fix #3924

Merged
merged 1 commit into from Aug 3, 2018

Conversation

Projects
None yet
4 participants
@zhaowenlan1779
Member

zhaowenlan1779 commented Jul 9, 2018

This is to fix #3531.
Design:

  1. When a setting does not has a default value, it is stored in the same way as before. (Simple QSettings entry)
  2. When a setting has a default value, it is separated into two entries: xxxxx\default and xxxxx. If xxxxx\default = true, it means that the setting is using its default value, and when the configuration is loaded, the setting will be automatically replaced with the new default value (if any). The xxxxx entry stores the setting value, however it is only used when xxxxx\default = false. See the ReadSetting and WriteSetting functions for more detail.
  3. In this way the new config file will be compatible with the old file.
  4. I did not make any changes to the core/settings part. It remained as-is.

Advantages:

  1. We can tell if a user is using the default setting or not and upgrade them to the new defaults when any is changed.
  2. Well, I can't think of more.

Concerns:

  1. Do we really want to upgrade users to new default settings silently? What if I intentionally set it to some value, and the value happened to be the default value? But I think for a common user it is good enough to use the default settings most of the time.

This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Jul 9, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-09T12:33:22Z: 610acf2...zhaowenlan1779:24de4f833c91f0cf1d6239176534a6de8b57a3a7

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Jul 9, 2018

Hmm. After another thought I think I can make it compatible with the old config file. Let me try.

@cluezbot

This comment has been minimized.

cluezbot commented Jul 9, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-09T12:45:21Z: 610acf2...zhaowenlan1779:52c4d8a3c137ba7066ca96979c4299d20ff607a1

@BreadFish64

This comment has been minimized.

Contributor

BreadFish64 commented Jul 9, 2018

What if I intentionally set it to some value, and the value happened to be the default value?

I think ideally if the user changes the setting at all then the value should not be updated when there's a new default.

@wwylele wwylele added canary-merge and removed canary-merge labels Jul 17, 2018

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:settings branch from 52c4d8a to b68fb92 Jul 20, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jul 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-20T09:22:06Z: 7868506...zhaowenlan1779:b68fb920c4ec1b05e396f3804557dc934318245a

@@ -10,7 +11,6 @@
#include "network/network.h"
Config::Config() {
// TODO: Don't hardcode the path; let the frontend decide where to put the config files.

This comment has been minimized.

@wwylele

wwylele Jul 20, 2018

Member

Why is this removed?

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:settings branch from b68fb92 to 1d7dc5a Jul 20, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Jul 20, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-07-20T10:57:23Z: 7868506...zhaowenlan1779:1d7dc5a23c8ab9a035aed8da6995483b35a236ca

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:settings branch from 1d7dc5a to 650c20e Aug 3, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Aug 3, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-08-03T05:50:03Z: 751e00a...zhaowenlan1779:650c20eebf83b53de5d01d763b895efd008478cf

@wwylele wwylele merged commit 5bc72cc into citra-emu:master Aug 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zhaowenlan1779 zhaowenlan1779 deleted the zhaowenlan1779:settings branch Aug 4, 2018

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