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

Timeline : fix 4959 #4966

Closed
wants to merge 1 commit into from
Closed

Conversation

ganfra
Copy link
Member

@ganfra ganfra commented Jan 17, 2022

Fix #4959 and add more logs.

@ganfra ganfra requested a review from bmarty January 17, 2022 18:17
@github-actions
Copy link

Unit Test Results

  66 files  ±0    66 suites  ±0   57s ⏱️ ±0s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit b91adf9. ± Comparison against base commit 256929b.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, just a small remark on a comment.
Thanks for the fast investigation!

(position until position + count).forEach {
// Invalidate cache
modelCache[it] = null
}
// Also invalidate the first previous displayable event if
// it's sent by the same user so we are sure we have up to date information.
val invalidatedSenderId: String? = currentSnapshot.getOrNull(position)?.senderInfo?.userId
val prevDisplayableEventIndex = currentSnapshot.subList(0, position).indexOfLast {
// In some cases onChanged will be called before onRemoved and onInserted so position will be smaller than currentSnapshot.size.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// In some cases onChanged will be called before onRemoved and onInserted so position will be smaller than currentSnapshot.size.
// In some cases onChanged will be called before onRemoved and onInserted so position will be bigger than currentSnapshot.size.

I think, no?

@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed="21" failures="0" errors="0" skipped="2"
  • [org.matrix.android.sdk.account]
    passed="5" failures="0" errors="0" skipped="2"
  • [org.matrix.android.sdk.internal]
    passed="158" failures="1" errors="0" skipped="38"
  • [org.matrix.android.sdk.ordering]
    passed="16" failures="0" errors="0" skipped="0"
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed="2" failures="0" errors="0" skipped="0"

timelineEventVisibilityHelper.shouldShowEvent(it, partialState.highlightedEventId)
}
if (prevDisplayableEventIndex != -1 && currentSnapshot[prevDisplayableEventIndex].senderInfo.userId == invalidatedSenderId) {
if (prevDisplayableEventIndex != -1 && currentSnapshot.getOrNull(prevDisplayableEventIndex)?.senderInfo?.userId == invalidatedSenderId) {
Copy link
Member

Choose a reason for hiding this comment

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

I do not think this change is necessary, since prevDisplayableEventIndex will always be in the range [0, currentSnapshot.size[.
Anyway, better safe than sorry I guess.

@bmarty bmarty changed the base branch from develop to hotfix/1.3.15 January 18, 2022 08:04
@bmarty bmarty changed the base branch from hotfix/1.3.15 to develop January 18, 2022 08:05
@bmarty
Copy link
Member

bmarty commented Jan 18, 2022

Included in hotfix #4971

@bmarty bmarty closed this Jan 18, 2022
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.

Crash in timeline: sublist
2 participants