D22910: Replace obsolete QSignalMapper and qSort with non-obsolete code

Sebastian Gottfried noreply at phabricator.kde.org
Mon Aug 5 09:29:06 BST 2019


gottfried requested changes to this revision.
gottfried added a comment.
This revision now requires changes to proceed.


  Looks mostly good and works as intended. But its seems you have missed the member declarations and include directives:
  
    ktouch/src$ grep -r "QSignalMapper"
    models/lessonmodel.cpp:#include <QSignalMapper>
    models/charactersmodel.h:class QSignalMapper;
    models/charactersmodel.h:    QSignalMapper* m_signalMapper;
    models/resourcemodel.h:class QSignalMapper;
    models/resourcemodel.h:    QSignalMapper* m_signalMapper;
    models/resourcemodel.cpp:#include <QSignalMapper>
    models/charactersmodel.cpp:#include <QSignalMapper>
    models/lessonmodel.h:class QSignalMapper;
    models/lessonmodel.h:    QSignalMapper* m_signalMapper;
    core/course.cpp:#include <QSignalMapper>
    core/keyboardlayout.h:class QSignalMapper;
    core/keyboardlayout.h:    QSignalMapper* m_signalMapper;
    core/course.h:class QSignalMapper;
    core/course.h:    QSignalMapper* m_signalMapper;
    core/keyboardlayout.cpp:#include <QSignalMapper>
  
  Can you please remove these?

INLINE COMMENTS

> lessonmodel.cpp:152
>      {
> -        m_signalMapper->setMapping(m_course->lesson(i), i);
> +        connect(m_course->lesson(i), &Lesson::titleChanged, this, [=] { emitLessonChanged(i); });
> +        connect(m_course->lesson(i), &Lesson::newCharactersChanged, this, [=] { emitLessonChanged(i); });

`updateMappings` is called multiple times. If I am not mistaken, this will result in multiple connections to the lambda with different values of `ì` of you don't `disconnect()` first.

REPOSITORY
  R336 KTouch

REVISION DETAIL
  https://phabricator.kde.org/D22910

To: yurchor, #kde_edu, gottfried
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190805/e50b7335/attachment.html>


More information about the kde-edu mailing list