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

Implement scene manager for interacting with groups and retain view/sort/filter settings #492

Merged
merged 13 commits into from
Oct 17, 2021

Conversation

jdlayman
Copy link
Contributor

Changes
Implement a scene manager which controls scene information throughout app, including overlay and content. Content groups are managed with a private stack. This new scene manager controls a new "content" node within JFScene node, placing the top group from the stack in the node at all times. Scene manager also observes the overlayTitle for the active group and updates the title within the overlay on changes (fixes my title update conundrum from #486). Additionally, the overlay is now displayed above the content node (would have been very hard to do in old code) which prevents problems where text gets covered and allows for posters and other content to more easily blend with overlay.

This also allows us to remove a lot of UI-related events from the main loop (like backPressed). I expect there are even more opportunities to improve this over time. The most obvious seems like all of the itemSelection code, which could move into handlers for ItemGrid and other SceneGraph nodes.

Additionally, we should be better situated for other options bars or other UI constructs in the future.

Issues
Continuation of PR #486. If this change is accepted, we can close #486 without merging.

@neilsb
Copy link
Member

neilsb commented Oct 16, 2021

This is great. A good amount of code removed from the Main loop is a great start (would love to get the main loop down to just a few lines in time 😄)

Changes all look good, and I've been testing it for a bit and not come across any issues with what's been done.

I've spotted one issue, that might exist in other places within app too however. The place I've encountered it is for TV Shows, on the screen that lists the seasons. When a season is selected, the call to CreateSeasonDetailsGroup creates the screen, loads the data from the server, then returns screen to be pushed into the stack.

Previously, as soon as a season was selected the existing screen was hidden, the user got a blank screen while the episode list loaded. Now the original screen remains in place while the episodes are loaded, allowing the user to click again, or even navigate to another season and click again. These season screens then stack up and the "back" button takes them to another season screen, rathe than the TV Show screen.

e.g.

  • Goto a TV Program
  • Double click on Season 1 and wait for the episode list to be shown (and wait the same amount of time again)
  • 2 Season 1 screens have been pushed on the stack
  • Press the back button and you'll stay on Season 1.
  • Pressing back again takes you to the TV Show

For TV Shows with a large number of episodes per season where it takes a while to load, the user can select several other seasons and then they all open on top of each other.

I'm thinking the fix to this is to ensure that screens do any loading of data using a Task so screen can be added immediately and a loading spinner can be shown while the data loads in the background (as the ItemGrid does)?

Although, doing the call to screenmanager push from within the CreateSeasonDetailsGroup as soon as it's created would probably alleviate the problem in the short term and make it work like it currently does (although longer term would still be better to have the Screen load the data it needs by itself in a Task). What do you think?

I've just reproduced the same thing when selecting a TV Show from the main ItemGrid screen by double clicking and item.

@jdlayman
Copy link
Contributor Author

Good catch! Let me play with it some later and see what I think. Definitely agree with your long term solution.

@jdlayman
Copy link
Contributor Author

I pushed a change that does what you suggested and it works as a short term solution. I occasionally see screens that aren't populated with data quite yet, but that seems better than the page stack confusion that can be created by allowing for more button presses on previous scene.

Longer term, I think this will go away when we move things out of the main loop for processing. When we move processing item selection into the SG/render thread, I don't think the Roku environment will even let you call ParseJson() within a SceneGraph component. This will push us to the more correct solution of using task thread and implementing a loading spinner.

Copy link
Member

@neilsb neilsb left a comment

Choose a reason for hiding this comment

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

All looks good now.

@neilsb neilsb merged commit eacadf1 into jellyfin:master Oct 17, 2021
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.

2 participants