dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > KoDialog.cpp:161 > > - QObject::connect(button, SIGNAL(clicked()), > - &mButtonSignalMapper, SLOT(map())); > + QObject::connect(button, &QPushButton::clicked, [this, &key] { > q_ptr->slotButtonClicked(key); }); > I bet this crashes at runtime. You're capturing the local variable `key` by reference, so it will have gone out of scope by the time the lambda calls the slot. Remove the `&` to make it a capture by value. > KoFormulaTool.cpp:115 > connect(m_formulaShape->formulaData(), > SIGNAL(dataChanged(FormulaCommand*,bool)), this, > SLOT(updateCursor(FormulaCommand*,bool))); > - connect(m_signalMapper, SIGNAL(mapped(QString)), this, > SLOT(insert(QString))); > + for (const TemplateAction &templateAction : m_templateActions) { > + connect(templateAction.action, &QAction::triggered, [this, > &templateAction] { insert(templateAction.data); }); `qAsConst(m_templateActions)` to avoid a detach > KoFormulaTool.cpp:124 > { > + for (const TemplateAction &templateAction : m_templateActions) { > + disconnect(templateAction.action, &QAction::triggered, nullptr, > nullptr); same here > KoFormulaTool.cpp:134 > //TODO: is this save? > delete m_cursorList[0]; > m_cursorList.removeAt(0); completely unrelated : this those two lines could be combined into `delete m_cursorList.takeAt(0);`, and the comment removed. > SpellCheckMenu.cpp:83 > QAction *action = new QAction(suggestion, m_suggestionsMenu); > - connect(action, SIGNAL(triggered()), m_suggestionsSignalMapper, > SLOT(map())); > - m_suggestionsSignalMapper->setMapping(action, suggestion); > + connect(action, &QAction::triggered, [this, &suggestion] { > replaceWord(suggestion); }); > m_suggestionsMenu->addAction(action); You're capturing `suggestion` by reference, and it's a reference itself. Are you 100% sure that this will point to the m_suggestions list and not to the local variable? OK I did some googling and found that C++11 was unclear about that, C++14 clarified it to be OK. Still, I would just make a copy, so the next reader doesn't have to spend time googling for edge cases ;) > TextTool.cpp:192 > QAction *a = new QAction(factory->title(), this); > - connect(a, SIGNAL(triggered()), signalMapper, SLOT(map())); > - signalMapper->setMapping(a, factory->id()); > + connect(a, &QAction::triggered, [this, &factory] { > startTextEditingPlugin(factory->id()); }); > list.append(a); Remove `&` here too -- reference to local variable, used later = crash. > SimpleCitationBibliographyWidget.cpp:80 > preview->updatePreview(info); > - connect(preview, SIGNAL(pixmapGenerated()), m_signalMapper, > SLOT(map())); > - m_signalMapper->setMapping(preview, index); > + connect(preview, &BibliographyPreview::pixmapGenerated, [this, > &index] { pixmapReady(index); }); > m_previewGenerator.append(preview); you know what I'm going to say ;) + add `this` as 3rd argument > SimpleCitationBibliographyWidget.cpp:110 > widget.addBibliography->addItem(m_chooser, > m_previewGenerator.at(templateId)->previewPixmap(), templateId + 1); > - disconnect(m_previewGenerator.at(templateId), SIGNAL(pixmapGenerated()), > m_signalMapper, SLOT(map())); > + disconnect(m_previewGenerator.at(templateId), > &BibliographyPreview::pixmapGenerated, nullptr, nullptr); > m_previewGenerator.at(templateId)->deleteLater(); This is a bit dangerous, it also disconnects any other possible receiver. You should be able to pass `this` as 3rd arg instead of `nullptr`, if you pass `this` at connect time too. (this applies to all the disconnects in this change request) > SimpleTableOfContentsWidget.cpp:78 > preview->updatePreview(info); > - connect(preview, SIGNAL(pixmapGenerated()), m_signalMapper, > SLOT(map())); > - m_signalMapper->setMapping(preview, index); > + connect(preview, &TableOfContentsPreview::pixmapGenerated, [this, > &index] { pixmapReady(index); }); > m_previewGenerator.append(preview); same Also, add `this` as third argument. > Tool.h:69 > for (QHash<QString, QAction*>::const_iterator it = > actionhash.constBegin(); it != actionhash.constEnd(); ++it) { > - connect(it.value(), SIGNAL(triggered()), m_signalMapper, > SLOT(map())); > - m_signalMapper->setMapping(it.value() , it.key()); > + connect(it.value(), &QAction::triggered, [this, &it] { > actionTriggered(it.key()); }); > } Better capture a local variable called `key` by value. Iterators get invalidated when containers change. REPOSITORY R8 Calligra REVISION DETAIL https://phabricator.kde.org/D25664 To: ognarb, #calligra:_3.0, #kf6, dfaure Cc: dfaure, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, cochise, vandenoever