-
Notifications
You must be signed in to change notification settings - Fork 287
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
Toggle chain from menu (using Settings) #414
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -564,6 +564,10 @@ int GuiMain(int argc, char* argv[]) | |
// - QSettings() will use the new application name after this, resulting in network-specific settings | ||
// - Needs to be done before createOptionsModel | ||
|
||
QSettings settings; | ||
std::string chain = settings.value("chain", "main").toString().toStdString(); | ||
gArgs.SoftSetArg("-chain", chain); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In commit "gui: toggle chain via menu" (12431e0) It might be good to show a message on startup the first time bitcoin is started after switching networks. The toggle chain menu could set a boolean "switched_chain" QSetting, and this code could check for the "switched_chain" setting, clear it, and show a message like "Bitcoin Core is connecting to new network {network}. Use the "Switch chain" menu to switch networks again." (It could be a dialog with a "Do not show this message in the future checkbox" if users switch network repeatedly.) But I think this would provide a useful alert and a good second chance to recover if someone clicked through the first dialog not expecting bitcoin to be starting from scratch again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #414 (comment)
Replying to myself, maybe a better startup message would be: "Bitcoin-Qt configuration has changed and will now connect to the {Testnet} network instead of the {Mainnet} network. {Mainnet} wallets will not be loaded while connected to the {Testnet} network. Also {Testnet} coins are separate from actual bitcoins, and are not supposed to have any value." Dialog could include a checkbox "Do not show this message again," and buttons "Continue connecting to {Testnet} network" / "Exit and switch back to {Mainnet network}" for a safe escape. (And again I really think ideally none of these dialogs would be necessary and new networks would just open in new windows without closing existing ones. But showing clear messages could at least avoid pitfalls if only one window can be open at a time) |
||
|
||
// Check for chain settings (Params() calls are only valid after this clause) | ||
try { | ||
SelectParams(gArgs.GetChainName()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -454,6 +454,46 @@ void BitcoinGUI::createMenuBar() | |
appMenuBar = menuBar(); | ||
#endif | ||
|
||
// Add chain selection submenu | ||
QAction* chain_selection_action{nullptr}; | ||
QMenu* chain_selection_menu{nullptr}; | ||
chain_selection_action = new QAction(tr("&Switch chain"), this); | ||
chain_selection_action->setStatusTip(tr("Restart application using a different (test) network")); | ||
chain_selection_menu = new QMenu(this); | ||
|
||
connect(chain_selection_menu, &QMenu::aboutToShow, [this, chain_selection_menu] { | ||
chain_selection_menu->clear(); | ||
const std::vector<std::pair<QString, QString>> chains = {{"main", "&Bitcoin"}, {"test", "&Testnet"}, {"regtest", "&Regtest"}, {"signet", "&Signet"}}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In commit "gui: toggle chain via menu" (12431e0) Interesting the text is is "Bitcoin" instead of "Mainnet". I think that it is good because it implies the other networks are not the Bitcoin network. Another way to do this while being a little more consistent could be to make the choices "Mainnet (Bitcoin)" / "Testnet" / "Regtest" / "Signet" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noted this as well - IMO "Bitcoin" is a better way to go - "Mainnet" may actually confuse new users at this point. |
||
for (auto chain : chains) { | ||
const bool is_current = gArgs.GetChainName() == chain.first.toStdString(); | ||
QAction* action = chain_selection_menu->addAction(chain.second); | ||
action->setCheckable(true); | ||
action->setChecked(is_current); | ||
action->setEnabled(!is_current); | ||
|
||
connect(action, &QAction::triggered, [this, chain] { | ||
//: Switch to the mainnet, testnet, signet or regtest chain. | ||
QMessageBox::StandardButton btn_ret_val = QMessageBox::question(this, tr("Switch chain"), | ||
//: Switching between mainnet, testnet, signet or regtest chain requires a restart. | ||
tr("Client restart required to switch chain.\n\nClient will be shut down. Do you want to proceed?"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In commit "gui: toggle chain via menu" (12431e0) I think it would be good for the dialog to describe what the option actually does. I'm concerned that someone could click this without really understanding it, and then next time they start bitcoin they find their wallets are missing and bitcoin is doing a new sync to a different network. I think ideally dialog would say something like. "Are you sure you want to switch networks from main to test? Testnet coins are separate from actual bitcoins, and are not supposed to have any value. Switching networks |
||
QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel); | ||
|
||
if (btn_ret_val == QMessageBox::Cancel) return; | ||
|
||
// QSettings are stored seperately for each network. Switch application name | ||
// to mainnet before storing the selected chain. | ||
QScopedPointer<const NetworkStyle> networkStyle(NetworkStyle::instantiate("main")); | ||
assert(!networkStyle.isNull()); | ||
QApplication::setApplicationName(networkStyle->getAppName()); | ||
|
||
QSettings settings; | ||
settings.setValue("chain", chain.first); | ||
Q_EMIT quitRequested(); | ||
}); | ||
} | ||
}); | ||
chain_selection_action->setMenu(chain_selection_menu); | ||
|
||
// Configure the menus | ||
QMenu *file = appMenuBar->addMenu(tr("&File")); | ||
if(walletFrame) | ||
|
@@ -483,6 +523,7 @@ void BitcoinGUI::createMenuBar() | |
settings->addSeparator(); | ||
} | ||
settings->addAction(optionsAction); | ||
settings->addAction(chain_selection_action); | ||
|
||
QMenu* window_menu = appMenuBar->addMenu(tr("&Window")); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "gui: toggle chain via menu" (12431e0)
I think it would be better if these notes were clearer about what behavior is. What happens if you don't follow the advice? Is this saying if you run use -testnet the parameter will be silently ignored because the GUI overrides it internally?
If so, I think this might rise to the level of a bug because command line arguments by design are supposed to override GUI settings and other configuration sources. If not, and there is an explicit error about conflicting settings, that is probably ok. But I think ideally GUI setting would still be overridden if -chain or -testnet or -signet or -regtest were specified. It seems like it should be trivial to implement that instead of requiring users to read this and change their setups.