Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScrollView jumping improvements #2315

Merged
merged 9 commits into from
Oct 15, 2024
Merged

ScrollView jumping improvements #2315

merged 9 commits into from
Oct 15, 2024

Conversation

Amzd
Copy link
Collaborator

@Amzd Amzd commented Oct 12, 2024

In this PR I flipped the scrollView so now new messages automatically cause the rest of the messages to move up so we no longer need to programmatically scroll down when messages are sent and/or received.

Fixes:

Intended changes:

  • Less twitching when receiving/sending messages
  • Smooth scrolling when searching
  • No longer show inputAccessoryView when presenting other views (without having to resign firstResponder)

Side effects:

Messages are not refreshed until user stops editing
This is because we rely on selectedIndexPaths to keep which messages are selected, which can get out of sync with the actual message list when new messages are received or when they are automatically deleted. This was already the case before with deleted messages (eg through auto deletion) but is now also the case for new messages because messages are now pushed at the beginning of the indexPaths instead of at the end (because tableView is flipped).

This can be mitigated by tracking selected messages instead of relying on selectedIndexPaths.

Messages don't float to the top when you have only few messages
Since tableView now starts at the bottom, when you only have like 2 messages they are at the bottom instead of at the top.

This can totally be mitigated with a bit more effort.

Empty state view is a bit higher up
I think this is due to safeArea change to tableView but it is not a bad change because now it is still visible when keyboard is up (on iPhone SE anyway)

Notes:

  • I feel like it would be better if we did not use inputAccessoryViews that switch between keyboard and view controller attachment but instead just a normal view which we animate to always stay above the keyboard. This would eliminate the need for much of the first responder juggling we do right now and also eliminate the edge case where the input view is hidden when you don't expect it.

  • Would be really nice to have tests that can confirm if the keyboard comes back after actions when expected.

@Amzd
Copy link
Collaborator Author

Amzd commented Oct 12, 2024

This PR already fixed #2312 by disallowing first responder when a the chatvc is presenting another vc, which is the conflict.

@r10s
Copy link
Member

r10s commented Oct 12, 2024

thanks a lot for diving into all that!

i already run the draft in its current state, and i have to say, that it improves things significantly already. wow!

please let us know, when it is ready-to-review - or if you have any questions

Messages are not refreshed until user stops editing

i am not sure what you mean here - i just tested receiving messages while using the input field - and that works as expected

Messages don't float to the top when you have only few messages

this is totally fine, and also how it was intended to work. this is also how it works on android/desktop: if the chat is not "full", messages are at the bottom, close to the input field

@Amzd
Copy link
Collaborator Author

Amzd commented Oct 12, 2024

i am not sure what you mean here - i just tested receiving messages while using the input field - and that works as expected

I mean while selecting messages (long press message > more options)

this is totally fine, and also how it was intended to work.

OK that's easy then. My reference (iMessage) keeps it at the top so I thought that was intended.

please let us know, when it is ready-to-review - or if you have any questions

It is basically all I wanted to do for this PR, I opened it in draft because I wasn't sure if it was OK to create PRs of this size.

@Amzd Amzd marked this pull request as ready for review October 12, 2024 16:53
@Amzd
Copy link
Collaborator Author

Amzd commented Oct 12, 2024

This PR

RPReplay_Final1728752673.mov

Release version

RPReplay_Final1728752673.mov

@Amzd
Copy link
Collaborator Author

Amzd commented Oct 13, 2024

The image from the read me showing the chat screen is no longer accurate since this PR (since messages float down instead of up here)

@Amzd
Copy link
Collaborator Author

Amzd commented Oct 13, 2024

af65d6f broke first message scrolling while trying to prevent the empty state view from jumping up and down when keyboard appears.

Fixed twitching by adding it to the background view instead of to the actual tableView content
@Amzd
Copy link
Collaborator Author

Amzd commented Oct 13, 2024

Resolved

@Amzd
Copy link
Collaborator Author

Amzd commented Oct 13, 2024

RPReplay_Final1728837824.mp4

@Amzd
Copy link
Collaborator Author

Amzd commented Oct 13, 2024

@r10s I don't have permissions to add reviewers

@link2xt
Copy link
Contributor

link2xt commented Oct 13, 2024

This is similar to how KDeltaChat does it, having scrollview laying out its elements from bottom to top is easier than trying to scroll to bottom on new messages.

@Amzd Amzd requested a review from zeitschlag October 14, 2024 09:15
@adbenitez
Copy link
Member

I am amazed thanks a lot!!! 🤩

@Amzd Amzd merged commit dbe85fc into deltachat:main Oct 15, 2024
@Amzd
Copy link
Collaborator Author

Amzd commented Oct 15, 2024

I am amazed thanks a lot!!! 🤩

Wait then who am I?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants