[SMS App] Make sms app not crash when conversation is selected with no devices connected
Summary: This patch fixes T10184 and stops the SMS app from crashing when a conversation is selected but no devices are connected. It also allows the SMS app to access the cached messages in the ConversationsDbusInterface so the app is still slightly useful even when the device is disconnected. Test Plan: - Open sms app - Open a few conversations - Disconnect phone (Force close app?) - Re-open a conversation which was previously opened - Verify that the messages appear. It is possible to scroll up to view any older cached messages too! - Open a conversation which was not opened previously - Verify that a single messages is shown (since this was the only one in cache, from populating the list of all conversations) - Verify that attempting to scroll this conversation does nothing, but also does not crash the app Note: Opening the app with no phone connected will cause it to lose its handle on the deviceId, so it can't spawn a new Dbus interface, so it will remain blank and empty. Solving that is a project for another day. Reviewers: #kde_connect Reviewed By: #kde_connect Subscribers: apol, nicolasfella, kdeconnect Tags: #kde_connect Maniphest Tasks: T10184 Differential Revision: https://phabricator.kde.org/D17634
This commit is contained in:
Simon Redman
parent
74ba660cad
commit
a14b39d541
6 changed files with 77 additions and 22 deletions
|
|
@ -43,6 +43,12 @@ ConversationsDbusInterface::ConversationsDbusInterface(KdeConnectPlugin* plugin)
|
|||
|
||||
ConversationsDbusInterface::~ConversationsDbusInterface()
|
||||
{
|
||||
// Wake all threads which were waiting for a reply from this interface
|
||||
// This might result in some noise on dbus, but it's better than leaking a bunch of resources!
|
||||
waitingForMessagesLock.lock();
|
||||
conversationsWaitingForMessages.clear();
|
||||
waitingForMessages.wakeAll();
|
||||
waitingForMessagesLock.unlock();
|
||||
}
|
||||
|
||||
QVariantList ConversationsDbusInterface::activeConversations()
|
||||
|
|
@ -130,6 +136,12 @@ QList<ConversationMessage> ConversationsDbusInterface::getConversation(const qin
|
|||
void ConversationsDbusInterface::updateConversation(const qint64& conversationID)
|
||||
{
|
||||
waitingForMessagesLock.lock();
|
||||
if (conversationsWaitingForMessages.contains(conversationID)) {
|
||||
// This conversation is already being waited on, don't allow more than one thread to wait at a time
|
||||
qCDebug(KDECONNECT_CONVERSATIONS) << "Not allowing two threads to wait for conversationID" << conversationID;
|
||||
waitingForMessagesLock.unlock();
|
||||
return;
|
||||
}
|
||||
qCDebug(KDECONNECT_CONVERSATIONS) << "Requesting conversation with ID" << conversationID << "from remote";
|
||||
conversationsWaitingForMessages.insert(conversationID);
|
||||
m_smsInterface.requestConversation(conversationID);
|
||||
|
|
|
|||
|
|
@ -28,10 +28,12 @@ RequestConversationWorker::RequestConversationWorker(const qint64& conversationI
|
|||
//QObject(interface)
|
||||
conversationID(conversationID)
|
||||
, start(start)
|
||||
, end(end)
|
||||
, parent(interface)
|
||||
, m_thread(new QThread)
|
||||
{
|
||||
Q_ASSERT(end >= start && "Not allowed to have a negative-length range");
|
||||
howMany = end - start;
|
||||
|
||||
this->moveToThread(m_thread);
|
||||
connect(m_thread, &QThread::started,
|
||||
this, &RequestConversationWorker::handleRequestConversation);
|
||||
|
|
@ -52,31 +54,39 @@ void RequestConversationWorker::handleRequestConversation()
|
|||
qCWarning(KDECONNECT_CONVERSATIONS) << "Got a conversationID for a conversation with no messages!" << conversationID;
|
||||
}
|
||||
|
||||
// TODO: Reply with all messages we currently have available, even if we don't have enough to completely fill the request
|
||||
// In case the remote takes awhile to respond, we should go ahead and do anything we can from the cache
|
||||
size_t numHandled = replyForConversation(messagesList, start, howMany);
|
||||
|
||||
if (messagesList.length() <= end) {
|
||||
if (numHandled < howMany) {
|
||||
size_t numRemaining = howMany - numHandled;
|
||||
// If we don't have enough messages in cache, go get some more
|
||||
// TODO: Make Android interface capable of requesting small window of messages
|
||||
parent->updateConversation(conversationID);
|
||||
messagesList = parent->getConversation(conversationID);
|
||||
}
|
||||
|
||||
// Messages are sorted in ascending order of keys, meaning the front of the list has the oldest
|
||||
// messages (smallest timestamp number)
|
||||
// Therefore, return the end of the list first (most recent messages)
|
||||
int i = start;
|
||||
for(auto it = messagesList.crbegin() + start; it != messagesList.crend(); ++it) {
|
||||
Q_EMIT conversationMessageRead(it->toVariant());
|
||||
i++;
|
||||
if (i >= end) {
|
||||
break;
|
||||
}
|
||||
//ConversationsDbusInterface::getConversation blocks until it sees new messages in the requested conversation
|
||||
replyForConversation(messagesList, start + numHandled, numRemaining);
|
||||
}
|
||||
|
||||
Q_EMIT finished();
|
||||
}
|
||||
|
||||
size_t RequestConversationWorker::replyForConversation(const QList<ConversationMessage>& conversation, int start, size_t howMany) {
|
||||
Q_ASSERT(start >= 0);
|
||||
// Messages are sorted in ascending order of keys, meaning the front of the list has the oldest
|
||||
// messages (smallest timestamp number)
|
||||
// Therefore, return the end of the list first (most recent messages)
|
||||
int i = 0;
|
||||
for(auto it = conversation.crbegin() + start; it != conversation.crend(); ++it) {
|
||||
if (i >= howMany) {
|
||||
break;
|
||||
}
|
||||
Q_EMIT conversationMessageRead(it->toVariant());
|
||||
i++;
|
||||
}
|
||||
|
||||
return i;
|
||||
}
|
||||
|
||||
void RequestConversationWorker::work()
|
||||
{
|
||||
m_thread->start();
|
||||
|
|
|
|||
|
|
@ -41,6 +41,13 @@ public:
|
|||
|
||||
|
||||
public Q_SLOTS:
|
||||
/**
|
||||
* Main body of this worker
|
||||
*
|
||||
* Reply to a request for messages and, if needed, wait for the remote to reply with more
|
||||
*
|
||||
* Emits conversationMessageRead for every message handled
|
||||
*/
|
||||
void handleRequestConversation();
|
||||
void work();
|
||||
|
||||
|
|
@ -50,11 +57,24 @@ Q_SIGNALS:
|
|||
|
||||
private:
|
||||
qint64 conversationID;
|
||||
int start;
|
||||
int end;
|
||||
int start; // Start of range to request messages
|
||||
size_t howMany; // Number of messages being requested
|
||||
ConversationsDbusInterface* parent;
|
||||
|
||||
QThread* m_thread;
|
||||
|
||||
/**
|
||||
* Reply with all messages we currently have available in the requested range
|
||||
*
|
||||
* If the range specified by start and howMany is not in the range of messages in the conversation,
|
||||
* reply with only as many messages as we have available in that range
|
||||
*
|
||||
* @param conversation Conversation to send messages from
|
||||
* @param start Start of requested range, 0-indexed, inclusive
|
||||
* @param howMany Maximum number of messages to return
|
||||
* $return Number of messages processed
|
||||
*/
|
||||
size_t replyForConversation(const QList<ConversationMessage>& conversation, int start, size_t howMany);
|
||||
};
|
||||
|
||||
#endif // REQUESTCONVERSATIONWORKER_H
|
||||
|
|
|
|||
|
|
@ -53,6 +53,10 @@ void ConversationListModel::setDeviceId(const QString& deviceId)
|
|||
return;
|
||||
}
|
||||
|
||||
if (deviceId == "") {
|
||||
return;
|
||||
}
|
||||
|
||||
qCDebug(KDECONNECT_SMS_CONVERSATIONS_LIST_MODEL) << "setDeviceId" << deviceId << "of" << this;
|
||||
|
||||
if (m_conversationsInterface) {
|
||||
|
|
|
|||
|
|
@ -34,7 +34,9 @@ Kirigami.ScrollablePage
|
|||
readonly property QtObject person: PersonData {
|
||||
id: person
|
||||
}
|
||||
property QtObject device
|
||||
|
||||
property bool deviceConnected
|
||||
property string deviceId
|
||||
property int conversationId
|
||||
|
||||
property string phoneNumber
|
||||
|
|
@ -66,7 +68,7 @@ Kirigami.ScrollablePage
|
|||
sortOrder: Qt.AscendingOrder
|
||||
sortRole: ConversationModel.DateRole
|
||||
sourceModel: ConversationModel {
|
||||
deviceId: device.id()
|
||||
deviceId: page.deviceId
|
||||
threadId: page.conversationId
|
||||
}
|
||||
}
|
||||
|
|
@ -120,7 +122,7 @@ Kirigami.ScrollablePage
|
|||
}
|
||||
|
||||
footer: RowLayout {
|
||||
enabled: page.device
|
||||
enabled: page.deviceConnected
|
||||
ScrollView {
|
||||
Layout.maximumHeight: page.height / 3
|
||||
Layout.fillWidth: true
|
||||
|
|
|
|||
|
|
@ -30,6 +30,7 @@ import org.kde.kdeconnect.sms 1.0
|
|||
|
||||
Kirigami.ScrollablePage
|
||||
{
|
||||
id: page
|
||||
footer: ComboBox {
|
||||
id: devicesCombo
|
||||
enabled: count > 0
|
||||
|
|
@ -50,11 +51,16 @@ Kirigami.ScrollablePage
|
|||
visible: !devicesCombo.enabled
|
||||
}
|
||||
|
||||
readonly property bool deviceConnected: devicesCombo.enabled
|
||||
readonly property QtObject device: devicesCombo.currentIndex >= 0 ? devicesModel.data(devicesModel.index(devicesCombo.currentIndex, 0), DevicesModel.DeviceRole) : null
|
||||
readonly property alias lastDeviceId: conversationListModel.deviceId
|
||||
|
||||
Component {
|
||||
id: chatView
|
||||
ConversationDisplay {}
|
||||
ConversationDisplay {
|
||||
deviceId: page.lastDeviceId
|
||||
deviceConnected: page.deviceConnected
|
||||
}
|
||||
}
|
||||
|
||||
ListView {
|
||||
|
|
@ -66,6 +72,7 @@ Kirigami.ScrollablePage
|
|||
sortRole: ConversationListModel.DateRole
|
||||
filterCaseSensitivity: Qt.CaseInsensitive
|
||||
sourceModel: ConversationListModel {
|
||||
id: conversationListModel
|
||||
deviceId: device ? device.id() : ""
|
||||
}
|
||||
}
|
||||
|
|
@ -113,7 +120,7 @@ Kirigami.ScrollablePage
|
|||
personUri: model.personUri,
|
||||
phoneNumber: address,
|
||||
conversationId: model.conversationId,
|
||||
device: device})
|
||||
})
|
||||
}
|
||||
onClicked: { startChat(); }
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue