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

Fix navigating back from an actor page should keep focus on the actors row #2788

Closed

Conversation

fedesenmartin
Copy link

@fedesenmartin fedesenmartin commented May 23, 2023

Changes

  • Use childfragmentManager instead of supportfragmentmanger at FullDetailsFragment.java and AudioNowPlayingFragment.java
  • indentation for FullDetailsFragment.java

Issues:
Fixes #2713

@nielsvanvelzen nielsvanvelzen self-requested a review May 23, 2023 17:23
@fedesenmartin fedesenmartin changed the title Fix Navigating back from an actor page should keep focus on the actors row Fix navigating back from an actor page should keep focus on the actors row May 23, 2023
@nielsvanvelzen
Copy link
Member

It looks like the same issue also exists in the AudioNowPlayingFragment. Perhaps you can fix that file in this same pull request.

@nielsvanvelzen nielsvanvelzen added the bug Something isn't working label May 24, 2023
@@ -59,7 +59,11 @@ class MyDetailsOverviewRowPresenter(
}

binding.fdButtonRow.removeAllViews()
for (button in row.actions) binding.fdButtonRow.addView(button)
for (button in row.actions) {
if (button.parent != null) {
Copy link
Member

@nielsvanvelzen nielsvanvelzen May 24, 2023

Choose a reason for hiding this comment

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

This check makes the buttons never show at all.

edit: to properly fix the crash that exists without this check we should update the "MyDetailsOverViewRow" to only contain data and not views. So the TextUnderButon is instantiated in the presenter.

Copy link
Author

@fedesenmartin fedesenmartin May 26, 2023

Choose a reason for hiding this comment

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

removed from "MyDetailsOverViewRow" all views; but without this check if (button.parent != null) I cant get rid off this crash -> java.lang.IllegalStateException: The specified child already has a parent. You must call removeView() on the child's parent first.

Copy link
Member

Choose a reason for hiding this comment

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

You're storing the views in the presenter instead of the viewholder right now. That means views are still re-used. You need to make sure to never re-use the views and scope them to the viewholder.

@fedesenmartin fedesenmartin marked this pull request as draft May 26, 2023 18:07
refactor to fulldetailsfragment and indentation
use getChildFragmentManager on AudioNowPlaying fragment instead of supportfragment manager
@fedesenmartin fedesenmartin marked this pull request as ready for review May 29, 2023 14:44
@fedesenmartin fedesenmartin marked this pull request as draft May 29, 2023 14:48
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jun 3, 2023
@nielsvanvelzen
Copy link
Member

Hey @fedesenmartin, are you still working on this PR/planning to finish it?

@fedesenmartin
Copy link
Author

I am not actively working on the fix. I have an idea on how to solve it, but it requires rewriting the entire view. I will continue it when I have a moment, unless there is a significant change in the code. I will leave it as a draft if that's okay with you, in case someone wants to continue it. Does that sound okay to you? @nielsvanvelzen

@nielsvanvelzen
Copy link
Member

While fixing a crash in the initial 0.16 beta I made similar changes as this PR (#2939, #2940 and #2943). The mentioned issue (#2713) might be fixed now in the master branch. I've not tested it myself though.

@nielsvanvelzen
Copy link
Member

Closing this PR as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working merge conflict Conflicts prevent merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigating back from an actor page should keep focus on the actors row
4 participants