Replace deprecated QSignalMapper with lambda expressions #2427

Holzhaus · 2020-01-07T17:00:30Z

This fixes a bunch of -Wdeprecated-declarations warnings. Please review thorougly, I'm not that experienced with lambda expressions. ;-)

Holzhaus · 2020-01-07T18:02:55Z

Oh, the CI builds failed because we need at least Qt 5.6 to use lambdas with QMenu::addAction().

src/controllers/controlpickermenu.cpp

src/controllers/dlgprefcontrollers.cpp

src/library/autodj/autodjfeature.cpp

src/widget/wtracktableview.cpp

…essions

…ressions

…expressions

…xpressions

…ssions

…y value

…alue

Holzhaus · 2020-01-09T01:06:54Z

@daschuer I fixed the remaining issues (hopefully) and also deduplicated the controlpickermenu.cpp code a little.

daschuer · 2020-01-10T14:53:32Z

Mmm .. rebasing a 500 lines a PR under review does not work well, because instead of reading your fixes, I now have to read everything again. Rebasing und review should only be done after talking to the reviewer.

daschuer · 2020-01-10T15:42:09Z

Ah, nevermind. It is not too bad. I hope I find time to have a look and manual test.

Holzhaus · 2020-01-10T23:38:27Z

Sorry I just fixed up the last two commits that you didn't review yet anyway.

uklotzde · 2020-01-11T09:13:18Z

IMHO rebasing is a viable option for technical PRs like this one that suffer from our broad range of supported platforms and versions. Otherwise we would need to mark each opened PR by default as work-in-progress until all CI builds succeeded. Without rebasing there would be intermediate versions that don't build successfully.

uklotzde

We should avoid to use std::unique_ptr for objects that are already managed by Qt.

src/controllers/controlpickermenu.cpp

src/controllers/controlpickermenu.h

src/widget/wtracktableview.cpp

daschuer · 2020-01-11T12:41:23Z

.. Without rebasing there would be intermediate versions that don't build successfully.

Yes indeed, that is a very good reason for debasing a PR. In this case I was probably complaining unjustified.

Also, in a case we have only one faulty HEAD commit with no usable change else, a rebase of this single commit is also a good thing. Just keep in mind the extra review effort before deciding for a rebase.
It is finally an issue about good communication ...

uklotzde · 2020-01-12T22:03:49Z

The failure of the Windows build on AppVeyor is unrelated.

uklotzde · 2020-01-13T20:51:49Z

Ping @daschuer

daschuer · 2020-01-13T22:16:34Z

I like to do manual test before merge.
Hopefully I will find time soon.

daschuer

I have added some optional minor comments. The PR looks good. Will test tonight.

src/controllers/controlpickermenu.cpp

daschuer · 2020-01-14T07:15:50Z

src/controllers/controlpickermenu.cpp


    if (addReset) {
-        QString resetMenuTitle = QString("%1 (%2)").arg(controlTitle, m_resetStr);
-        QString resetMenuDescription = QString("%1 (%2)").arg(controlDescription, m_resetStr);
+        QString resetTitle = QString("%1 (%2)").arg(title, m_resetStr);


Here and nelow, we can make use of QStringBuilder and QStringLiteral:

Titel + QStringLiteral (" (") + m_resetStr + QChar(')');

Is this giving more performance? Because I think it's less readable.

Yes, because it does not create the intermediate "%1 (%2)" string from char[] and there is no string parsing for %. But readability is also a high value ... I don't mind.

daschuer · 2020-01-14T20:29:17Z

src/library/autodj/autodjfeature.cpp

-                    [=](bool) { m_crateMapper.map(); });
+            int iCrateId = crate.getId().value();
+            connect(pAction.get(), &QAction::triggered,
+                    this, [this, iCrateId] { slotAddCrateToAutoDj(iCrateId); });


This one does not work anymore ... but why?

Never mind, master is also effected.

I will file a bug

Ups.. It is already there: https://bugs.launchpad.net/mixxx/+bug/1856405

daschuer · 2020-01-14T20:59:43Z

Works and looks good now. Thank you very much.
Thank you for helping to get rid of all string based connect calls.

uklotzde · 2020-01-16T23:49:40Z

Adding tracks to crates through the check boxes in the context menu fails:

