GUI: Visually move and rephrase port mapping checkboxes #197
Conversation
| CreateOptionUI(ui->verticalLayout_Network, QStringLiteral("%1"), {upnp}, { .insert_at=insert_at, .indent=checkbox_indent, }); | ||
| connect(ui->allowIncoming, &QPushButton::toggled, upnp, &QWidget::setEnabled); | ||
| connect(ui->allowIncoming, &QPushButton::toggled, ui->mapPortNatpmp, &QWidget::setEnabled); | ||
| upnp->setEnabled(ui->allowIncoming->isChecked()); |
There was a problem hiding this comment.
i believe line 318 will override the #ifndef USE_UPNP behavior here - not sure that is the intended behavior.
There was a problem hiding this comment.
line 316 also.. since again, this would allow it to be enabled even if USE_UPNP is 0.
I wonder if it makes sense to SHOW this checkbox if no USE_UPNP... could do something altogether like this:
#ifdef USE_UPNP
upnp = new QCheckBox(ui->tabNetwork);
upnp->setText(tr("Automatically configure router(s) that support &UPnP"));
upnp->setToolTip(tr("Automatically open the Bitcoin client port on the router. This only works when your router supports UPnP and it is enabled."));
CreateOptionUI(ui->verticalLayout_Network, QStringLiteral("%1"), {upnp}, { .insert_at=insert_at, .indent=checkbox_indent, });
connect(ui->allowIncoming, &QCheckBox::toggled, upnp, &QWidget::setEnabled);
upnp->setEnabled(ui->allowIncoming->isChecked());
#endif
| #endif | ||
| CreateOptionUI(ui->verticalLayout_Network, QStringLiteral("%1"), {upnp}, { .insert_at=insert_at, .indent=checkbox_indent, }); | ||
| connect(ui->allowIncoming, &QPushButton::toggled, upnp, &QWidget::setEnabled); | ||
| connect(ui->allowIncoming, &QPushButton::toggled, ui->mapPortNatpmp, &QWidget::setEnabled); |
There was a problem hiding this comment.
lines 316, 317 - allowIncoming is a QCheckBox not a QPushButton. code for the signal works fine of course, but may be better to just use QCheckBox::toggled to be correct.
| upnp->setEnabled(ui->allowIncoming->isChecked()); | ||
| ui->mapPortNatpmp->setEnabled(ui->allowIncoming->isChecked()); | ||
|
|
||
| prevwidget = dynamic_cast<QWidgetItem*>(ui->verticalLayout_Network->itemAt(ui->verticalLayout_Network->count() - 1))->widget(); |
There was a problem hiding this comment.
I know this is NOT code changed by the PR... line 321 - but stared at it for a while as it comes right after a big block of changed code.
first, this code seems potentially very fragile... seems to require assumptions as to what comes previously. I may be wrong here. just jumps out to me as fragile.
plus, doesn't seem to do anything useful here. prevwidget is just set here on line 321, not used, and then set again on line 336 to an explicit widget.
might be out-of-scope for this PR... but me no likey
There was a problem hiding this comment.
It's used in FixTabOrder called from CreateOptionUI. Setting it here ensures the correct tab order is set.
| // do not map ports or try to retrieve public IP when not listening (pointless) | ||
| if (args.SoftSetBoolArg("-upnp", false)) | ||
| if (args.GetBoolArg("-upnp", DEFAULT_UPNP)) { | ||
| args.ForceSetArg("-upnp", "0"); |
There was a problem hiding this comment.
ForceSetArg only accepts strings.
Also includes a behaviour change: if listening is disabled, port mapping is forced off too.