Details

Reviewers

Group Reviewers

Commits

R224:a7db3ab5e105: [Desktop] Update conversation list when a new message arrives

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 3048
Build 3066: 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

Harbormaster completed remote builds in B3016: Diff 41949.Sep 19 2018, 10:11 PM

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.
87	Should probably offset it by start?
smsapp/conversationlistmodel.cpp
107	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
66–69	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.
107	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

Resolve possible null dereference error when replying with list of conversations
Reply with proper messages if caller requests start != 0

Harbormaster completed remote builds in B3037: Diff 42002.Sep 20 2018, 6:39 PM

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!
87	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

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

Harbormaster completed remote builds in B3048: Diff 42016.Sep 20 2018, 10:59 PM

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

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

Update conversation list when we see a new message
Correct braces' placement

Harbormaster completed remote builds in B3535: Diff 42955.Oct 5 2018, 11:44 PM

Undo committing D15978

Harbormaster completed remote builds in B3536: Diff 42956.Oct 5 2018, 11:46 PM

Third time is the charm?

Harbormaster completed remote builds in B3537: Diff 42957.Oct 5 2018, 11:47 PM

sredman added a dependency: D15978: Correct braces and add contributors.Oct 5 2018, 11:51 PM

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.

sredman added a dependent revision: D15979: [SMS App] Allow scrolling up to load and display older messages.Oct 6 2018, 3:38 AM

sredman removed a dependency: D15978: Correct braces and add contributors.Oct 7 2018, 4:56 AM

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

Rebase onto master

Harbormaster completed remote builds in B3624: Diff 43098.Oct 8 2018, 3:44 AM

Closed by commit R224:a7db3ab5e105: [Desktop] Update conversation list when a new message arrives (authored by sredman). · Explain WhyOct 8 2018, 3:46 AM

This revision was automatically updated to reflect the committed changes.

sredman removed a dependent revision: D15979: [SMS App] Allow scrolling up to load and display older messages.Oct 10 2018, 3:33 AM

		Path
M		plugins/sms/conversationsdbusinterface.h (9 lines)
M		plugins/sms/conversationsdbusinterface.cpp (28 lines)
M		smsapp/conversationlistmodel.h (5 lines)
M		smsapp/conversationlistmodel.cpp (59 lines)
M		smsapp/conversationmodel.cpp (6 lines)
D	M	smsapp/qml/ContactList.qml (97 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	41949	b2a53c6		Sep 19 2018, 10:11 PM	★	★
Diff 2	42002	b2a53c6	- Resolve possible null dereference error when replying with list of…	Sep 20 2018, 6:39 PM	★	★
Diff 3	42016	b2a53c6	- Make proper use of m_conversations when replying with conversation starters	Sep 20 2018, 10:59 PM	★	★
Diff 4	42955	d314758	- Update conversation list when we see a new message	Oct 5 2018, 11:44 PM	★	★
Diff 5	42956	2efb38e	Undo committing D15978	Oct 5 2018, 11:45 PM	★	★
Diff 6	42957	8673d5a	Third time is the charm?	Oct 5 2018, 11:47 PM	★	★
Diff 7	43098	ab33cce	Rebase onto master	Oct 8 2018, 3:44 AM	★	★
Diff 8	43099	ab33cce	R224:a7db3ab5e105b2f6cd2c6d437bf7a3bc4954f9ce	Oct 8 2018, 3:46 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
b3d82bc8ddce	429c88b476a2	f85c9ee0d598	Simon Redman	Disconnect from conversationInterface when creating a new one (Show More…)	Sep 20 2018, 10:57 PM
f85c9ee0d598	753b5b157456	56f7f34e87ab	Simon Redman	Check for valid device when receiving ID (Show More…)	Sep 20 2018, 10:55 PM
56f7f34e87ab	827fcb3b1911	a7f7cc51c578	Simon Redman	Delete apparently unused ContactsList (Show More…)	Sep 20 2018, 10:52 PM
a7f7cc51c578	98709155e085	967e108371c2	Simon Redman	Make proper use of m_conversations when replying with conversation starters	Sep 20 2018, 10:52 PM
967e108371c2	9a3f6f90a57d	2ca632e5a47d	Simon Redman	Reply with proper messages if caller requests start != 0	Sep 20 2018, 6:38 PM
2ca632e5a47d	741b3f868b4c	2fd9338f1991	Simon Redman	Resolve possible null dereference error when replying with list of conversations	Sep 20 2018, 6:38 PM
2fd9338f1991	8186f0275341	b2a53c6d2e58	Simon Redman	[Desktop] Prevent SMS app from requesting entire contents of every conversation (Show More…)	Sep 19 2018, 8:57 PM

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

Details

Diff Detail

Revision Contents
Changeset List

Diff 42016

plugins/sms/conversationsdbusinterface.h

plugins/sms/conversationsdbusinterface.cpp

smsapp/conversationlistmodel.h

smsapp/conversationlistmodel.cpp

smsapp/conversationmodel.cpp

smsapp/qml/ContactList.qml

[Desktop] Update conversation list when a new message arrivesClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 42016

plugins/sms/conversationsdbusinterface.h

plugins/sms/conversationsdbusinterface.cpp

smsapp/conversationlistmodel.h

smsapp/conversationlistmodel.cpp

smsapp/conversationmodel.cpp

smsapp/qml/ContactList.qml

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

Revision Contents
Changeset List