[Desktop] Update conversation list when a new message arrives
ClosedPublic

Authored by sredman on Sep 19 2018, 10:11 PM.

Details

Summary

When a new message is delivered, the conversation list should update by changing the preview text and re-sorting the conversations

Bonus bug discovered and fixed: previously, when the conversations list was being populated, it made a request for the first message in every conversation. This would be fine if the conversationdbusinterface pulled from local cache. However, this actually triggers a request to the phone for *every* conversation.

This should be handled differently in conversationdbusinterface's requestConversation as well, but that's a project for a later day (TODO comments added)

Test Plan
  • Launch SMS app
  • Verify conversations list appears
  • Verify lack of massive stream of debug output indicating lots of messages for the wrong conversation are being received
  • Verify that opening a particular conversation shows the messages after a short delay while the backend fetches the content from the phone
  • Verify that receiving a new message into an existing conversation updates the conversation list

Diff Detail

Repository
R224 KDE Connect
Branch
convo-preview-update
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3016
Build 3034: arc lint + arc unit
sredman created this revision.Sep 19 2018, 10:11 PM
Restricted Application added a project: KDE Connect. Β· View Herald TranscriptSep 19 2018, 10:11 PM
Restricted Application added a subscriber: kdeconnect. Β· View Herald Transcript
sredman requested review of this revision.Sep 19 2018, 10:11 PM
apol added a subscriber: apol.Sep 20 2018, 9:02 AM

LGTM overall, I'm a bit concerned this is getting a bit too complex.

plugins/sms/conversationsdbusinterface.cpp
53

Looks like it's casting twice? How about toReturn += conversation.cbegin()->toVariant()?
Also we probably should make sure it's not empty.

79

Should probably offset it by start?

smsapp/conversationlistmodel.cpp
98

leave empty?

In D15608#328757, @apol wrote:

LGTM overall, I'm a bit concerned this is getting a bit too complex.

I agree. Can you see anywhere where we can make it better, though? This patch actually removes one call/response on the dbus interface by directly providing the message contents instead of requesting them with the threadID, so nice from that point of view.

I will look into your other comments as soon as possible

smsapp/conversationlistmodel.cpp
63–66

What do you think about this being above the checks to delete m_conversationsInterface? That way, when the device is disconnected, you can still browse messages locally cached in the conversationDbusInterface.

98

I left this here so I could put a breakpoint on it and decide what to do with the incoming data 😬

Working on filling out this method is my next to-do to solve bug 398818

sredman updated this revision to Diff 42002.Sep 20 2018, 6:39 PM
  • Resolve possible null dereference error when replying with list of conversations
  • Reply with proper messages if caller requests start != 0
sredman marked 2 inline comments as done.Sep 20 2018, 6:40 PM
sredman added inline comments.
plugins/sms/conversationsdbusinterface.cpp
53

The double cast was because I was trying to resolve a (different) issue. Good catch on the "null" dereference, thank you!

79

I forgot I could use a different dbus viewer to make the call: Now I have been able to test this and it appears to work!

sredman marked 2 inline comments as done.Sep 20 2018, 6:41 PM
sredman updated this revision to Diff 42016.Sep 20 2018, 10:59 PM
  • Make proper use of m_conversations when replying with conversation starters
  • Delete apparently unused ContactsList
  • Check for valid device when receiving ID
  • Disconnect from conversationInterface when creating a new one
sredman marked an inline comment as done.Sep 20 2018, 11:29 PM

The problem I was thinking of solving by shuffling this method is actually not a problem in this component, so ignore this!

nicolasfella requested changes to this revision.Sep 25 2018, 1:10 AM
nicolasfella added a subscriber: nicolasfella.

A word on coding style:
In C++ KDE Connect we put the { on the next line for methods but on the same line for block within a method. Please adjust the patch and also your past work (in a separate commit)

This revision now requires changes to proceed.Sep 25 2018, 1:10 AM
sredman updated this revision to Diff 42955.Oct 5 2018, 11:44 PM
  • Update conversation list when we see a new message
  • Correct braces' placement
sredman updated this revision to Diff 42956.Oct 5 2018, 11:45 PM

Undo committing D15978

sredman updated this revision to Diff 42957.Oct 5 2018, 11:47 PM

Third time is the charm?

sredman retitled this revision from [Desktop] Prevent SMS app from requesting entire contents of every conversation to [Desktop] Update conversation list when a new message arrives.Oct 5 2018, 11:54 PM
sredman edited the summary of this revision. (Show Details)
sredman edited the test plan for this revision. (Show Details)
sredman marked 4 inline comments as done.
nicolasfella accepted this revision.Oct 7 2018, 6:54 PM
This revision is now accepted and ready to land.Oct 7 2018, 6:54 PM
sredman updated this revision to Diff 43098.Oct 8 2018, 3:44 AM

Rebase onto master

This revision was automatically updated to reflect the committed changes.