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

Fix: Scroll state in retained in the partial translation mode #2643

Conversation

ikerfah
Copy link
Contributor

@ikerfah ikerfah commented Mar 25, 2024

Fixes #2598 Retain scroll state in partial translation mode

The issue was with the call to scrollTo(0, 0) within the setAyahs(), so basically when the Ayahs are set the scroll is rested to 0,0.

Removing scrollTo(0, 0) persists the scroll position since InlineTranslationView extends ScrollView and it saves the scroll state out of the box. but this introduce a new problem which is when you select a different ayah then the scroll will not be rested. to fix this I added a flag called ayahHasBeenChanged to reset the scroll to 0,0 when ayah is changed

Copy link

OLD: app-madani-debug.apk (signature: V1, V2)
NEW: app-madani-debug.apk (signature: V1, V2)

          │           compressed           │          uncompressed          
          ├───────────┬───────────┬────────┼───────────┬───────────┬────────
 APK      │ old       │ new       │ diff   │ old       │ new       │ diff   
──────────┼───────────┼───────────┼────────┼───────────┼───────────┼────────
      dex │  20.3 MiB │  20.3 MiB │ +404 B │  65.5 MiB │  65.5 MiB │ +684 B 
     arsc │   2.2 MiB │   2.2 MiB │    0 B │   2.2 MiB │   2.2 MiB │    0 B 
 manifest │   5.6 KiB │   5.6 KiB │    0 B │  27.1 KiB │  27.1 KiB │    0 B 
      res │   1.6 MiB │   1.6 MiB │    0 B │   1.8 MiB │   1.8 MiB │    0 B 
    asset │ 404.2 KiB │ 404.2 KiB │    0 B │ 678.6 KiB │ 678.6 KiB │    0 B 
    other │ 188.6 KiB │ 188.6 KiB │   -3 B │ 385.6 KiB │ 385.6 KiB │    0 B 
──────────┼───────────┼───────────┼────────┼───────────┼───────────┼────────
    total │  24.7 MiB │  24.7 MiB │ +401 B │  70.6 MiB │  70.6 MiB │ +684 B 


Copy link
Contributor

@ahmedre ahmedre left a comment

Choose a reason for hiding this comment

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

jazakumAllah khairan, really appreciate the great work on this and looking forward to merging it! just one small comment in the PR that I think will simplify the PR a bit.

@@ -38,11 +38,11 @@ class InlineTranslationPresenter @Inject constructor(
.launchIn(scope)
}

suspend fun refresh(verseRange: VerseRange) {
suspend fun refresh(verseRange: VerseRange, ayahHasBeenChanged: Boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we keep track of the last verse range within InlineTranslationPresenter, we can avoid having to update all the callers to pass in whether or not the ayah has changed - can we do that instead? i.e.:

private var lastVerseRange: VerseRange? = null

suspend fun refresh(verseRange: verseRange) {
  val ayahHasBeenChanged = verseRange == lastVerseRange
  // existing logic here
  lastVerseRange = verseRange
}

Copy link
Contributor Author

@ikerfah ikerfah Mar 30, 2024

Choose a reason for hiding this comment

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

Looks good! I changed the PR (forced pushed to omit the old commit)

@ikerfah ikerfah force-pushed the fix/2598-scroll-state-not-retained-in-partial-translation-mode branch from 2208af5 to 089106c Compare March 30, 2024 12:48
@ikerfah ikerfah requested a review from ahmedre March 30, 2024 12:59
Copy link
Contributor

@ahmedre ahmedre left a comment

Choose a reason for hiding this comment

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

jazakumAllah khairan!

@ahmedre ahmedre enabled auto-merge March 30, 2024 13:04
@ahmedre ahmedre merged commit 64f83ea into quran:main Mar 30, 2024
3 checks passed
Copy link

OLD: app-madani-debug.apk (signature: V1, V2)
NEW: app-madani-debug.apk (signature: V1, V2)

          │           compressed           │          uncompressed          
          ├───────────┬───────────┬────────┼───────────┬───────────┬────────
 APK      │ old       │ new       │ diff   │ old       │ new       │ diff   
──────────┼───────────┼───────────┼────────┼───────────┼───────────┼────────
      dex │  20.3 MiB │  20.3 MiB │ +230 B │  65.5 MiB │  65.5 MiB │ +584 B 
     arsc │   2.2 MiB │   2.2 MiB │    0 B │   2.2 MiB │   2.2 MiB │    0 B 
 manifest │   5.6 KiB │   5.6 KiB │    0 B │  27.1 KiB │  27.1 KiB │    0 B 
      res │   1.6 MiB │   1.6 MiB │    0 B │   1.8 MiB │   1.8 MiB │    0 B 
    asset │ 404.2 KiB │ 404.2 KiB │    0 B │ 678.6 KiB │ 678.6 KiB │    0 B 
    other │ 188.6 KiB │ 188.6 KiB │   -3 B │ 385.6 KiB │ 385.6 KiB │    0 B 
──────────┼───────────┼───────────┼────────┼───────────┼───────────┼────────
    total │  24.7 MiB │  24.7 MiB │ +227 B │  70.6 MiB │  70.6 MiB │ +584 B 


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.

Retain scroll state in partial translation mode
2 participants