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

Migrate other screens to new episode multi-select #5250

Closed
peakvalleytech opened this issue Jul 1, 2021 · 23 comments
Closed

Migrate other screens to new episode multi-select #5250

peakvalleytech opened this issue Jul 1, 2021 · 23 comments

Comments

@peakvalleytech
Copy link
Contributor

peakvalleytech commented Jul 1, 2021

Checklist

  • [ x ] I have used the search function to see if someone else has already submitted the same feature request.
  • [ x ] I will only create one feature request per issue.
  • [ x ] I will describe the problem with as much detail as possible.

System info

App version: 2.2.1

App source: Google Play / F-Droid / ...

Feature description

Problem you may be having, or feature you want:

PR #5130 was recently merged. It introduced the new multi-select feature replacing the previous and it only applies to the FeedItemListsFragmgment. The other fragments still rely on old multi-select.

Suggested solution:

I want to open a new feature to migrate the new multi-select to the other fragments.

I know there have been a lot of discussion about major changes to the UI. So i am not sure which screens will still exists when the once the new changes are implemented.
The only one I am a high degree of certaintiy will still be around is the queue. I am considering starting with that. If anyone, especially @ByteHamster or @keunes, would like to list all the screens that new the new multi select, that would be awesome. Actually, it's not really a request. Otherwise, I would not be implement any of the screens :)

@ByteHamster
Copy link
Member

I think the downloads screen will also stay - so yeah, both downloads screen and queue should be migrated :)

@keunes
Copy link
Member

keunes commented Jul 1, 2021

The All episodes screen was requested to have multi-select on the forum. I think that makes sense as well.

I'm just wondering: what are the 'major UI changes' we are talking about here? Are that the swipe actions?
Or you are wondering about which screens might disappear? The only one that I would think of is the New episodes screen/tab (Inbox) and I'm not too sure about Favourites (so better to wait with that one, too). All the rest I reckon would be 'safe'.

@antennapod-bot
Copy link

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

https://forum.antennapod.org/t/files-added-from-local-folders-are-not-added-to-queue/1089/5

@ByteHamster
Copy link
Member

"All episodes" will be harder to implement but (after the others are done) would be great, yeah. The reason is that it dynamically loads more items when scrolling to the bottom.

@antennapod-bot
Copy link

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

https://forum.antennapod.org/t/files-added-from-local-folders-are-not-added-to-queue/1089/6

@peakvalleytech
Copy link
Contributor Author

peakvalleytech commented Jul 1, 2021

The All episodes screen was requested to have multi-select on the forum. I think that makes sense as well.

Yes, I was just looking at that issue.
But i'm not sure if it will be replaced by the inbox screen. I'll have to wait to see what @ByteHamster says. He mentioned only the Queue and the Downloads.

I'm just wondering: what are the 'major UI changes' we are talking about here? Are that the swipe actions?

Well, i don't think swip actions would be a factor in deciding what screen to implement multi select.
I was manily concerned about the episodes screen, which I am assuming might be replaced by the new Inbox screen.

Or you are wondering about which screens might disappear? The only one that I would think of is the New episodes screen/tab (Inbox) and I'm not too sure about Favourites (so better to wait with that one, too). All the rest I reckon would be 'safe'.

Yes, that is pretty much it. Like i said, @ByteHamster has only mentioned the Qeue and the Downloads screen. I'm going to start with those first.

@peakvalleytech
Copy link
Contributor Author

"All episodes" will be harder to implement but (after the others are done) would be great, yeah. The reason is that it dynamically loads more items when scrolling to the bottom.

Won't "All episodes" be changed in the UI update?

@peakvalleytech
Copy link
Contributor Author

peakvalleytech commented Jul 10, 2021

I think this issue should be closed. What do you think @ByteHamster @keunes?

@keunes
Copy link
Member

keunes commented Jul 11, 2021

So we now have 3 tabs in the Episodes screen. What we know:

  • 'New' will be replaced by Inbox
  • 'All' will certainly stay
  • 'Favorites' might be moved to a separate screen, but that's not certain yet.

So I think this should be kept open, as multiselect would be great to have on the All tab.
'Favorites' should also get multi-select - whether it might be moved into a separate screen or not doesn't really matter I guess. @ByteHamster can confirm, but even if it gets its own screen, if you implement multi-select, it would be preserved.

@ByteHamster
Copy link
Member

Yeah, it should be preserved. The "all","favorites" and "new" screens all use the same parent fragment. So when implementing, it will already be done for all of them :)

even if it gets its own screen, if you implement multi-select, it would be preserved.

Yeah.


Multi-select on the episodes screens is a bit harder than on the other screens because of the lazy loading. If you select all items in the list (with the action button), it only selects the ones that are loaded. Not the ones that are still in the database but not loaded yet.

@keunes
Copy link
Member

keunes commented Jul 11, 2021

If you select all items in the list (with the action button), it only selects the ones that are loaded. Not the ones that are still in the database but not loaded yet.

If you scroll down to the end, would lazy loading then still work?

@ByteHamster
Copy link
Member

Yeah if you scroll all the way to the bottom before pressing the "select all" button, selection would work as expected (but that will take a long time). When manually ticking episodes, it should also work as expected.

@peakvalleytech
Copy link
Contributor Author

@ByteHamster @keunes Ok!, i'll send a new PR soon.

@peakvalleytech
Copy link
Contributor Author

@ByteHamster @keunes
Ok, I have considered various solutions. The one i'm considering implementing is to keep the lazyloading as is and only load all when 'select all' is selected.

But the problem with this solution is we need to know the total episodes but we don't have access to that before the user selects 'select all'. We can probably create a query that uses sqlite's 'count' function.

However this approach is assuming that the reason we are implementing lazyloading is primarly for performance. Otherwise, another apprach would gradually load all episodes in the background and update the number recycler. This would basically get rid of lazyloading at least in terms of user-initiated lazyloading.

@ByteHamster
Copy link
Member

The point of lazy loading is that for many users, the number of episodes is too large to hold all of them in RAM at the same time. Both of your approaches keep all episodes in RAM. That's why I said that the episodes screen is harder to do. The screen somehow needs to remember if "select all" or "select all below" was pressed and then afterwards, when applying the action, lazily load items and apply the change to them, too.

@peakvalleytech
Copy link
Contributor Author

peakvalleytech commented Jul 14, 2021

So are you implying that many of the users can't even scroll to the end? Wouldn't that be a bug that should be fixed? But you could be wrongly assuming how lazyloading is implemented for the fragment. Because I checked the code and when you load more items, you append them to the episodes lists. If the user keeps scrolling then they would eventually load all the episodes.

Maybe i'm misunderstanding something. Could you please clarify what you mean?

How much is too large? Each FeedItem object can't be that large. 10,000 episodes is probably not even 0.1 MB.

@ByteHamster
Copy link
Member

If the user keeps scrolling then they would eventually load all the episodes.

Yes but it would take hours of scrolling.

How much is too large? Each FeedItem object can't be that large. 10,000 episodes is probably not even 0.1 MB.

See #2312 and linked issues - the cause for that was that we (repeatedly) loaded all items from the database.

@peakvalleytech
Copy link
Contributor Author

But the apps still currently appends newly loaded items to existing items. So it seems that we can just leave it as is. Well. I don't get why 'select all' should be implemented if the user, in cases with low memory, can't even load all items.

Maybe the app can detect when users reached their memory threshold. And if they do, then prevent them from adding more feeds. With this approach, we can implement the suggested solution above. (Load all items when the user activates 'select all')

This may also resolve #2312.

@ByteHamster
Copy link
Member

I don't get why 'select all' should be implemented if the user, in cases with low memory, can't even load all items.

A user might still want to do something to all items - without having to load all of them at once. A method to do that would be to remember if "select all" or "select all below" was pressed and then when actually applying the action, apply the action in batches - load a few additional items, apply the action to them, no longer store them in RAM.

@peakvalleytech
Copy link
Contributor Author

peakvalleytech commented Jul 18, 2021

@ByteHamster
Ok, i'll try this soon.
So what currently happens when the user does scroll all the way down and they don't have enough RAM. Does the app crash?
If so should we create a new issue?

Edit: Also, we still need to somehow get the total number of items since it is used in the title.
I think we can create a query to and use sqlite's COUNT() function.

@peakvalleytech
Copy link
Contributor Author

@ByteHamster I intend to submit a PR eventually, but considering the extra complexity involved and that we are in the process of changing major parts of the UI, I would like to hold off on this for now. I would like to request this issue be close for now.

@keunes
Copy link
Member

keunes commented Dec 29, 2021

@ByteHamster Quite a few users seem in some way or another affected by the absence of multi-select on the all episodes screen. However, that feature request isn't recorded anywhere as such. Should I create a separate issue for it, or add it to the list of #5267? (I want to have a place where I can link to when relevant in forum, issues or on social media.)

@ByteHamster
Copy link
Member

Hmm, I re-opened this issue but actually, it is probably better to create a new one because multi-select on the "all episodes" screen would be a new feature (while this issue is about migrating an existing feature).

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

No branches or pull requests

4 participants