Page MenuHomePhabricator

Qt/GUI only: Migrate GUI settings from Bitcoin Core's settings file to our own file
ClosedPublic

Authored by CCulianu on Aug 31 2017, 08:14.

Details

Summary

This diff finishes up the transition to the Qt GUI app name to 'BitcoinABC'.

Since Qt stores settings files based on Qt App name and organization name,
changing the app name to BitcoinABC and the organization to 'bitcoincash.org'
has the side-effect of making the GUI Qt app use a different settings file
location on all platforms.

Since this can produce unexpected results for users upgrading from
Bitcoin ABC 0.14.6 (or perhaps even Core), I added a settings migration
mechanism which copies settings from Core/BitcoinABC-0.14.6 -> our new
settings location *only* if settings exist in the old location and not
in the new location.

Note that this is for GUI settings only and doesn't affect things like
the conf file -- although the GUI does have its own concept of where the
data dir is which basically overrides the conf file spec.

Note that this diff basically makes the GUI settings be BitcoinABC
specific and as such bitcoin classic or BU GUI settings will not be
shared anymore, if they ever were (I actually don't know if classic or BU
use Core's old GUI settings file location -- I presume they don't!).

Test Plan

I was unable to write a test case for this as the code in question
doesn't even get compiled in test setup. What I did was I compiled the app
for OSX, Linux, and Windows and verified that settings were copied correctly
to the new settings file locations/registry keys and that settings were
never erroneously copied or clobbered.

The logic in this diff is fairly trivial so I don't think it necessarily
warrants a test case anyway considering the great difficulty involved in
mutating the test suite to support testing this.

If you like, build the GUI yourself first without this diff to produce
a settings file/registry entry then build it with this diff and see
the settings got copied over.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Bah. Arcanist! Why do you vex me?! Fixed with a rebase.

Overall it looks good. We want to make sure we make the right choice for the new configs, however.

src/qt/bitcoin.cpp
575 ↗(On Diff #1287)

Use C++11 style for loops.

src/qt/guiconstants.h
52 ↗(On Diff #1287)

Why not bitcoinabc.org ? If we are using bitcoincash, then we probably want to sync with other implementations.

src/qt/bitcoin.cpp
575 ↗(On Diff #1287)

OK.

src/qt/guiconstants.h
52 ↗(On Diff #1287)

We can use whatever -- it hardly matters much.

I don't understand your comment about "syncing with other implementations" -- really the file will be unique because it takes QAPP_APP_NAME_DEFAULT to create a unique file. No synching needed.

Or do you mean we should *all* use bitcoincash.org or none of us?

It's up to you.. this is your baby.

Modified to use C++11 ranged for loop

Updated all app bundle-related names to be bitoinabc.org and not bitcoincash.org
Bitcoincash is the protocol. The app org name is bitcoinabc.

Oops -- QAPP_ORG_NAME "BitcoinCash" -> "BitcoinABC"

This revision is now accepted and ready to land.Aug 31 2017, 18:27
This revision was automatically updated to reflect the committed changes.