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

PLAYRTS-5578 - iOS 18 Update #512

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mutaben
Copy link
Contributor

@mutaben mutaben commented Aug 15, 2024

Description

  1. A crash was occurring when building against the iOS 18 SDK. On regular horizontal size class, the UITabBar adopts now a new design paradigm. An internal change caused the UITabBar to not live in the same view hierarchy anymore. This caused a crash within the AutoLayout engine because the two views we were trying to constrain together didn't have a common ancestor.
  2. The new UITabBar design paradigm caused issues with the Mini Player and the GoogleCast button positions on regular horizontal size class.

This PR solves both issues.

Changes Made

Both issues were fixed by:

  • Aligning the Mini Player to the safe area on regular horizontal size class, since the UITabBar is not there anymore anyway. On compact size class, we have to rely on computing the UITabBar's height in order to achieve the same result as the current version.
  • The GoogleCast button was misplaced on regular horizontal size class as well, since the new floating UITabBar moves the safe area. It's now fixed manually, which isn't ideal so I'm open to better ways to do it.

Checklist

  • I have followed the project's style guidelines.
  • I have performed a self-review of my own changes.
  • I have made corresponding changes to the documentation.
  • My changes do not generate new warnings.
  • I have tested my changes and I am confident that it works as expected and doesn't introduce any known regressions.
  • I have reviewed the contribution guidelines.

@mutaben mutaben added improvement Feature or update (issue and PR) - release notes section bug Issue to be fix (issue and PR) - release notes section labels Aug 15, 2024
@mutaben mutaben requested review from defagos and pyby August 15, 2024 15:19
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYRTS-5578-ios18-update August 15, 2024 15:22 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYRTS-5578-ios18-update August 15, 2024 15:23 Inactive
Copy link
Member

@defagos defagos left a comment

Choose a reason for hiding this comment

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

I tested the result in the simulator on iOS 18 and on an iOS 17 device. Two quick feedbacks without diving too much into the code yet:

  • Restoring the small vertical space between the player bar and the bottom bar would be nicer IMHO.
  • The TV program header seems broken, on iPad at least. Maybe a simulator thing, this needs to be tested on a device running the latest beta.

@defagos
Copy link
Member

defagos commented Aug 15, 2024

A suggestion also: You might want to assign the .search role to the corresponding tab item so that it is displayed in the standard position on the right.

@defagos
Copy link
Member

defagos commented Aug 16, 2024

A few additional inputs:

  • The settings button is also not visible anymore in the Profile section on iPadOS 18.
  • When enabling VoiceOver we were extending the player to fill the whole width. I guess this behavior does not make sense on iPadOS 18 anymore (and probably on earlier iPadOS versions too).
  • With the new tab bar appearance some titles are lost (e.g. we don't have the radio channel name anymore). Maybe there is nothing that can be done, though.
  • The TV program title starts in a broken state also on device (this is fixed by slightly scrolling vertically). Maybe this is still a beta issue, though.
  • The color gradients on topic pages has an incorrect offset after scrolling vertically. The back button also exhibits strange behavior.
  • I had to tweak the pre-build phase since it failed to locate the associated SRG User Data script. Maybe I missed something in my setup?
  • I experience crashes when toggling favorites. Maybe related to the tweak I made, maybe not?

Comment on lines 339 to 340
// Place the button where it would appear if a navigation bar was available.
if traitCollection.horizontalSizeClass == .regular {
Copy link
Member

@pyby pyby Aug 16, 2024

Choose a reason for hiding this comment

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

Should we keep the same previous offset for iPadOS previous 18 and introduce an os version check?
The new condition should be only for iPadOS 18 and more.

UI change: no more top and right "look like" same margin on the GoogleCast floating button above the heart stage card.

IMG_0659

Comment on lines 131 to 132
if (! _playerBottomToViewConstraint) {
_playerBottomToViewConstraint = [self.miniPlayerView.bottomAnchor constraintEqualToAnchor:self.view.bottomAnchor];
Copy link
Member

@pyby pyby Aug 16, 2024

Choose a reason for hiding this comment

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

Running on iPadOS less than 18.0, the mini player is now not visible, as displayed below the bottom tab bar.

The new condition should be only for iPadOS 18 and more.

IMG_0662

Comment on lines 422 to 426
if (self.traitCollection.horizontalSizeClass == UIUserInterfaceSizeClassRegular) {
self.playerBottomToSafeAreaConstraint.active = YES;
} else {
self.playerBottomToViewConstraint.active = YES;
}
Copy link
Member

Choose a reason for hiding this comment

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

Are the two contraints compatible in the same time as they are link both to self.miniPlayerView.bottomAnchor?
Do we expect to set active = NO to one, and active = YES to the other one?

@mutaben mutaben force-pushed the feature/PLAYRTS-5578-ios18-update branch from a5def0f to 4b33ceb Compare November 18, 2024 10:20
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYRTS-5578-ios18-update November 18, 2024 11:50 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYRTS-5578-ios18-update November 18, 2024 12:06 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYRTS-5578-ios18-update November 18, 2024 12:20 Inactive
@rts-devops rts-devops had a problem deploying to playsrg-ios-nightly+PLAYRTS-5578-ios18-update November 18, 2024 12:39 Error
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYRTS-5578-ios18-update November 18, 2024 13:55 Inactive
@mutaben mutaben added improvement Feature or update (issue and PR) - release notes section and removed improvement Feature or update (issue and PR) - release notes section bug Issue to be fix (issue and PR) - release notes section labels Nov 18, 2024
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+PLAYRTS-5578-ios18-update November 18, 2024 13:59 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+PLAYRTS-5578-ios18-update November 18, 2024 14:31 Inactive
Comment on lines +63 to +66
} else {
if let presentationController {
config.presentationContext = .viewController(presentationController)
}
Copy link
Member

Choose a reason for hiding this comment

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

Combining the else with the enclosed if is a bit more readable:

Suggested change
} else {
if let presentationController {
config.presentationContext = .viewController(presentationController)
}
}
else if let presentationController {
config.presentationContext = .viewController(presentationController)

Comment on lines +437 to +443
if (self.traitCollection.horizontalSizeClass == UIUserInterfaceSizeClassRegular) {
self.playerBottomToViewConstraint.active = NO;
self.playerBottomToSafeAreaConstraint.active = YES;
} else {
self.playerBottomToViewConstraint.active = YES;
self.playerBottomToSafeAreaConstraint.active = NO;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (self.traitCollection.horizontalSizeClass == UIUserInterfaceSizeClassRegular) {
self.playerBottomToViewConstraint.active = NO;
self.playerBottomToSafeAreaConstraint.active = YES;
} else {
self.playerBottomToViewConstraint.active = YES;
self.playerBottomToSafeAreaConstraint.active = NO;
}
BOOL isRegular = (self.traitCollection.horizontalSizeClass == UIUserInterfaceSizeClassRegular);
self.playerBottomToViewConstraint.active = ! isRegular;
self.playerBottomToSafeAreaConstraint.active = isRegular;

Copy link
Member

Choose a reason for hiding this comment

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

A few remarks:

  • On iOS 17 there is a small offset between the mini player and the tab bar. This makes the layout more readable.
  • Using a block to factor out part of the implementation requires awkward block signatures and dereferencing. It is better factored out as a method IMHO (which, BTW, should probably not be related to pre-iOS 18 behavior, as not all code paths are related to such behavior).

The original code is quite poor. If it can help we can try refactoring things a little bit together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Feature or update (issue and PR) - release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants