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

Refresh all home view data #1195

Merged
merged 6 commits into from
Apr 14, 2023
Merged

Conversation

1hitsong
Copy link
Member

@1hitsong 1hitsong commented Apr 12, 2023

Changes

When refresh is called, refresh all home data, for all rows.

This rolls back changes made to homerows.brs that converted the data loads from linear to async.

Issues

Fixes #1184

@jellyfin-bot jellyfin-bot added this to In progress in Ongoing development Apr 12, 2023
Copy link
Member

@cewert cewert left a comment

Choose a reason for hiding this comment

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

Not sure if this is new but I broke it.

  • I played a show from Next up for a while then hit back and the row went away like it should.
  • Then I resumed that show from continue watching and ff till the end and let it finish.
  • After a few seconds of watching the next episode, I hit back and the home rows got messed up. Next up and favorites were missing and the sizeArray was messed up I think because the My Media and continue watching row had the wrong subtitles

PXL_20230413_022609065 MP
PXL_20230413_022539743 MP

Ongoing development automation moved this from In progress to Review in progress Apr 13, 2023
@cewert
Copy link
Member

cewert commented Apr 13, 2023

I think the issue I ran into is because we are no longer chaining the tasks together but the task logic still assumes we are?

onLibrariesLoaded() doesn't set the sizeArray till the very end of the function on line 141 m.top.rowItemSize = sizeArray but it's possible that the three tasks fired on lines 86-96 are coming back with data before then and also trying to set the sizeArray

Edit: Another thing that may be messing things up is that when I wrote #324 I was hardcoding the My Media row as the first row and NOT updating it during the home screen refresh. That's why the old code was calling m.LoadContinueTask instead of m.LoadLibrariesTask

@cewert
Copy link
Member

cewert commented Apr 13, 2023

After more thinking about this, I'm certain that firing off all the task nodes at once is a very bad idea. If ANY two task nodes finish at the same time, there will be two functions trying to modify the sizeArray at the same time i.e. updateNextUpItems() updateContinueItems() updateFavoritesItems()

That's why onLibrariesLoaded() loops through all the libraries to create "Latest in" rows before updating the sizeArray, because we have no idea how many rows to create until the loop is over.

My vote would be going back to the old way of doing things instead of refactoring this like crazy. The sizeArray is a PITA

@cewert
Copy link
Member

cewert commented Apr 13, 2023

The old way of doing things on first first load:

  • Run onLibrariesLoaded() only once when the app is first loaded
  • Use onLibrariesLoaded() to mock up empty rows for all of the screen's content
  • Fire task nodes to retrieve data from API and update the empty row

The old way of doing things on refresh:

  • (Rows are already mocked up on first load and rows can't change since we aren't refreshing the user's library)
  • Fire the m.LoadContinueTask task node
  • When updateContinueItems() is finished update the Continue Watching row and then fire the m.LoadNextUpTask task node
  • etc. (chaining the task nodes prevents us from having two functions modify the sizeArray at the same time.

@jellyfin-bot jellyfin-bot moved this from Review in progress to In progress in Ongoing development Apr 13, 2023
Copy link
Member

@cewert cewert left a comment

Choose a reason for hiding this comment

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

The rows refresh like they should and the sizeArray isn't getting messed up now 👍

When I finish a video and am kicked back to the home screen, the continue watching animation is being shown twice. I'm not sure if the rows are refreshing twice or it's just the animation. Sent you a video on matrix. If this isn't a regression we can wait till next release

Ongoing development automation moved this from In progress to Review in progress Apr 13, 2023
@jellyfin-bot jellyfin-bot moved this from Review in progress to In progress in Ongoing development Apr 13, 2023
Ongoing development automation moved this from In progress to Reviewer approved Apr 14, 2023
Copy link
Member

@cewert cewert left a comment

Choose a reason for hiding this comment

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

🚀

@cewert cewert merged commit fea5c6f into jellyfin:unstable Apr 14, 2023
8 checks passed
Ongoing development automation moved this from Reviewer approved to Done Apr 14, 2023
@p37307
Copy link

p37307 commented Apr 14, 2023

@cewert, curious how long until this fix (and others, if any) will take to come down to the Roku store app as an update. This issue, #1184, is a major frustrating one. Thanks for picking it up and working on it so quickly. Also, I'm willing to try Beta apps, if any. I have several, different models, Roku's.

@cewert
Copy link
Member

cewert commented Apr 14, 2023

@p37307 generally speaking the answer is "when it's ready" ™️.

That said we've been working towards releasing a hotfix ever since we discovered how bad #1177 was a few days ago. Then we discovered this issue and decided to add it in with the hotfix. We currently have no plans of adding anything else to this hotfix release. So assuming that doesn't change, you should see it on your roku devices within a week

@1hitsong 1hitsong deleted the refreshHomeRows branch April 14, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

"Next Up" home row not updating
3 participants