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

[Estuary] Fix home categories focus position #24000

Merged
merged 3 commits into from
Oct 29, 2023
Merged

[Estuary] Fix home categories focus position #24000

merged 3 commits into from
Oct 29, 2023

Conversation

Hitcher
Copy link
Contributor

@Hitcher Hitcher commented Oct 25, 2023

Description

This fixes the problem where the 'Now playing' (for Movies) and 'Next Aired' (for TV Shows) would get focus even though they're at the end of the categories list.

Motivation and context

It seems that in a mixed content type list the static content gets focus ahead of the dynamic content. By simply placing the static items at the front of the categories list means the focus always goes to the front of the list.
Problem reported on the forum - Category Icons at top is right aligned for some reason
Fixes this issue - #20139

How has this been tested?

Tested and works as expected.

What is the effect on users?

Screenshots (if appropriate):

Before PR:
The categories list is offset on load
screenshot00004
And the first item doesn't get selected
screenshot00005

After PR:
The categories list is correctly aligned
screenshot00006
And the first item gets selected
screenshot00007

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

This fixes the problem where static content gets focused first in mixed content lists by simply moving it in front pf the dynamic content. Problem was raised on the forum - https://forum.kodi.tv/showthread.php?tid=374267
@Hitcher Hitcher requested a review from ksooo October 25, 2023 14:01
@Hitcher Hitcher added Type: Fix non-breaking change which fixes an issue Component: Skin labels Oct 25, 2023
@Hitcher Hitcher self-assigned this Oct 25, 2023
@Hitcher Hitcher added this to the Omega 21.0 Beta 2 milestone Oct 25, 2023
@ksooo
Copy link
Member

ksooo commented Oct 27, 2023

@HitcherUK you selected me for review, but honestly, I have no idea what consequences the change you made could have. My skinning skills are quite basic, I'd say.

Copy link
Member

@garbear garbear left a comment

Choose a reason for hiding this comment

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

I guess I can approve, I tested this and confirmed that it fixed the problem.

@Hitcher
Copy link
Contributor Author

Hitcher commented Oct 27, 2023

Sorry ksooo and thanks garbear.

@Hitcher Hitcher changed the title Fix home categories focus position [Estuary] Fix home categories focus position Oct 29, 2023
@enen92 enen92 merged commit 950a081 into xbmc:master Oct 29, 2023
1 check passed
@rainman74
Copy link

Could this also be made available for Nexus, please?

@garbear
Copy link
Member

garbear commented Oct 29, 2023

I support a backport. There's a small conflict introduced by #23814 but it's not too hard to resolve.

@HitcherUK Can you backport? I can help take care of the rebase conflict if you have any problems.

@Hitcher
Copy link
Contributor Author

Hitcher commented Oct 29, 2023

There's a small conflict introduced by #23814 but it's not too hard to resolve.

With this PR?

Can you backport?

Not sure how to; any pointers?

@garbear
Copy link
Member

garbear commented Oct 29, 2023

Not sure how to; any pointers?

Sure, to avoid the conflict without any complicated git resolving magic you can do a poor man's backport - create a branch based on Nexus, make the same change by hand, then send a PR.

Create a new branch based on the current tip of the Nexus branch (make sure to fetch from upstream first):

git checkout -b backport-fix-categories 618d1e35d89f1c49c2a37f5d233319f3f3bbe01b

You'll then have a branch named backport-fix-categories based on upstream Nexus.

Then, make the same change by hand and commit it.

Then push the branch to your repo

git push --set-upstream origin backport-fix-categories

Then you can PR the backport-fix-categories branch. Make sure to target the upstream Nexus branch when creating the PR. You'll see a big mess, but when you target Nexus then you'll see a clean PR with just your commit and the PR title neatly filled out.

@Hitcher Hitcher mentioned this pull request Oct 29, 2023
14 tasks
@garbear
Copy link
Member

garbear commented Oct 29, 2023

Close enough 😉 You missed the "target Nexus" part so I edited the PR. You can see how now it's clean against Nexus. Thanks!

@Hitcher
Copy link
Contributor Author

Hitcher commented Oct 29, 2023

@garbear sorry, I don't do command line.

Had a go but I think there's wrong as it says Files changed 599 - #24027

@garbear
Copy link
Member

garbear commented Oct 29, 2023

Check now all fixed!

@Hitcher
Copy link
Contributor Author

Hitcher commented Oct 29, 2023

That was quick, thanks.

@garbear
Copy link
Member

garbear commented Oct 29, 2023

You rocked the git part! But I'm not surprised the confusing GitHub interface tripped up the backport, it's bitten me before too.

@KarellenX
Copy link
Member

Nice. I was wondering why it displayed this way. I made the change to my local install, and all working correctly again.

@rainman74
Copy link

@HitcherUK
Something seems to have gone wrong, I can't find the PR in the xbmc:Nexus branch.

@Hitcher
Copy link
Contributor Author

Hitcher commented Nov 3, 2023

Something seems to have gone wrong, I can't find the PR in the xbmc:Nexus branch.

@garbear any ideas?

@jjd-uk
Copy link
Member

jjd-uk commented Nov 3, 2023

Well that's expected as PR24027 is still closed, it needs reopening, then approval, and merging.

@Hitcher
Copy link
Contributor Author

Hitcher commented Nov 3, 2023

Could have sworn it was merged. I'll try and sort it over the weekend.

@Hitcher Hitcher mentioned this pull request Nov 3, 2023
14 tasks
@joseluismarti joseluismarti mentioned this pull request Dec 4, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport: Done Component: Skin Type: Fix non-breaking change which fixes an issue v21 Omega
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants