Skip to content

Conversation

@ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Mar 14, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fixes a crash due to a view with id R.id.additionalTopSpace not being found in the base timeline view. This is a view that's only being used in bubbles

Motivation and context

Closes #5535 and https://github.com/matrix-org/element-android-rageshakes/issues/35624

The additionalTopSpace view is only being used in bubbles so I reduced its scope to only the bubbles view rather than the base timeline view.

I also included some slight refactoring of the bubbles view class for readability

Screenshots / GIFs

No UI changes but simple confirmation that top space above bubbles works as expected

image

Tests

  • Open timeline. Observe no crash
  • Observe top margin behaviour added in this PR is still present

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 10

Checklist

@github-actions
Copy link

github-actions bot commented Mar 14, 2022

Unit Test Results

  98 files  ±  0    98 suites  ±0   1m 6s ⏱️ -8s
178 tests +  4  178 ✔️ +  4  0 💤 ±0  0 ±0 
584 runs  +16  584 ✔️ +16  0 💤 ±0  0 ±0 

Results for commit 184b35a. ± Comparison against base commit acfeb7f.

This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both.
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ when handling display name update then updates upstream user display name
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is not supported when updating display name then updates upstream user display name and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is supported when updating display name then updates upstream user display name and moves to choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given homeserver does not support personalisation when registering account then updates state and emits account created event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given only supports changing profile picture when handling PersonalizeProfile then emits contents choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given supports changing display name when handling PersonalizeProfile then emits contents choose display name

♻️ This comment has been updated with latest results.

@ericdecanini ericdecanini marked this pull request as ready for review March 14, 2022 13:37
@ericdecanini ericdecanini requested review from a team and mnaturel and removed request for a team March 14, 2022 13:37
Copy link
Contributor

@mnaturel mnaturel left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Okay for me, just 1 question about changelog entry.

@@ -0,0 +1 @@
Fixes crash upon opening timeline due to additionalTopSpace
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if this fix needs an entry. Is the crash on the current release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After several discussions, updated the PR and removed the changelog 👍️

@ericdecanini ericdecanini requested a review from mnaturel March 14, 2022 16:36
@ericdecanini ericdecanini merged commit de14e10 into develop Mar 15, 2022
@ericdecanini ericdecanini deleted the bugfix/eric/top-space-crash branch March 15, 2022 08:59
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.

Timeline crashing due to additionalTopSpace

3 participants