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

doc: Naming convention for new QSettings setting names #295

Open
hebasto opened this issue Apr 25, 2021 · 3 comments
Open

doc: Naming convention for new QSettings setting names #295

hebasto opened this issue Apr 25, 2021 · 3 comments

Comments

@hebasto
Copy link
Member

hebasto commented Apr 25, 2021

On master (8f80092):

$ cat ~/.config/Bitcoin/Bitcoin-Qt.conf
[General]
MainWindowGeometry=@ByteArray(\x1\xd9\xd0\xcb\0\x3\0\0\0\0\ta\0\0\0h\0\0\r\x18\0\0\x2\x86\0\0\ta\0\0\0\x80\0\0\r\x18\0\0\x2\x86\0\0\0\0\0\0\0\0\n\0\0\0\ta\0\0\0\x80\0\0\r\x18\0\0\x2\x86)
PeersTabSplitterSizes=@ByteArray(\0\0\0\xff\0\0\0\x1\0\0\0\x2\0\0\x1\x12\xff\xff\xff\xff\0\xff\xff\xff\xff\x1\0\0\0\x1\0)
RPCConsoleWindowGeometry=@ByteArray(\x1\xd9\xd0\xcb\0\x3\0\0\0\0\r[\0\0\x1\v\0\0\x10p\0\0\x2\xe7\0\0\r[\0\0\x1#\0\0\x10p\0\0\x2\xe7\0\0\0\0\0\0\0\0\n\0\0\0\r[\0\0\x1#\0\0\x10p\0\0\x2\xe7)
RecentRequestsViewHeaderState=@ByteArray(\0\0\0\xff\0\0\0\0\0\0\0\x1\0\0\0\x1\0\0\0\0\x1\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\x3\x90\0\0\0\x4\0\x1\x1\x1\0\0\0\0\0\0\0\0\0\0\0\0\x64\0\0\0\x82\0\0\0\x84\0\0\0\0\0\0\0\x4\0\0\0\x9e\0\0\0\x1\0\0\0\0\0\0\0\xb3\0\0\0\x1\0\0\0\0\0\0\x1\xa2\0\0\0\x1\0\0\0\0\0\0\0\x9d\0\0\0\x1\0\0\0\0\0\0\x3\xe8\0\0\0\0\x64)
TransactionViewHeaderState=@ByteArray(\0\0\0\xff\0\0\0\0\0\0\0\x1\0\0\0\x1\0\0\0\x2\x1\0\0\0\0\0\0\0\0\0\0\0\x6\x2\0\0\0\x1\0\0\0\x1\0\0\0\x64\0\0\x3\x96\0\0\0\x6\0\x1\x1\x1\0\0\0\0\0\0\0\0\0\0\0\0\x64\0\0\0\x17\0\0\0\x84\0\0\0\0\0\0\0\x6\0\0\0\x64\0\0\0\x1\0\0\0\0\0\0\0\0\0\0\0\x1\0\0\0\0\0\0\0\x64\0\0\0\x1\0\0\0\0\0\0\0\x64\0\0\0\x1\0\0\0\0\0\0\0\xec\0\0\0\x1\0\0\0\0\0\0\x1~\0\0\0\x1\0\0\0\0\0\0\x3\xe8\0\0\0\0\x64)
UseEmbeddedMonospacedFont=true
addrProxy=127.0.0.1:9050
addrSeparateProxyTor=127.0.0.1:9050
bPrune=false
bSpendZeroConfChange=true
fCoinControlFeatures=false
fFeeSectionMinimized=true
fHideTrayIcon=false
fListen=true
fMinimizeOnClose=false
fMinimizeToTray=false
fReset=true
fRestartRequired=false
fUseNatpmp=false
fUseProxy=false
fUseSeparateProxyTor=false
fUseUPnP=false
language=
nConfTarget=6
nDatabaseCache=450
nDisplayUnit=1
nFeeRadio=0
nPruneSize=2
nSettingsVersion=219900
nSmartFeeSliderPosition=0
nThreadsScriptVerif=0
nTransactionFee=1000
strDataDir=/home/hebasto/.bitcoin
strThirdPartyTxUrls=

The ancient setting names follow Hungarian notation, except for addrProxy, addrSeparateProxyTor, and language.

Here are the changes since then:

Current suggestions in the open PRs:

Some observations:

  1. Widget geometry setting names follow the PascalCase (including open PRs), and the first parts of them point to the appropriate widgets
  2. Other setting names, e.g., display_unit in refactor: Make BitcoinUnits::Unit a scoped enum #60, follow the usual snake_case

It seems good for future code changes and maintenance to have explicit and documented naming conventions for new QSettings setting names before v22.0 branch off.

What do you think?

@hebasto hebasto added this to the 22.0 milestone Apr 25, 2021
@hebasto hebasto changed the title Naming convention for new QSettings setting names doc: Naming convention for new QSettings setting names Apr 25, 2021
@ryanofsky
Copy link
Contributor

Saw a ping on IRC about this, but I don't have strong opinions. Few thoughts:

  • I agree it would be good to have a documented convention for new names, but I don't think the benefits of renaming existing settings would outweigh the costs. Having seamless forward and backward compatibility is user friendly and developer friendly, and whenever there are multiple names for the same thing it can be more confusing than having a single name, even if the name isn't perfect.

  • After interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936 there is no longer any overlap between node and GUI settings. Node settings are stored in bitcoin.conf/settings.json, and GUI settings like window positions are stored in QSettings. Specifically, the following QSettings are all migrated and removed: nDatabaseCache, nThreadsScriptVerif, bSpendZeroConfChange, fUseUPnP, fUseNatpmp, fListen, nPruneSize, bPrune, addrProxy, fUseProxy, addrSeparateProxyTor, fUseSeparateProxyTor

  • Snake convention, camel case, even hungarian notation all seem reasonable here. (In general I don't like hungarian notatation, but a vague ini/conf format that doesn't have a direct way of expressing types is a place where hungarian notation can be useful for self-documentation of settings. I'm thinking of user who might want to tweak the conf file without looking up source or documentation.)

@hebasto
Copy link
Member Author

hebasto commented Apr 26, 2021

@ryanofsky Thanks!

... I don't think the benefits of renaming existing settings would outweigh the costs.

True.

@ryanofsky
Copy link
Contributor

I think you could open a PR that adds a suggested naming convention for new settings to developer notes and maybe optionsmodel comments. Whatever convention you like should probably be fine, and if others prefer something different they can comment in the PR.

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

No branches or pull requests

2 participants