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

Do not switch screens when clicking 'Remove podcast' #6259

Merged
merged 8 commits into from Apr 7, 2023

Conversation

gitstart
Copy link
Contributor

@gitstart gitstart commented Jan 3, 2023

Issue

Closes #5909

Steps to reproduce

Currently, removing a podcast from within the podcast or from the Subscription list in the sidebar (but not from the Subscriptions screen) ends with the user being shown the Episodes screen.
In addition, the user is taken to the Episodes screen even before they have confirmed they actually want to remove the podcast.

Issue Video:

https://www.loom.com/share/34ba40d96dd244e99c25f3b6c11af722

Video After Fix:

https://www.loom.com/share/ed3abd70e03c4f07aedfb663145ed513


This code was written and reviewed by GitStart Community. Growing future engineers, one PR at a time.

@gitstart gitstart marked this pull request as ready for review January 3, 2023 09:37
@ByteHamster ByteHamster added this to the 3.1.0 milestone Jan 3, 2023
@ByteHamster
Copy link
Member

As it is now, this basically reverts de8496f, which was added deliberately to fix a bug. There need to be additional changes that avoid crashes here.

@gitstart
Copy link
Contributor Author

Thank you @ByteHamster for reviewing the PR,

Implementation in this PR is bit different from the implementation mentioned in comment, please have a look at following difference.

In this PR callback triggered as soon as user confirms deletion before actual deletion from DB while in the implementation you shared in link callback triggers after actual deletion process.

I also tried with that implementation and faced crash but with current implementation i tested with 45+ subscriptions and crash did not appeared.

image

image

Thank you again..!!

@ByteHamster
Copy link
Member

In this PR callback triggered as soon as user confirms deletion before actual deletion from DB

The callback is called before actual deletion, but I doubt Android completely destroys the fragment synchronously before deletion starts. Or does it? The fragment either needs to be completely destroyed before deleting or the fragment needs to be able to deal with the case that its own podcast gets deleted.

@gitstart
Copy link
Contributor Author

In this PR callback triggered as soon as user confirms deletion before actual deletion from DB

The callback is called before actual deletion, but I doubt Android completely destroys the fragment synchronously before deletion starts. Or does it? The fragment either needs to be completely destroyed before deleting or the fragment needs to be able to deal with the case that its own podcast gets deleted.

Thank you @ByteHamster for reviewing this,
Regarding, the fragment gets destroyed before deletion and stops listening for events, so in this implementation we don't observe a crash, while if we add callback after deletion the destroy process and update UI (Fragment listen to event) process takes places simultaneously or during updating fragment get destroyed and crash appears.

image

@ByteHamster ByteHamster changed the title Do not switch screens when clicking "Remove podcast" #5909 Do not switch screens when clicking "Remove podcast" Feb 11, 2023
@ByteHamster
Copy link
Member

ByteHamster commented Apr 7, 2023

Regarding, the fragment gets destroyed before deletion and stops listening for events

Is this just an observation that you make by looking at logs or is it actually specified to be that way?

@ByteHamster ByteHamster merged commit a828660 into AntennaPod:develop Apr 7, 2023
7 checks passed
@keunes keunes changed the title Do not switch screens when clicking "Remove podcast" Do not switch screens when clicking 'Remove podcast' Aug 20, 2023
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.

Do not switch screens when clicking "Remove podcast"
2 participants