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 crash deleting video refreshing #5116

Merged
merged 3 commits into from
Dec 15, 2020

Conversation

hlloreda
Copy link
Contributor

@hlloreda hlloreda commented Dec 6, 2020

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Method equals has been overridden in the PlaylistStreamEntry class so that the index (in the method 'removeItem' in the Class LocalItemListAdapter) does not return '-1' if the memory address is not the same. In this method, I compare all the different attributes to make sure it's the same object. The hashCode method was generated by Android Studio.

Fixes the following issue(s)

APK testing

app-deleting-video-refreshing-crashfix.zip

Due diligence

Comment on lines 40 to 46
override fun equals(other: Any?): Boolean {
if (other == null || other !is PlaylistStreamEntry || streamEntity != other.streamEntity ||
progressTime != other.progressTime || streamId != other.streamId || joinIndex != other.joinIndex
) return false

return true
}
Copy link
Contributor

@triallax triallax Dec 7, 2020

Choose a reason for hiding this comment

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

Suggested change
override fun equals(other: Any?): Boolean {
if (other == null || other !is PlaylistStreamEntry || streamEntity != other.streamEntity ||
progressTime != other.progressTime || streamId != other.streamId || joinIndex != other.joinIndex
) return false
return true
}
override fun equals(other: Any?): Boolean =
!(other == null || other !is PlaylistStreamEntry || streamEntity != other.streamEntity ||
progressTime != other.progressTime || streamId != other.streamId || joinIndex != other.joinIndex)

Edit: ignore this suggestion.

Copy link
Contributor

@triallax triallax left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one minor nit.

Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

Can't a data class be used? You mention that you just compare all fields, which the default generated method from Android Studio should also do, which in turn is implicitly generated in data classes

@triallax
Copy link
Contributor

triallax commented Dec 7, 2020

@XiangRongLin I missed that.

@hlloreda ignore my suggestion and try @XiangRongLin's one.

@hlloreda
Copy link
Contributor Author

hlloreda commented Dec 7, 2020

Can't a data class be used? You mention that you just compare all fields, which the default generated method from Android Studio should also do, which in turn is implicitly generated in data classes

Indeed, it seems better. So to be sure to follow @XiangRongLin completely, the idea would be to change the current constructor to initialise a 'data' attribute which would be a data class containing all the current attributes or you mean to simply define the 'PlaylistStreamEntry' class as a 'data class' ?

@XiangRongLin
Copy link
Collaborator

or you mean to simply define the 'PlaylistStreamEntry' class as a 'data class' ?

@hlloreda This.

Copy link
Contributor

@triallax triallax left a comment

Choose a reason for hiding this comment

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

LGTM

@Stypox Stypox merged commit e4a1fc9 into TeamNewPipe:dev Dec 15, 2020
@Stypox
Copy link
Member

Stypox commented Dec 15, 2020

Thank you for the contribution :-D

This was referenced Dec 21, 2020
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 while deleting a video from a list
4 participants