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

Replace old episode multi-select with new multi-select. #5253

Merged

Conversation

peakvalleytech
Copy link
Contributor

@peakvalleytech peakvalleytech commented Jul 2, 2021

Resolves issue #5250.

This is only preliminary. I know there is still a lot of cleaning up to do. The current commit mainly implements the new multi select for the QeueFragment and CompletedDownloadsFragment

Copy link
Member

@ByteHamster ByteHamster 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 working on this. A few things I noticed when playing around with it:

  • In the queue, the multi-select context menu item is not the top-most one. I think it should consistently be either the first or the last one on all screens.
  • In the queue, you can still drag around episodes. We only store the ID of the selected items (not the position) but it still feels a bit risky that episodes can be dragged in select mode.

@ByteHamster ByteHamster force-pushed the new-episode-multi-select-migration branch from e2a6512 to c289632 Compare July 9, 2021 19:51
@ByteHamster
Copy link
Member

I just did a little bit of cleanup and also moved the multi-select item to the bottom. I know it was on the top before but having to assign positions to every single item looks like too much overhead to me. Also, I think it is fine because multi-select is probably not the most-used action and should therefore not be the first one.

Looks pretty much ready to merge now. Do you still notice any issues?

@peakvalleytech
Copy link
Contributor Author

peakvalleytech commented Jul 9, 2021

I just did a little bit of cleanup and also moved the multi-select item to the bottom. I know it was on the top before but having to assign positions to every single item looks like too much overhead to me. Also, I think it is fine because multi-select is probably not the most-used action and should therefore not be the first one.

Looks pretty much ready to merge now. Do you still notice any issues?

Everything looks great. Although I did notice this problem, #5252. You can merge this PR now. I'll just create a new one to fix it.

Now assuming multi select for episodes is finished, we can start on multi select for feeds. There is already a PR currently opend targeted at multi select for feeds.
But that code in that PR has divereged so much from the new implementation that I think it is best to close it and create a new one.

@peakvalleytech peakvalleytech reopened this Jul 9, 2021
@ByteHamster ByteHamster merged commit 00bf2db into AntennaPod:develop Jul 10, 2021
@ByteHamster
Copy link
Member

Thanks! Will be released in AntennaPod 2.4.0 :)

@keunes
Copy link
Member

keunes commented Jul 10, 2021

Thanks a lot @peakvalleytech!

peakvalleytech added a commit to peakvalleytech/AntennaPod that referenced this pull request Jul 12, 2021
@antennapod-bot
Copy link

This pull request has been mentioned on AntennaPod Forum. There might be relevant details there:

https://forum.antennapod.org/t/antennapod-2-4-release-notes/1300/1

@antennapod-bot
Copy link

This pull request has been mentioned on AntennaPod Forum. There might be relevant details there:

https://forum.antennapod.org/t/antennapod-2-4-release-notes/1300/3

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.

4 participants