Skip to content

GUI: Visually move and rephrase port mapping checkboxes #197

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: 29.x-knots
Choose a base branch
from
+37 −24
Open

GUI: Visually move and rephrase port mapping checkboxes #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e338aec
Force-disable UPnP and NAT-PMP/PCP if listening is disabled
luke-jr Sep 25, 2025
1d3cb22
GUI/OptionsDialog: Support insert_at position in CreateOptionUI
luke-jr Sep 25, 2025
b16faeb
GUI/OptionsDialog: Move port mapping options indented under allow-inc…
luke-jr Sep 25, 2025
07b3d87
GUI/OptionsDialog: Disable port mapping checkboxes if listening check…
luke-jr Sep 25, 2025
4324e84
GUI/OptionsDialog: Rephrase UPnP & PCP/NAT-PMP checkboxes
luke-jr Sep 25, 2025
49fd570
GUI/OptionsDialog: Refactor UPnP checkbox (move to C++ code)
luke-jr Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,9 +903,12 @@ void InitParameterInteraction(ArgsManager& args)

if (!args.GetBoolArg("-listen", DEFAULT_LISTEN)) {
// 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");
Copy link

Choose a reason for hiding this comment

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

why not false, why "0"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ForceSetArg only accepts strings.

LogInfo("parameter interaction: -listen=0 -> setting -upnp=0\n");
if (args.SoftSetBoolArg("-natpmp", false)) {
}
if (args.GetBoolArg("-natpmp", DEFAULT_NATPMP)) {
args.ForceSetArg("-natpmp", "0");
LogInfo("parameter interaction: -listen=0 -> setting -natpmp=0\n");
}
if (args.SoftSetBoolArg("-discover", false))
Expand Down
12 changes: 1 addition & 11 deletions src/qt/forms/optionsdialog.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,23 +385,13 @@
</item>
</layout>
</item>
<item>
<widget class="QCheckBox" name="mapPortUpnp">
<property name="toolTip">
<string>Automatically open the Bitcoin client port on the router. This only works when your router supports UPnP and it is enabled.</string>
</property>
<property name="text">
<string>Map port using &amp;UPnP</string>
</property>
</widget>
</item>
<item>
<widget class="QCheckBox" name="mapPortNatpmp">
<property name="toolTip">
<string>Automatically open the Bitcoin client port on the router. This only works when your router supports PCP or NAT-PMP and it is enabled. The external port could be random.</string>
</property>
<property name="text">
<string>Map port using PCP or NA&amp;T-PMP</string>
<string>Automatically configure router(s) that support PCP or NA&amp;T-PMP</string>
</property>
</widget>
</item>
Expand Down
33 changes: 24 additions & 9 deletions src/qt/optionsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ void OptionsDialog::FixTabOrder(QWidget * const o)
struct CreateOptionUIOpts {
QBoxLayout *horizontal_layout{nullptr};
int stretch{1};
int insert_at{-1};
int indent{0};
};

Expand Down Expand Up @@ -153,7 +154,7 @@ void OptionsDialog::CreateOptionUI(QBoxLayout * const layout, const QString& tex

if (opts.stretch) horizontalLayout->addStretch(opts.stretch);

layout->addLayout(horizontalLayout);
layout->insertLayout(opts.insert_at, horizontalLayout);

for (auto& o : objs) {
o->setProperty("L", QVariant::fromValue((QLayout*)horizontalLayout));
Expand Down Expand Up @@ -266,10 +267,6 @@ OptionsDialog::OptionsDialog(QWidget* parent, bool enableWallet)
connect(ui->networkPort, SIGNAL(textChanged(const QString&)), this, SLOT(checkLineEdit()));

/* Network elements init */
#ifndef USE_UPNP
ui->mapPortUpnp->setEnabled(false);
#endif

ui->proxyIp->setEnabled(false);
ui->proxyPort->setEnabled(false);
ui->proxyPort->setValidator(new QIntValidator(1, 65535, this));
Expand Down Expand Up @@ -298,8 +295,29 @@ OptionsDialog::OptionsDialog(QWidget* parent, bool enableWallet)
ui->verticalLayout_Wallet->insertWidget(0, walletrbf);
FixTabOrder(walletrbf);

QStyleOptionButton styleoptbtn;
const auto checkbox_indent = ui->allowIncoming->style()->subElementRect(QStyle::SE_CheckBoxIndicator, &styleoptbtn, ui->allowIncoming).width();

/* Network tab */
QLayoutItem *spacer = ui->verticalLayout_Network->takeAt(ui->verticalLayout_Network->count() - 1);

prevwidget = ui->allowIncoming;
ui->verticalLayout_Network->removeWidget(ui->mapPortNatpmp);
int insert_at = ui->verticalLayout_Network->indexOf(ui->connectSocks);
// NOTE: Re-inserted in bottom-to-top order
CreateOptionUI(ui->verticalLayout_Network, QStringLiteral("%1"), {ui->mapPortNatpmp}, { .insert_at=insert_at, .indent=checkbox_indent, });
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."));
#ifndef USE_UPNP
upnp->setEnabled(false);
#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);
Copy link

Choose a reason for hiding this comment

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

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());
Copy link

@bigshiny90 bigshiny90 Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i believe line 318 will override the #ifndef USE_UPNP behavior here - not sure that is the intended behavior.

Copy link

Choose a reason for hiding this comment

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

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

ui->mapPortNatpmp->setEnabled(ui->allowIncoming->isChecked());

prevwidget = dynamic_cast<QWidgetItem*>(ui->verticalLayout_Network->itemAt(ui->verticalLayout_Network->count() - 1))->widget();
Copy link

@bigshiny90 bigshiny90 Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's used in FixTabOrder called from CreateOptionUI. Setting it here ensures the correct tab order is set.


blockreconstructionextratxn = new QSpinBox(ui->tabNetwork);
Expand Down Expand Up @@ -552,9 +570,6 @@ OptionsDialog::OptionsDialog(QWidget* parent, bool enableWallet)
dustdynamic_multiplier->setValue(DEFAULT_DUST_RELAY_MULTIPLIER / 1000.0);
CreateOptionUI(verticalLayout_Spamfiltering, tr("%1 Automatically adjust the dust limit upward to %2 times:"), {dustdynamic_enable, dustdynamic_multiplier});

QStyleOptionButton styleoptbtn;
const auto checkbox_indent = dustdynamic_enable->style()->subElementRect(QStyle::SE_CheckBoxIndicator, &styleoptbtn, dustdynamic_enable).width();

dustdynamic_target = new QRadioButton(groupBox_Spamfiltering);
dustdynamic_target_blocks = new QSpinBox(groupBox_Spamfiltering);
dustdynamic_target_blocks->setMinimum(2);
Expand Down Expand Up @@ -851,7 +866,7 @@ void OptionsDialog::setMapper()

/* Network */
mapper->addMapping(ui->networkPort, OptionsModel::NetworkPort);
mapper->addMapping(ui->mapPortUpnp, OptionsModel::MapPortUPnP);
mapper->addMapping(upnp, OptionsModel::MapPortUPnP);
mapper->addMapping(ui->mapPortNatpmp, OptionsModel::MapPortNatpmp);
mapper->addMapping(ui->allowIncoming, OptionsModel::Listen);
mapper->addMapping(ui->enableServer, OptionsModel::Server);
Expand Down
1 change: 1 addition & 0 deletions src/qt/optionsdialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ private Q_SLOTS:

QCheckBox *walletrbf;

QCheckBox *upnp;
QSpinBox *blockreconstructionextratxn;
QDoubleSpinBox *blockreconstructionextratxnsize;

Expand Down
8 changes: 6 additions & 2 deletions src/qt/optionsmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,13 +851,17 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value, const std::
case MapPortUPnP: // core option - can be changed on-the-fly
if (changed()) {
update(value.toBool());
node().mapPort(value.toBool(), getOption(MapPortNatpmp).toBool());
if (gArgs.GetBoolArg("-listen", DEFAULT_LISTEN)) {
node().mapPort(value.toBool(), getOption(MapPortNatpmp).toBool());
}
}
break;
case MapPortNatpmp: // core option - can be changed on-the-fly
if (changed()) {
update(value.toBool());
node().mapPort(getOption(MapPortUPnP).toBool(), value.toBool());
if (gArgs.GetBoolArg("-listen", DEFAULT_LISTEN)) {
node().mapPort(getOption(MapPortUPnP).toBool(), value.toBool());
}
}
break;
case MinimizeOnClose:
Expand Down
Loading