#0  0x00007ffff42cd5cc in QMetaObject::cast(QObject const*) const () at /lib64/libQt5Core.so.5
#1  0x00000000006f5ddd in qobject_cast<QCheckBox*>(QObject*) (object=<optimized out>) at /usr/include/qt5/QtCore/qobject.h:503
#2  WTrackTableView::updateSelectionCrates(QWidget*) (this=0x87cb020, pWidget=<optimized out>) at /tmp/mixxx/src/widget/wtracktableview.cpp:1876
#3  0x00007ffff42eead0 in QMetaObject::activate(QObject*, int, int, void**) () at /lib64/libQt5Core.so.5
#4  0x00007ffff6ef9162 in QCheckBox::stateChanged(int) () at /lib64/libQt5Widgets.so.5
#5  0x00007ffff6eea0b0 in QAbstractButtonPrivate::click() () at /lib64/libQt5Widgets.so.5
#6  0x00007ffff6eea2d5 in QAbstractButton::mouseReleaseEvent(QMouseEvent*) () at /lib64/libQt5Widgets.so.5
#7  0x00007ffff6e34c7e in QWidget::event(QEvent*) () at /lib64/libQt5Widgets.so.5
#8  0x00007ffff6df1ab6 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib64/libQt5Widgets.so.5
#9  0x00007ffff6dfb3a3 in QApplication::notify(QObject*, QEvent*) () at /lib64/libQt5Widgets.so.5
#10 0x00007ffff42c4458 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib64/libQt5Core.so.5
#11 0x00007ffff6dfa4b7 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool, bool) () at /lib64/libQt5Widgets.so.5
#12 0x00007ffff6e51614 in QWidgetWindow::handleMouseEvent(QMouseEvent*) () at /lib64/libQt5Widgets.so.5
#13 0x00007ffff6e53c8c in QWidgetWindow::event(QEvent*) () at /lib64/libQt5Widgets.so.5
#14 0x00007ffff6df1ab6 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib64/libQt5Widgets.so.5
#15 0x00007ffff6dfb150 in QApplication::notify(QObject*, QEvent*) () at /lib64/libQt5Widgets.so.5
#16 0x00007ffff42c4458 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib64/libQt5Core.so.5
#17 0x00007ffff66b5b83 in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () at /lib64/libQt5Gui.so.5
#18 0x00007ffff66b737b in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () at /lib64/libQt5Gui.so.5
#19 0x00007ffff66936eb in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib64/libQt5Gui.so.5
#20 0x00007fffe1163d5e in xcbSourceDispatch(_GSource*, int (*)(void*), void*) () at /lib64/libQt5XcbQpa.so.5
#21 0x00007ffff630c510 in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
#22 0x00007ffff630c8a0 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0
#23 0x00007ffff630c943 in g_main_context_iteration () at /lib64/libglib-2.0.so.0
#24 0x00007ffff4319895 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib64/libQt5Core.so.5
#25 0x00007ffff42c324b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /lib64/libQt5Core.so.5
#26 0x00007ffff42cb126 in QCoreApplication::exec() () at /lib64/libQt5Core.so.5
#27 0x000000000054acbd in (anonymous namespace)::runMixxx (args=..., app=0x7fffffffd120) at /tmp/mixxx/src/main.cpp:53
#28 main(int, char**) (argc=<optimized out>, argv=<optimized out>) at /tmp/mixxx/src/main.cpp:130

Maybe a regression? I haven't done a bisect yet.

Holzhaus · 2020-01-17T00:53:03Z

Maybe pCheckbox goes out of scope after being referenced in the lambda in lines 1745/1747 in wtracktableview.cpp. I'll have to check tomorrow if #2451 resolves the issue.

Holzhaus added minor bug needs review labels Jan 7, 2020

Holzhaus force-pushed the remove-qsignalmapper branch from aaa29fa to d38ef93 Compare January 8, 2020 14:06

daschuer requested changes Jan 8, 2020

View reviewed changes

src/controllers/controlpickermenu.cpp Outdated Show resolved Hide resolved

daschuer reviewed Jan 8, 2020

View reviewed changes

src/controllers/dlgprefcontrollers.cpp Outdated Show resolved Hide resolved

daschuer reviewed Jan 8, 2020

View reviewed changes

src/library/autodj/autodjfeature.cpp Outdated Show resolved Hide resolved

src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved

Holzhaus added 23 commits January 9, 2020 00:35

controllers/controlpickermenu: Replace QSignalMapper with lambda expr…

ffd9e0b

…essions

widget/wwidgetstack: Replace QSignalMapper with lambda expressions

e1ec9d0

widget/wwidgetstack: Replace old-style SLOT/SIGNAL connection

49c3efb

controllers/dlgprefcontrollers: Replace QSignalMapper with lambda exp…

d205414

…ressions

effects/effectchainslot: Replace QSignalMapper with lambda expressions

83ed067

errordialoghandler: Replace QSignalMapper with lambda expressions

1640879

widget/wtracktableview: Replace m_BpmMapper with lambda expressions

d35f294

widget/wtracktableview: Remove unused QSignalMapper

a5f51d4

widget/wtracktableview: Replace m_playlistMapper with lambda expressions

cfc1a38

widget/wtracktableview: Replace m_crateMapper with lambda expressions

ac41c16

widget/wtracktableview: Replace m_deckMapper with lambda expressions

5c5034d

widget/wtracktableview: Replace m_samplerMapper with lambda expressions

f264ba3

widget/wmainmenubar: Replace m_loadToDeckMapper with lambda expressions

5421bb1

widget/wmainmenubar: Replace m_visitUrlMapper with lambda expressions

3ff5ce1

widget/wmainmenubar: Replace m_vinylControlEnabledMapper with lambda …

603afe8

…expressions

