NewPipe icon indicating copy to clipboard operation
NewPipe copied to clipboard

[Code] Switching from RV-Adapter to ListAdapter for local item lists

Open vkay94 opened this issue 5 years ago • 5 comments

Checklist

  • [x] I checked, but didn't find any duplicates (open OR closed) of this issue in the repo.
  • [x] I have read the contribution guidelines given at https://github.com/TeamNewPipe/NewPipe/blob/HEAD/.github/CONTRIBUTING.md.
  • [x] This issue contains only one feature request. I will open one issue for every feature I want to request.

Describe the feature you want

I'd like to recommend switching from the current implementation of local item lists with RecyclerView.Adapter's notify-callbacks to RecyclerView's ListAdapter with its submitList-callbacks and DiffUtil for efficiently of displaying changed items.

Is your feature request related to a problem? Please describe it

There is no explicit problem but it is less main thread operation consuming when updating single list item instead of reloading/rebinding the whole list.

Additional context

Currently, when there is a change in the HistoryRecorder-tables the whole list will be re-rendered in the history-fragment. You can see it well when you

  1. switch to history fragment after populating some streams into it, then
  2. open a video in popup and
  3. start changing the progress by double-tap to seek or seeking through the seekbar.

You'll notice that all items are 'flashing' when the progress has been saved to the database - in other words, all items are re-rendered even if there is only a single item which needs an update.

This kind results in even more laggy UI feedback when I've tried to implement #4264 which updates the database even more frequently (and holding me back opening a PR for the feature).

I already did some research and it works for the usual operations within the app (adding, deleting, swaping and updating; tried on history and playlist fragments) including the header.

How will you/everyone benefit from this feature?

Using DffUtil is a more efficient way than calling notifyDatasetChanged() or the other updating methods and consumes less main thread time

vkay94 avatar Oct 31 '20 12:10 vkay94

I hate how the current implementation works, so I am completely in favour of this :-)

Stypox avatar Oct 31 '20 20:10 Stypox

@Stypox Alright, so I'm focusing on this (to optimize) and opening a PR in the near future ;) Basically, it's all done regarding local listings: app-debug-local-listadapter.zip

What I've done/changed:

  • Switched the implementation from notify-callbacks (especially removed (almost all) notifyDataSetChanged calls) to AsyncDiffer/ListAdapter (not really surprising :))
  • Update latestAccessDate-field during seeking (previously it was updated only when opening and closing a stream), otherwise it can't be tracked in history fragment
  • "Correct" animation in history fragment when deleting a stream
  • Smoothen update animation when the order in history fragment is changed by modifying the DefaultItemAnimation class of RecyclerView (looks great (my opinion)).

The only problem I'm facing now is that you can click on the "change order"-view during the update, so it's not blocked (well, nobody will click on it multiple times within a short time frame but it should be blocked somehow for this period, shouldn't it?)

vkay94 avatar Nov 02 '20 13:11 vkay94

Update latestAccessDate-field during seeking (previously it was updated only when opening and closing a stream), otherwise it can't be tracked in history fragment

Great! Maybe you could also update it while playing, but I maybe it's a better idea not to, as not to introduce performance issues in the player

"Correct" animation in history fragment when deleting a stream Smoothen update animation when the order in history fragment is changed by modifying the DefaultItemAnimation class of RecyclerView (looks great (my opinion)).

Looks good!

The only problem I'm facing now is that you can click on the "change order"-view during the update, so it's not blocked (well, nobody will click on it multiple times within a short time frame but it should be blocked somehow for this period, shouldn't it?)

If that does not create crashes then it wouldn't be much of a problem, as far as consistency is kept (i.e. the button shows what it does), but if you can fix it the better

Stypox avatar Nov 02 '20 14:11 Stypox

Great! Maybe you could also update it while playing, but I maybe it's a better idea not to, as not to introduce performance issues in the player

I think updating the progress periodically wouldn't be necessary. It won't be noticeable when the player is opened (except popup player). Seeking and closing should be enough when there is a greater change.

If that does not create crashes then it wouldn't be much of a problem, as far as consistency is kept (i.e. the button shows what it does), but if you can fix it the better

it doesn't crash in the first place, it just doesn't look good if the items are overlapping (that's the downside of the asynchronous ListAdapter - you can't abort except with notifyDataSetChanged which should be prevented if possible :'))

vkay94 avatar Nov 02 '20 14:11 vkay94

I think updating the progress periodically wouldn't be necessary

Ok, I agree

it doesn't crash in the first place, it just doesn't look good if the items are overlapping

Then if you manage to block clicks on the reorder button the better, but otherwise it wouldn't be a problem, at the end of the day the user has to be really nitpicking in order to criticize an animation which does not work as intended if he keeps clicking.

Stypox avatar Nov 02 '20 16:11 Stypox