[WIP]: Port away from deprecated QSignalMapper

Authored by ognarb on Dec 1 2019, 9:22 PM.



Most of the cases are pretty straight forward. The exception was KoFormulaTool

WIP since there are still some instance of QSignalMapper

Test Plan

Change in KoFormulaTool were manually tested

Diff Detail

R8 Calligra
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
ognarb created this revision.Dec 1 2019, 9:22 PM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptDec 1 2019, 9:22 PM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald Transcript
ognarb requested review of this revision.Dec 1 2019, 9:22 PM
ognarb edited the summary of this revision. (Show Details)Dec 1 2019, 9:24 PM
ognarb edited the test plan for this revision. (Show Details)
ognarb added reviewers: Calligra: 3.0, KF6.
ognarb retitled this revision from [WIP]: Port away from QSignalMapper to [WIP]: Port away from deprecated QSignalMapper.Dec 1 2019, 9:47 PM
ognarb edited the summary of this revision. (Show Details)
dfaure requested changes to this revision.Dec 1 2019, 10:55 PM
dfaure added a subscriber: dfaure.
dfaure added inline comments.

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.


qAsConst(m_templateActions) to avoid a detach


same here


completely unrelated : this those two lines could be combined into delete m_cursorList.takeAt(0);, and the comment removed.


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 ;)


Remove & here too -- reference to local variable, used later = crash.


you know what I'm going to say ;)

+ add this as 3rd argument


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)



Also, add this as third argument.


Better capture a local variable called key by value.

Iterators get invalidated when containers change.

This revision now requires changes to proceed.Dec 1 2019, 10:55 PM
ognarb updated this revision to Diff 70755.Dec 2 2019, 2:23 PM
ognarb marked 7 inline comments as done.

Thanks for the review David. I will now follow your advice and continue porting
away from QSignalMapper.

ognarb planned changes to this revision.Dec 2 2019, 2:24 PM

Planned changes: Port all the QSignalMapper ;)

danders added a subscriber: danders.Dec 2 2019, 2:29 PM

Afaik we still support qt5.3, I don't think qAsConst is availabel.
Disregard if this has changed.

dfaure added a comment.Dec 2 2019, 3:19 PM

Then write your own, it's easy:

// this adds const to non-const objects (like std::as_const)
template <typename T> Q_DECL_CONSTEXPR typename std::add_const<T>::type &koAsConst(T &t) noexcept { return t; }
// prevent rvalue arguments:
template <typename T> void koAsConst(const T &&) = delete;
ognarb added a comment.Dec 2 2019, 3:30 PM

Ok I will replace the qAsConst with koAsConst ;)

I suppose the reason why Calligra needs Qt5.3 is the Qt Quick 1 support?

ognarb updated this revision to Diff 70790.Dec 2 2019, 10:39 PM

Finish porting away from QSignalMapper

ognarb marked 3 inline comments as done.Dec 3 2019, 12:51 AM
dfaure accepted this revision as: dfaure.Dec 3 2019, 8:36 AM
dfaure accepted this revision.
This revision is now accepted and ready to land.Dec 3 2019, 8:36 AM
This revision was automatically updated to reflect the committed changes.