wip: gui: Drop connectSlotsByName usage #18246

The code generated by uic tool adds QObject::connectSlotsByName(), see https://doc.qt.io/qt-5/qmetaobject.html#connectSlotsByName for more information. This can be confirmed with grep connectSlotsByName src/qt/forms/ui*

After #13529 it makes sense to also drop connectSlotsByName usage following the same reasoning - these connections are now in compile time checked.

Also, starting from https://github.com/qt/qtbase/commit/6301d5e51b0c56d09b0bd1cb63f4908c3f8e2c70#diff-d720bd2a996f09262e267c4917e4cc86it is possible to pass --no-autoconnection or -a, but this will take longer since it's only available in recent Qt.

For reference:

--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -355,7 +355,7 @@ bitcoin_qt : qt/bitcoin-qt$(EXEEXT)
 ui_%.h: %.ui [@test](/github-metadata-backup-bitcoin-bitcoin/contributor/test/) -f $(UIC)
        @$(MKDIR_P) $(@D)
-       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(UIC) -o $@ $< || (echo "Error creating $@"; false)
+       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(UIC) -a -o $@ $< || (echo "Error creating $@"; false)

 %.moc: %.cpp
        $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(MOC) $(DEFAULT_INCLUDES) $(QT_INCLUDES) $(MOC_DEFS) $< | \

Review with QT_FATAL_WARNINGS=1.

Seeking concept ACK - this is still incomplete.

Maybe worth adding a lint script to prevent future ThatWidget::on_<obj>_<signal> methods.

Maybe worth adding a lint script to prevent future ThatWidget::on_<obj>_<signal> methods.

... and add a note to the dev docs.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18656 (gui: Add a Make unsigned button next to Send by achow101)
• #18027 ("PSBT Operations" dialog by gwillen)
• #17597 (qt: Fix height of QR-less ReceiveRequestDialog by hebasto)
• #17509 (gui: save and load PSBT by Sjors)
• #15768 (gui: Add close window shortcut by IPGlider)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

~Concept ~0~ See #18246 (comment)

Pros of the current implementation (from Qt docs):

Automatic connection of signals and slots provides both a standard naming convention and an explicit interface for widget designers to work to. By providing source code that implements a given interface, user interface designers can check that their designs actually work without having to write code themselves.

OTOH, QMetaObject::connectSlotsByName() generates the old syntax (see: QTBUG-76375).

I'd prefer to handle ui-form-emitted signals in a distinguishable way, and to have 100% test coverage for them. So I'm ok with auto-connection for such cases.

Here are some refs for the record:

• Using a Designer UI File - Automatic Connections
• QObject Class - Auto-Connection
• QMetaObject::connectSlotsByName()
• QTBUG-76375

@promag

... this is still incomplete.

Mind adding "WIP" marker?

@hebasto thanks for reviewing. WIP added.

I'd prefer to handle ui-form-emitted signals in a distinguishable way, and to have 100% test coverage for them

That sounds more work than replacing with explicit QObject::connect().

I don't agree with those Pros you reference. There's nothing guaranteeing (beside review) the UI doesn't break if a slot is renamed/changed or a UI component is renamed. For this reason it's also dangerous to touch/refactor/cleanup this code.

I can change the approach to keep the slots instead of moving to connect-to-lambda though. Slots could be transformed to camel case and then a linter would prevent the connectSlotsByName slot syntax.

There's nothing guaranteeing (beside review) the UI doesn't break if a slot is renamed/changed or a UI component is renamed.

We could test ui-emitted signals though.

For this reason it's also dangerous to touch/refactor/cleanup this code.

Agree.

I can change the approach to keep the slots instead of moving to connect-to-lambda though.

That was my first thought too. I have no strong opinion about that. It seems we should consider code readability as well.

@hebasto also note that the new uic option --no-autoconnection probably indicates that connectSlotsByName will be deprecated/removed, see https://bugreports.qt.io/browse/QTBUG-76375.

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

I don't agree with those Pros you reference. There's nothing guaranteeing (beside review) the UI doesn't break if a slot is renamed/changed or a UI component is renamed. For this reason it's also dangerous to touch/refactor/cleanup this code.

Concept ACK.

Too soon to spend time working/reviewing on this. I'll reopen once we use Qt6. Thanks for @hebasto.