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

refactor: Add OptionsModel getOption/setOption methods #600

Merged
merged 1 commit into from
May 22, 2022

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented May 19, 2022

This is a trivial change which is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It is split off from #602 because it causes a lot of rebase conflicts (any time there is a GUI options change).

This PR is very small and easy to review ignoring whitespace: https://github.com/bitcoin-core/gui/pull/600/files?w=1

Easiest to review ignoring whitespace.
@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #497 (Enable users to configure their monospace font specifically by luke-jr)
  • #440 (Do not require restart if overridden option is modified by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a63b60f

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK a63b60f

successful = setOption(OptionID(index.row()), value);
}

Q_EMIT dataChanged(index, index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR but we actually don't need to call dataChanged if nothing changed inside setOption (which can happen if the new value is the same as the stored one or if something bad happened internally, like a bad parsing or a impossibility to store the new value).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine as is.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK a63b60f.

}
break;
default:
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it should set successful = false? Or maybe just drop the default cases and let the compiler complain about unhandled cases?

successful = setOption(OptionID(index.row()), value);
}

Q_EMIT dataChanged(index, index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine as is.

@hebasto hebasto merged commit b0e16eb into bitcoin-core:master May 22, 2022
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

post merge ACK a63b60f

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 28, 2022
@bitcoin-core bitcoin-core locked and limited conversation to collaborators May 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants