[SMS App] Allow scrolling up to load and display older messages
ClosedPublic

Authored by sredman on Oct 6 2018, 3:38 AM.

Details

Summary

Scroll up to show older messages

Newly received messages will not force the view to the bottom unless the new message is being added "very close" to the visible area

Test Plan

Message History:

  • Open conversation
  • Scroll/Drag up to load older messages

New Message:

  • Open conversation
  • Scroll to bottom
  • Verify that a newly-received or newly-sent message is added to the GUI
  • Scroll up
  • Verify that sending/receiving a message does not disturb the view
  • Scroll back to verify that the new message was indeed added to the list

Diff Detail

Repository
R224 KDE Connect
Branch
asynchonous-message-fetch-scrollback
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4294
Build 4312: arc lint + arc unit
sredman created this revision.Oct 6 2018, 3:38 AM
Restricted Application added a project: KDE Connect. · View Herald TranscriptOct 6 2018, 3:38 AM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
sredman requested review of this revision.Oct 6 2018, 3:38 AM

QML suggestions are welcome! I am not proud of everything here, but I spent a long time trying everything and what you see is the best way I found which works

When I open a long conversation I initially see one message. Scrolling has no effect at all. When dragging I see a couple of messages appearing and immediately disappearing. Still only one message stays. Scrolling still has no effect. I can show new messages by dragging the single message. If I show about half of the new messages scrolling still has no effect. When I drag long enough to trigger the second reload scrolling works as I would expect it.

When I open a long conversation I initially see one message.

Yes. The issue is the backend only has one message loaded from the phone. When the view first loads it requests the first 10 which causes the backend to request from the phone. Once they're in cache this isn't a problem (try opening the same conversation twice after a slight delay)
I agree this is kind of ugly but I don't know an easy way to fix it. One thought is to make ConversationsDbusInterface::requestConversation synchronous and block until it is able to serve the request. This would be good for several reasons... For one thing, it would enable having multiple conversation views open at the same time!

Scrolling has no effect at all.

Do you mean with the mouse wheel, or with a touchpad? It works with a mouse wheel (and with dragging), but I only just tested a touchpad and that doesn't seem to work. It works after there is more than one message showing. Maybe the solution is to fix the only-one-message problem.

When dragging I see a couple of messages appearing and immediately disappearing. Still only one message stays. Scrolling still has no effect. I can show new messages by dragging the single message. If I show about half of the new messages scrolling still has no effect. When I drag long enough to trigger the second reload scrolling works as I would expect it.

Could you try disabling the places where I mess with the highlight animation? These are lines 96 and 106 of ConversationDisplay.qml. This won't fix the scrolling, but it might fix the invisible message (of course, you'll get an ugly animation instead...)

Do you mean with the mouse wheel, or with a touchpad? It works with a mouse wheel (and with dragging), but I only just tested a touchpad and that doesn't seem to work. It works after there is more than one message showing. Maybe the solution is to fix the only-one-message problem.

Neither mouse nor touchpad work for me

Yes. The issue is the backend only has one message loaded from the phone. When the view first loads it requests the first 10 which causes the backend to request from the phone. Once they're in cache this isn't a problem (try opening the same conversation twice after a slight delay)
I agree this is kind of ugly but I don't know an easy way to fix it. One thought is to make ConversationsDbusInterface::requestConversation synchronous and block until it is able to serve the request. This would be good for several reasons... For one thing, it would enable having multiple conversation views open at the same time!

Making it blocking sound like it could cause issues when a device is not reachable. Have you actually tested this?
When opening the conversation a bunch (10) messages are requested. The problem is that once they arrive the view isn't updated, isn't it?

Yes. The issue is the backend only has one message loaded from the phone. When the view first loads it requests the first 10 which causes the backend to request from the phone. Once they're in cache this isn't a problem (try opening the same conversation twice after a slight delay)
I agree this is kind of ugly but I don't know an easy way to fix it. One thought is to make ConversationsDbusInterface::requestConversation synchronous and block until it is able to serve the request. This would be good for several reasons... For one thing, it would enable having multiple conversation views open at the same time!

Making it blocking sound like it could cause issues when a device is not reachable.

When a device is unreachable, we have a whole different pile of problems. I will work on solving those "soon"... Basically as soon as we can get this patch to work for you!

Have you actually tested this?

D16092

When opening the conversation a bunch (10) messages are requested. The problem is that once they arrive the view isn't updated, isn't it?

That is a different way of looking at the problem. Maybe I am looking at it the wrong way, but my thought is it is very hard for the view to communicate to the dbus interface:
a) Which conversations are currently being looked at (thus which conversations it wants to see updates for)
b) Which messages of those conversations it actually wants
My opinion is that the dbus interface should create the appearance of containing all messages, even if that is actually an illusion and it has to call back to Android. Thus, it should return the requested messages (if they exist) directly (synchronously) rather than just dropping the initial request while it scrambles around to find the requested messages.

It works quite well.

When opening a conversation there should be enough messages to fill the screen (if available). In my case this is about 20 single-line messages, so the current 10 are not enough.

The problem of scrolling having no effect seems to appear when there are not enough messages to fill the screen and thus when there is no scrollbar.

apol requested changes to this revision.Oct 28 2018, 11:34 PM
apol added a subscriber: apol.

Looking good!

plugins/sms/conversationsdbusinterface.cpp
71–86

Use m_conversations.value(conversationID). Otherwise, if conversationId doesn't exist, it will create the entry and leave it empty.

92–93

messagesList.length() <= end

plugins/sms/smsplugin.cpp
80

Why this change?

85

Definitely not.

plugins/sms/smsplugin.h
113

Would it be very hard to just do it now?

This revision now requires changes to proceed.Oct 28 2018, 11:34 PM
sredman marked 3 inline comments as done.Oct 29 2018, 12:03 AM

There is something really strange going on here. It looks like I accidentally uploaded an older patch 😬
This one is actually incompatible with D16475 😬 . Yet I suppose it was working for you guys...
I will upload the version I meant to upload in a second. This time I will double-check my patch.

plugins/sms/conversationsdbusinterface.cpp
71–86

This is actually fixed in D16475

plugins/sms/smsplugin.cpp
80

Because some places we had ints (specifically, this is how Android stores the value) and some places we had strings. It was a bit of a pain for me to remember which went where, so I declared everything should be int. Declaring everything should be string would be just as valid
However, that change was actually made in D16475

plugins/sms/smsplugin.h
113

I am actually working on this right now. I hope it is not too difficult

sredman updated this revision to Diff 44395.Oct 29 2018, 12:03 AM
sredman marked an inline comment as done.
  • Remove unnecessary return
sredman updated this revision to Diff 44396.Oct 29 2018, 12:04 AM

Patch against proper revision

apol accepted this revision.Oct 29 2018, 2:04 PM

LGTM

This revision is now accepted and ready to land.Oct 29 2018, 2:04 PM
sredman updated this revision to Diff 47402.Dec 12 2018, 12:57 AM

Rebase on master via D16475

sredman updated this revision to Diff 47488.Dec 13 2018, 5:45 AM
  • Re-Re-Rebase onto Master including D16475
This revision was automatically updated to reflect the committed changes.