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

Add home button when away from home view #3262

Merged
merged 9 commits into from
Apr 11, 2024

Conversation

VizuaaLOG
Copy link
Contributor

Changes
Added a home button to the top toolbar when away from the home view, allowing for a quick "back to home" feature. The Jellyfin web UI and android mobile app has this. Having this button provides a quick way to leave a long navigation chain without having to press the back button multiple times.

The home button has been added to the ClockUserView as this appears to already be shown on all views although it does make the name of the view a little misleading.

Issues
Closes #2053 and #2717

#2053 does mention that new navigation features may provide a resolution for this in future. However, given the nice quality of life this provides I decided to tackle it as my first PR, as I also have frustrations with this.

@nielsvanvelzen
Copy link
Member

Did you test these changes? Running this in the emulator and a lot of screens have issues, either not being able to focus the button or the button being behind the screen title.

@VizuaaLOG
Copy link
Contributor Author

I did both in an emulator and on my Philips TV 🤔 . I will look into it

@VizuaaLOG
Copy link
Contributor Author

VizuaaLOG commented Jan 6, 2024

@nielsvanvelzen Testing in a 4k and 1080p emulator.

The examples I have found are both when using the smart screen mode (at least this is how I found the buttons) and I navigate to the "By Letter" or "Genres" screen for each, the library name overlaps the home button, this is also the screens where I cannot focus either.

Are there other instances you have seen that I am missing?

I'm also thinking that the title on those screens should be left aligned? As shorter titles puts "Movies", for example, on the right which seems a little odd 🤔

**Edit: ** I see these screens are using an Android TV based library so the title is controlled by that, which I think is probably why it cannot be focused as clicking the home button with a mouse works. The right title is an Android design choice I guess

* Move clock view rendering to the new view instead of in the Browse Folder Fragment
@VizuaaLOG
Copy link
Contributor Author

VizuaaLOG commented Jan 6, 2024

I have had an attempt at resolving this. Android development isn't my expertise, so please do let me know if there is a better way to accomplish what I have done.

I believe the issue with the overlap/no focus was because the clock and user icon view was being added to the browse frame and then positioned to appear in the title area. After lots of research on how Leanback works it appears the recommendation, to allow for focusing, is to create a custom TitleView / BrowseTitleView and use the theme to replace it. This will then mean in terms of hierarchy the clock is now in the title, and so focusing it by pressing up would be possible.

This change has meant the BrowseFolderFragment no longer needs to add the clock/rearrange the title in the onStart call, instead this is now being handled by a normal view and the TitleView class.

From my research this appears to only affect the "ByLetter" and "ByGenre" screens, other screens in my testing appear to work okay and are focusable and do not have issues with titles overlapping.

@nielsvanvelzen
Copy link
Member

Another round of testing:

  • In the "now playing" screen for music, moving the focus to the home icon introduces some weird scroll behavior
  • The spacing between the title and the home icon is too little in the folder views (e.g. genre view)
  • The video player should not have the home icon
  • Using the home icon doesn't clear navigation history properly. When I open the app, navigate to somewhere, then use the home icon and after that use the back button on my remote it doesn't close the app

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jan 28, 2024
Thomas Erbe added 2 commits January 28, 2024 13:47
…tton

* Add some spacing between the folder view title and the clock user view
* Add missing line at end of TitleView
# Conflicts:
#	app/src/main/java/org/jellyfin/androidtv/ui/playback/AudioNowPlayingFragment.java
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jan 28, 2024
@VizuaaLOG
Copy link
Contributor Author

VizuaaLOG commented Jan 28, 2024

In the "now playing" screen for music, moving the focus to the home icon introduces some weird scroll behaviour

I think I have fixed this. Was caused by the focus handler not being aware the home button is also a focusable button now, not just the queue row

The spacing between the title and the home icon is too little in the folder views (e.g. genre view)

Increased the spacing slightly - added a 16dp margin - let me know if you would prefer more or less

The video player should not have the home icon

I did think about this when adding the button initially and I actually thought it could be useful to be there. Given an instance of finishing a movie or episode of a show, if the user would like to watch, listen, or view something else entirely they're able to just go home straight from the video player. Saving them from needing to press back first, small saving granted. If you would prefer it removed I can though.

Using the home icon doesn't clear navigation history properly.

This one took some digging to find. I think I have found the issue, however, I'm not sure how to resolve it, so any ideas would be appreciated!

The issue appears to be caused by the Navigation Repository's reset method not clearing the back stack in the supportFragmentManager. I see reset is called in some other instances and while I'm not 100% sure how to trigger them I think they may have a similar issue too? The deeper you are into the app, thus the higher that count in the back stack, the more you have to press back when on the home screen - as the back handler pops the stack. Once it hits zero, it'll exit the app.

I tried to see if there was a way to just reset the stack but I had trouble finding a method for that. I suppose an option would be too loop over the stack and pop it until empty but that didn't feel like the best solution so I thought I'd see if you have any ideas?

@nielsvanvelzen
Copy link
Member

  • In the music player, the title can overlap with the home icon

I did think about this when adding the button initially and I actually thought it could be useful to be there. Given an instance of finishing a movie or episode of a show, if the user would like to watch, listen, or view something else entirely they're able to just go home straight from the video player. Saving them from needing to press back first, small saving granted. If you would prefer it removed I can though.

Navigation is a part of browsing, the video player is a completely separate view. So yes it should be removed.

I tried to see if there was a way to just reset the stack but I had trouble finding a method for that. I suppose an option would be too loop over the stack and pop it until empty but that didn't feel like the best solution so I thought I'd see if you have any ideas?

The reset function is called by the MainActivity when it's fragment history is reset, not the other way around. So you'll need to add new behavior to the navigation code to deal with this. I don't have any suggestions for that.

Thomas Erbe added 2 commits March 3, 2024 13:20
…y reset and to allow for the back button to quit the app properly after navigating home.

* Fix the artist title overlapping the home button on the music now playing screen.
@VizuaaLOG
Copy link
Contributor Author

Apologies for the delay!

  • The home button is now hidden from the video player.
  • The artist title in the music player has been fixed so shouldn't overlap now.
  • I have also added a new GoHome navigation action which clears the back stack to fix the issue with the back button not exiting the app when using the home button.

Thanks!

@nielsvanvelzen nielsvanvelzen added enhancement New feature or request release-highlight Pull request might be suitable for mentioning in the release blog post labels Apr 11, 2024
@nielsvanvelzen nielsvanvelzen added this to the v0.17.0 milestone Apr 11, 2024
@nielsvanvelzen
Copy link
Member

Thank you for the pull request! I'm sure a lot of users will love this feature.

Instead of commenting on the small details I've added an additional commit that fixes some styling issues and merged the "goHome" function into the existing "reset" function in NavigationRepository.

@nielsvanvelzen nielsvanvelzen merged commit d7b5689 into jellyfin:master Apr 11, 2024
6 checks passed
@VizuaaLOG VizuaaLOG deleted the home-button branch April 11, 2024 20:03
@VizuaaLOG
Copy link
Contributor Author

Thank you for the pull request! I'm sure a lot of users will love this feature.

Instead of commenting on the small details I've added an additional commit that fixes some styling issues and merged the "goHome" function into the existing "reset" function in NavigationRepository.

I know I'll love it when I can't decide what to watch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request release-highlight Pull request might be suitable for mentioning in the release blog post
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global back button
3 participants