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

Crash on opening a room on Android 5.0 and 5.1 - Regression with Voice message #3903

Merged
merged 2 commits into from
Aug 27, 2021

Conversation

onurays
Copy link
Contributor

@onurays onurays commented Aug 26, 2021

Fixes #3897

@onurays onurays requested a review from bmarty August 26, 2021 10:51
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.

Thanks for the PR.

A few remarks after our internal discussion

Comment on lines +79 to +89
// Don't convert to primary constructor.
// We need to define views as lateinit var to be able to check if initialized for the bug fix on api 21 and 22.
@JvmOverloads constructor(
context: Context,
attrs: AttributeSet? = null,
defStyleAttr: Int = 0
) : super(context, attrs, defStyleAttr) {
initialize()
}

fun initialize() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, views has to be lateinit var to be able to use isInitialized. When we initialize views in init, lint says it is redundant to mark as lateinit...

attrs: AttributeSet? = null,
defStyleAttr: Int = 0
) : ConstraintLayout(context, attrs, defStyleAttr), VoiceMessagePlaybackTracker.Listener {
class VoiceMessageRecorderView: ConstraintLayout, VoiceMessagePlaybackTracker.Listener {
Copy link
Member

Choose a reason for hiding this comment

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

Is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same reason with the first comment.

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.

Thanks for your comments

@bmarty bmarty merged commit c6c66cc into develop Aug 27, 2021
@bmarty bmarty deleted the feature/ons/fix_voice_message_lollipop_crash branch August 27, 2021 14:31
@unclechu
Copy link

Do you have any estimation when this fix is going to be released on F-Droid?

@@ -90,6 +96,9 @@ class VoiceMessageRecorderView @JvmOverloads constructor(

override fun onVisibilityChanged(changedView: View, visibility: Int) {
super.onVisibilityChanged(changedView, visibility)
// onVisibilityChanged is called by constructor on api 21 and 22.
if (!this::views.isInitialized) return
Copy link
Contributor

Choose a reason for hiding this comment

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

@bmarty Were you able to reproduce the error on Android 5 and verify if this patch works?

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 on opening a room on Android 5.0 and 5.1 - Regression with Voice message
4 participants