D25664: [WIP]: Port away from deprecated QSignalMapper

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20191201/571b20b2/attachment.htm>