vinylcontrol/vinylcontrolmanager: Replace QSignalMapper with lambda e…

6c3dedf

…xpressions

widget/wtracktableviewheader: Replace QSignalMapper with lambda expre…

9a99371

…ssions

library/autodj/autodjfeature: Replace QSignalMapper with lambda expre…

ba0639e

…ssions

controllers/controlpickermenu: Use separate connect for QActions

8bbce85

Pass context object as third parameter to connect() call

abcd0da

controllers/controlpickermenu: Pass control indices to lambda exprs b…

76fd846

…y value

controllers/dlgprefcontrollers: Pass presetsPath to lambda exprs by v…

c6bcac5

…alue

library/autodj/autodjfeature: Pass crate id to lambda expression by v…

3d751f8

…alue

Holzhaus requested a review from daschuer January 10, 2020 01:38

uklotzde suggested changes Jan 11, 2020

View reviewed changes

src/controllers/controlpickermenu.cpp Outdated Show resolved Hide resolved

src/controllers/controlpickermenu.h Outdated Show resolved Hide resolved

src/widget/wtracktableview.cpp Outdated Show resolved Hide resolved

Holzhaus added 3 commits January 11, 2020 20:16

controllers/controlpickermenu: Do not use std::unique_ptr for QAction

87eeedc

controllers/controlpickermenu: Use a default null-string QString() param

aaac889

widget/wtracktableview: Pass captured lambda parameters explicitly

068c26e

uklotzde approved these changes Jan 12, 2020

View reviewed changes

uklotzde added this to the 2.3.0 milestone Jan 12, 2020

daschuer reviewed Jan 14, 2020

View reviewed changes

Holzhaus added 3 commits January 14, 2020 11:51

controllers/controlpickermenu: Use parented_ptr for QAction

38bd22e

controllers/controlpickermenu: Make addAvailableControl() return index

8249761

controllers/controlpickermenu: Use parented_ptr for QMenu instances

b5c18f3

daschuer reviewed Jan 14, 2020

View reviewed changes

daschuer merged commit da3c3af into mixxxdj:master Jan 14, 2020

Holzhaus mentioned this pull request Jan 17, 2020

[WIP] widget/wtracktableview: Fix crash in updateSelectionCrates #2451

Merged

Holzhaus mentioned this pull request Jan 31, 2020

Don't use local variables by reference #2475

Merged

Holzhaus added code quality and removed needs review labels Mar 3, 2020

Holzhaus added this to Done in 2.3 release Mar 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated QSignalMapper with lambda expressions #2427

Replace deprecated QSignalMapper with lambda expressions #2427

Holzhaus commented Jan 7, 2020

Holzhaus commented Jan 7, 2020

Holzhaus commented Jan 9, 2020

daschuer commented Jan 10, 2020

daschuer commented Jan 10, 2020

Holzhaus commented Jan 10, 2020 •

edited

Loading

uklotzde commented Jan 11, 2020

uklotzde left a comment

daschuer commented Jan 11, 2020

uklotzde commented Jan 12, 2020

uklotzde commented Jan 13, 2020

daschuer commented Jan 13, 2020

daschuer left a comment

daschuer Jan 14, 2020

Holzhaus Jan 14, 2020

daschuer Jan 14, 2020

daschuer Jan 14, 2020

daschuer Jan 14, 2020

daschuer Jan 14, 2020

daschuer Jan 14, 2020

daschuer commented Jan 14, 2020

uklotzde commented Jan 16, 2020 •

edited

Loading

Holzhaus commented Jan 17, 2020

Replace deprecated QSignalMapper with lambda expressions #2427

Replace deprecated QSignalMapper with lambda expressions #2427

Conversation

Holzhaus commented Jan 7, 2020

Holzhaus commented Jan 7, 2020

Holzhaus commented Jan 9, 2020

daschuer commented Jan 10, 2020

daschuer commented Jan 10, 2020

Holzhaus commented Jan 10, 2020 • edited Loading

uklotzde commented Jan 11, 2020

uklotzde left a comment

Choose a reason for hiding this comment

daschuer commented Jan 11, 2020

uklotzde commented Jan 12, 2020

uklotzde commented Jan 13, 2020

daschuer commented Jan 13, 2020

daschuer left a comment

Choose a reason for hiding this comment

daschuer Jan 14, 2020

Choose a reason for hiding this comment

Holzhaus Jan 14, 2020

Choose a reason for hiding this comment

daschuer Jan 14, 2020

Choose a reason for hiding this comment

daschuer Jan 14, 2020

Choose a reason for hiding this comment

daschuer Jan 14, 2020

Choose a reason for hiding this comment

daschuer Jan 14, 2020

Choose a reason for hiding this comment

daschuer Jan 14, 2020

Choose a reason for hiding this comment

daschuer commented Jan 14, 2020

uklotzde commented Jan 16, 2020 • edited Loading

Holzhaus commented Jan 17, 2020

Holzhaus commented Jan 10, 2020 •

edited

Loading

uklotzde commented Jan 16, 2020 •

edited

Loading