wip: gui: Drop connectSlotsByName usage

promag commented at 7:00 pm on March 2, 2020: member

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:

0--- a/src/Makefile.qt.include
1+++ b/src/Makefile.qt.include
2@@ -355,7 +355,7 @@ bitcoin_qt : qt/bitcoin-qt$(EXEEXT)
3 ui_%.h: %.ui [@test](/bitcoin-bitcoin/contributor/test/) -f $(UIC)
4        @$(MKDIR_P) $(@D)
5-       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(UIC) -o $@ $< || (echo "Error creating $@"; false)
6+       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(UIC) -a -o $@ $< || (echo "Error creating $@"; false)
7
8 %.moc: %.cpp
9        $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(MOC) $(DEFAULT_INCLUDES) $(QT_INCLUDES) $(MOC_DEFS) $< | \

Review with QT_FATAL_WARNINGS=1.

gui: Drop connectSlotsByName usage from HelpMessageDialog b67053534a

promag commented at 7:01 pm on March 2, 2020: member

Seeking concept ACK - this is still incomplete.

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

gui: Drop connectSlotsByName usage from SendCoinsEntry bd461fc518

gui: Drop connectSlotsByName usage from SendCoinsDialog 9bebf17037

gui: Drop connectSlotsByName usage from ReceiveRequestDialog 583b062418

gui: Drop connectSlotsByName usage from Intro 750ce5c76c

promag force-pushed on Mar 2, 2020

fanquake added the label GUI on Mar 2, 2020

hebasto commented at 8:57 pm on March 2, 2020: member

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

… and add a note to the dev docs.

DrahtBot commented at 10:19 pm on March 2, 2020: member

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

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.

hebasto commented at 10:16 am on March 3, 2020: member

~~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:

@promag

… this is still incomplete.

Mind adding “WIP” marker?

promag renamed this:
~~gui: Drop connectSlotsByName usage~~
wip: gui: Drop connectSlotsByName usage
on Mar 3, 2020

promag commented at 10:44 am on March 3, 2020: member

@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.

hebasto commented at 10:57 am on March 3, 2020: member

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.

promag commented at 11:20 am on March 3, 2020: member

@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.

DrahtBot added the label Needs rebase on Apr 23, 2020

DrahtBot commented at 2:46 am on April 23, 2020: member

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

hebasto commented at 3:03 am on May 3, 2020: member

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.

promag commented at 1:53 pm on May 5, 2020: member

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

promag closed this on May 5, 2020

promag deleted the branch on May 5, 2020

DrahtBot locked this on Aug 16, 2022

wip: gui: Drop connectSlotsByName usage #18246

Conflicts