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

Save scroll position in timeline #2459

Draft
wants to merge 1 commit into
base: release_1.10
Choose a base branch
from

Conversation

danieldaquino
Copy link
Contributor

This commit implements a way to save the scroll position within a timeline. It does that by assigning each note in the timeline an ID based on the NoteID, and adding a scrollPosition modifier along with SceneStorage to keep track of and persist the scroll position along the timeline view throughout a single app session.

Notes:

  • Scroll position is not persisted across app restarts. When the user completely quits the app, scroll position is lost.
  • This works on home feed and universe view. However, due to how Universe view is dynamically loaded, performance may not be as good as on the home feed
  • This only works on iOS 17 and higher, since the necessary scroll position reading mechanism is only available in those versions. On older versions things should work as before this change.

Testing

PASS

Damus: This commit
iOS: 17.6.1
Device: iPhone 13 mini
Steps:

  1. Scroll down home feed to a note with a memorable image
  2. Switch to the notifications tab
  3. Switch back to the home tab. Ensure scroll position is at the memorable image (or close). PASS
  4. Navigate into another profile from the home feed
  5. Go back to the home feed by clicking the "back" button on the top left. Ensure scroll position is preserved. PASS
  6. Navigate into another profile from the home feed again.
  7. Go back to the home feed by clicking the home button at the bottom tab bar. Ensure scroll position is preserved. PASS
  8. Click on the home button at the bottom tab bar while at the home feed. You should be taken to the top. PASS

iOS 16 regression testing

PASS

Damus: This commit
iOS: 16.4
Device: iPhone SE simulator
Steps:

  1. Navigate through the home feed, navigate between tabs
  2. Ensure there are no visible regressions on navigation. PASS

Changelog-Fixed: Fixed situations where scroll position would be lost (iOS 17 only)
Closes: #751

This commit implements a way to save the scroll position within a
timeline. It does that by assigning each note in the timeline an ID
based on the NoteID, and adding a `scrollPosition` modifier along with
SceneStorage to keep track of and persist the scroll position along the
timeline view throughout a single app session.

Notes:
- Scroll position is not persisted across app restarts. When the user
  completely quits the app, scroll position is lost.
- This works on home feed and universe view. However, due to how
  Universe view is dynamically loaded, performance may not be as good as
  on the home feed
- This only works on iOS 17 and higher, since the necessary scroll
  position reading mechanism is only available in those versions. On
  older versions things should work as before this change.

Testing
-------

PASS

Damus: This commit
iOS: 17.6.1
Device: iPhone 13 mini
Steps:
1. Scroll down home feed to a note with a memorable image
2. Switch to the notifications tab
3. Switch back to the home tab. Ensure scroll position is at the memorable image (or close). PASS
4. Navigate into another profile from the home feed
5. Go back to the home feed by clicking the "back" button on the top
   left. Ensure scroll position is preserved. PASS
6. Navigate into another profile from the home feed again.
7. Go back to the home feed by clicking the home button at the bottom
   tab bar. Ensure scroll position is preserved. PASS
8. Click on the home button at the bottom tab bar while at the home
   feed. You should be taken to the top. PASS

iOS 16 regression testing
-------------------------

PASS

Damus: This commit
iOS: 16.4
Device: iPhone SE simulator
Steps:
1. Navigate through the home feed, navigate between tabs
2. Ensure there are no visible regressions on navigation. PASS

Changelog-Fixed: Fixed situations where scroll position would be lost (iOS 17 only)
Closes: damus-io#751
Signed-off-by: Daniel D’Aquino <[email protected]>
Copy link
Collaborator

@jb55 jb55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

// therefore ensuring we will *own* this piece of memory, reducing the risk of being rugged by Ndb today and in future as the codebase evolves.
// This is a 32-byte copy operation without any parsing, so it should in theory not regress performance significantly.
// Thanks for coming to my TED talk about this one line of code.
let ev_id = ev.id
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

events held in timelines are owned data! they are not pointers to nostrdb. that would be bad, since the data would be outside of a transaction.

when we switch to the local relay model, timeline events will be lists of u64 primary keys. This is how notedeck works.

Copying an ID will probably be what we have to do when we switch to the local relay model, so this LGTM!

@danieldaquino
Copy link
Contributor Author

@jb55 as promised I did some performance testing, since I noticed some hitches while casually using a dev build, and wanted to check if it was just a fluke or if a real performance regression occurred. Unfortunately there was a performance regression.

Performance testing details

Baseline Damus version: 3902fe7b30f38ec104c13087948799e38e26fa91 (current tip of release_1.10)
Damus version under test: 7f0f5771135f60fcf5199c1c45b8a6ae6d47b83c (current tip of this PR)
Test parameters:

  • Device: iPhone 13 mini (real device)
  • Build scheme: release, built for profiling with Xcode instruments
  • iOS: 17.6.1
  • Xcode instruments: 15.3 (15F31d)
  • Computer/iPhone connection: USB 3.0

Procedure:

  1. Define a very specific navigation circuit for all samples (Scroll from home feed down to a specific note, click on profile of that author, scroll down profile view until another specific note)
  2. Setup Xcode Instruments instrumentation to measure animation hitches (see definition of "hitches" here)
  3. Take 3 performance samples of the same navigation circuit for both the baseline version and the version under test, observing the following constraints:
    • Perform scrolling with as much consistency as possible
    • Alternate between baseline and version under test when taking samples to avoid variables like app caching or hand fatigue from affecting samples disproportionately between versions
  4. Once the data is gathered, count the low, moderate, and high severity hitches for all samples, then take the mean, median, and standard deviation for each version
  5. Analyze results

Data

#751 mean #751 median #751 std deviation 1.10 base mean 1.10 base median 1.10 base std deviation
Low severity hitches 63.0 62 1.73 42.3 38 7.51
Moderate severity hitches 5.7 6 1.53 7.3 7 1.53
High severity hitches 4.3 5 1.15 5.3 5 0.58
Total 73.0 73 4.00 55.0 50 8.66

Analysis and results

It was hard to tell with certainty that there was a performance regression with the naked eye because there are so many factors that go into the animation performance, but the results of this rigorous test are pretty conclusive. Unfortunately there was a performance regression that caused an increase in low-severity hitches. @jb55, I am thinking of the following actions:

  1. Work to get these resolved within the scope of the 1.10 release
  2. Descope Return to feed after opening notes may return user to incorrect place in feed #751 from 1.10, merge directly to master (after the seamless navigation), and file a ticket to resolve these as part of the 1.11 scope
  3. Descope Return to feed after opening notes may return user to incorrect place in feed #751 from 1.10 (move it to 1.11), don't merge this, and keep it all open for when we work on 1.11

How would you like to proceed?

@danieldaquino danieldaquino marked this pull request as draft September 13, 2024 21:43
@danieldaquino
Copy link
Contributor Author

Converted to draft while we don't solve the performance issue

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.

2 participants