| nicolasfella | |
| albertvaka |
| KDE Connect |
The most serious change from this patch is to move the asynchronous replying to a request from the app for more messages to a newly-spawned, self-destructing thread. Within that thread, we block until the remote device replies with the requested messages.
All gotten messages are cached in the ConversationDbusInterface, so all future requests are fast and don't hit the remote device.
After applying this diff, the messaging app should show 10 messages every time it is opened
| Lint OK |
| No Unit Test Coverage |
| Buildable 5976 | |
| Build 5994: arc lint + arc unit |
Looks ok to me
show all messages in the conversation when the conversation is first loaded
What's your future plan wrt this?
I don't have a good plan yet... This is the main problem I was trying to fix by making the interface return an array
We could make requestConversation block on a condition variable which is unblocked by some call in addMessages, then only send conversationUpdated for new messages and the actually-requested messages. I just barely thought of this, but it sounds like it's worth trying.
Another thing to try is to add request window size to the Android interface like I have always been planning to do. I want to have this anyway so it would not be a waste of effort. Then, we request only and exactly the messages we want, and conversationUpdated only gets called with the messages we asked for.
The reason this plan is less good is because I was hoping to use the window size to pre-fill the cache in ConversationDbusInterface, thus reduce visible delays for the user
If none of that works, we could make ConvresationModel keep track of which messages it has requested and only display those. That is a bit of a hack...
I also got carried away and changed all QString conversationIDs to qint32. It was too much of a mess to un-tangle by the time I was finished... Sorry!
| plugins/sms/requestconversationworker.h | ||
|---|---|---|
| 35 | We still need something to solve the same problem as the QEventLoop was trying to solve. That is, we need some way to wait until the remote (Android) device replies with the messages we requested, otherwise we don't have anything to return to the user request and the GUI remains annoyingly empty until they make a second request. If we wait on the main thread, the event loop can't process the incoming packet, so we can never stop waiting This could be done by inheriting from QThread, but the Qt people are very firm that inheriting from QThread to do a simple task is the wrong way to do it. The way I have done it is the recommended way | |
With some time to think about this, I think I can combine some of Aleix's last comment. Rather than having the worker inherit from QThread, and rather than implicitly expecting the caller to know a QThread is required, the worker could have a QThread which it is in charge of creating, using, and destroying. I think this would make things much nicer.
I would be okay with accepting this patch. I wonder why it's better than the previous code though. It seems to be just doing the same but from another thread?
By previous code, do you mean the earlier version of this patch or the stuff currently on the master branch?
In case of comparing to the earlier version, it is basically the same, but no new threads have been created. The work in the constructor (even after this->moveToThread(..)) is all done on the main thread. Future signals delivered to the Worker object are handled on the new thread.
In case of comparing to the master branch, the new thread is doing the same work, but it also blocks (in ConversationsDbusInterface::updateConversation) while it waits for messages to be delivered from the remote device. The alternatives to blocking are:
I get some compile errors, apparently you need to explicitly covert int to QString
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp: In member function 'void ConversationModel::sendReplyToConversation(const QString&)':
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:79:51: error: invalid user-defined conversion from 'qint32' {aka 'int'} to 'const QString&' [-fpermissive]
m_conversationsInterface->replyToConversation(m_threadId, message);
^~~~~~~~~~
In file included from /usr/include/qt/QtCore/qhashfunctions.h:44,
from /usr/include/qt/QtCore/qlist.h:47,
from /usr/include/qt/QtCore/qvariant.h:45,
from /usr/include/qt/QtCore/qabstractitemmodel.h:43,
from /usr/include/qt/QtGui/qstandarditemmodel.h:44,
from /usr/include/qt/QtGui/QStandardItemModel:1,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.h:25,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:22:
/usr/include/qt/QtCore/qstring.h:693:31: note: candidate is: 'QString::QString(const char*)' <near match>
inline QT_ASCII_CAST_WARN QString(const char *ch)
^~~~~~~
/usr/include/qt/QtCore/qstring.h:693:31: note: conversion of argument 1 would be ill-formed:
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:79:51: error: invalid conversion from 'qint32' {aka 'int'} to 'const char*' [-fpermissive]
m_conversationsInterface->replyToConversation(m_threadId, message);
^~~~~~~~~~
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:79:51: error: invalid conversion from 'qint32' {aka 'int'} to 'const char*' [-fpermissive]
In file included from /usr/include/qt/QtCore/qhashfunctions.h:44,
from /usr/include/qt/QtCore/qlist.h:47,
from /usr/include/qt/QtCore/qvariant.h:45,
from /usr/include/qt/QtCore/qabstractitemmodel.h:43,
from /usr/include/qt/QtGui/qstandarditemmodel.h:44,
from /usr/include/qt/QtGui/QStandardItemModel:1,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.h:25,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:22:
/usr/include/qt/QtCore/qstring.h:693:51: note: initializing argument 1 of 'QString::QString(const char*)'
inline QT_ASCII_CAST_WARN QString(const char *ch)
~~~~~~~~~~~~^~
In file included from /home/nico/workspace/kdeconnect-kde/interfaces/dbusinterfaces.h:39,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.h:29,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:22:
/home/nico/workspace/kdeconnect-kde/interfaces/conversationsinterface.h:45:32: note: initializing argument 1 of 'QDBusPendingReply<> OrgKdeKdeconnectDeviceConversationsInterface::replyToConversation(const QString&, const QString&)'
inline QDBusPendingReply<> replyToConversation(const QString &conversationID, const QString &message)
^~~~~~~~~~~~~~~~~~~
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp: In member function 'void ConversationModel::requestMoreMessages(const quint32&)':
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:88:51: error: invalid user-defined conversion from 'qint32' {aka 'int'} to 'const QString&' [-fpermissive]
m_conversationsInterface->requestConversation(m_threadId, numMessages, numMessages + howMany);
^~~~~~~~~~
In file included from /usr/include/qt/QtCore/qhashfunctions.h:44,
from /usr/include/qt/QtCore/qlist.h:47,
from /usr/include/qt/QtCore/qvariant.h:45,
from /usr/include/qt/QtCore/qabstractitemmodel.h:43,
from /usr/include/qt/QtGui/qstandarditemmodel.h:44,
from /usr/include/qt/QtGui/QStandardItemModel:1,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.h:25,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:22:
/usr/include/qt/QtCore/qstring.h:693:31: note: candidate is: 'QString::QString(const char*)' <near match>
inline QT_ASCII_CAST_WARN QString(const char *ch)
^~~~~~~
/usr/include/qt/QtCore/qstring.h:693:31: note: conversion of argument 1 would be ill-formed:
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:88:51: error: invalid conversion from 'qint32' {aka 'int'} to 'const char*' [-fpermissive]
m_conversationsInterface->requestConversation(m_threadId, numMessages, numMessages + howMany);
^~~~~~~~~~
/home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:88:51: error: invalid conversion from 'qint32' {aka 'int'} to 'const char*' [-fpermissive]
In file included from /usr/include/qt/QtCore/qhashfunctions.h:44,
from /usr/include/qt/QtCore/qlist.h:47,
from /usr/include/qt/QtCore/qvariant.h:45,
from /usr/include/qt/QtCore/qabstractitemmodel.h:43,
from /usr/include/qt/QtGui/qstandarditemmodel.h:44,
from /usr/include/qt/QtGui/QStandardItemModel:1,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.h:25,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:22:
/usr/include/qt/QtCore/qstring.h:693:51: note: initializing argument 1 of 'QString::QString(const char*)'
inline QT_ASCII_CAST_WARN QString(const char *ch)
~~~~~~~~~~~~^~
In file included from /home/nico/workspace/kdeconnect-kde/interfaces/dbusinterfaces.h:39,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.h:29,
from /home/nico/workspace/kdeconnect-kde/smsapp/conversationmodel.cpp:22:
/home/nico/workspace/kdeconnect-kde/interfaces/conversationsinterface.h:58:32: note: initializing argument 1 of 'QDBusPendingReply<> OrgKdeKdeconnectDeviceConversationsInterface::requestConversation(const QString&, int, int)'
inline QDBusPendingReply<> requestConversation(const QString &conversationID, int start, int end)Thanks for looking in to this! It looks like you might need to do a make clean -- The particular error you're getting looks like the dbus interface might not have been re-built compared to the current version on master
The patch is kind of a mess right now because there have been so many changes to master. I will rebase it when I have time