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 trying to delete object not in list #6127

Merged
merged 2 commits into from
May 12, 2021

Conversation

sauravrao637
Copy link
Contributor

@sauravrao637 sauravrao637 commented Apr 21, 2021

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

-When the removeItem() is called it might happen somehow that object is not in list , in this case indexOf() will return -1 and in this case we should not try to delete object by it's index (i.e is -1)
-Coming to "somehow" this may happen when
1)the removeItem() is called on duplicate of item in list , in this case we need to implement a way to remove item by it's duplicate as duplicate is not in list
2)the object is already deleted and the UI is not updated (handled this in this PR)

Fixes the following issue(s)

Relies on the following changes

APK testing

On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.

Due diligence

@AudricV AudricV added the bug Issue is related to a bug label Apr 21, 2021
Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Thank you.

@sauravrao637
Copy link
Contributor Author

sauravrao637 commented Apr 22, 2021

@Redirion See it shows "Failing after 49s - build-and-test-jvm"
image

@Redirion
Copy link
Member

Redirion commented Apr 22, 2021

ah found the reason. Please modify the checkstyle-suppressions.xml

it contains: <suppress checks="FinalParameters" files="LocalItemListAdapter.java" lines="221,293"/>

the line numbers need to be adjusted accordingly

@sauravrao637
Copy link
Contributor Author

sauravrao637 commented Apr 22, 2021

ah found the reason. Please modify the checkstyle-suppressions.xml

it contains: <suppress checks="FinalParameters" files="LocalItemListAdapter.java" lines="221,293"/>

the line numbers need to be adjusted accordingly

What if in future line number changes again ? It will be back again :(
There must be a better way

@Redirion
Copy link
Member

Redirion commented Apr 22, 2021

Checkstyle is stupid in this regard. In my opinion it should not enforce "final" modifier for primitives and String, as String is immutable.

@sauravrao637
Copy link
Contributor Author

Any updates on this?

Copy link
Member

@Redirion Redirion left a comment

Choose a reason for hiding this comment

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

it still feels like fixing the symptom but not the cause. But it is better than crashing and its well commented.

@Redirion Redirion merged commit 5b4fbe3 into TeamNewPipe:dev May 12, 2021
This was referenced May 23, 2021
@sauravrao637 sauravrao637 deleted the fixE6124 branch June 4, 2021 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashing When Deleting From Playlist Tab
4 participants