Skip to content

Rework iPad's menu resizing#1453

Merged
kambala-decapitator merged 4 commits intoxbmc:masterfrom
wutschel:fix_playlist_ipad
Apr 1, 2026
Merged

Rework iPad's menu resizing#1453
kambala-decapitator merged 4 commits intoxbmc:masterfrom
wutschel:fix_playlist_ipad

Conversation

@wutschel
Copy link
Copy Markdown
Collaborator

Description

This PR reworks the menu resizing on iPad. It also fixes an issue reported in this forum post and via AppStore review. Before this rework it was possible to hide the playlist view completely. This could happen because the movable playlist header could be placed behind the playlist toolbar, either by user or by rotation to landscape mode.

The rework takes care that the playlist header is always kept visible and touchable above the playlist toolbar, including rotation use cases.

Summary for release notes

Bugfix: Always keep playlist header visible on iPad

@kodiai
Copy link
Copy Markdown

kodiai Bot commented Mar 27, 2026

Kodiai Review Summary

What Changed

Reworked iPad menu resizing to prevent hiding the playlist view completely and ensure the playlist header remains visible above the toolbar during rotation.

Reviewed: core logic

Strengths

  • ✅ The bounds checking in touchesMoved:withEvent: (ViewControllerIPad.m:208) properly prevents the playlist header from being dragged beyond the available space
  • ✅ The snap-back logic in touchesEnded:withEvent: (ViewControllerIPad.m:223-226) correctly adjusts when the header would overlap the toolbar
  • ✅ Rotation handling (ViewControllerIPad.m:649) properly reapplies the layout constraints during device orientation changes

Observations

Impact

[MAJOR] XBMC Remote/ViewControllerIPad.m (533): Potential use of uninitialized variable
The viewWillAppear: method calls changeLeftMenu:maxVisibleMenuItems at ViewControllerIPad.m:533, but maxVisibleMenuItems may not be initialized if viewWillAppear: is called before viewDidLoad. While instance variables are zero-initialized in Objective-C, passing 0 to changeLeftMenu: could cause the menu to collapse to zero height on first appearance. This is particularly risky since the view controller lifecycle doesn't guarantee viewDidLoad runs before viewWillAppear: in all scenarios.

Preference

[MINOR] XBMC Remote/ViewControllerIPad.m (272, 280): Typo in comments
Optional: The comment text uses "seperator" instead of "separator" at lines 272 and 280. While this doesn't affect functionality, fixing the spelling would improve code readability.

Suggestions

  • Optional: Consider adding a guard in viewWillAppear: to only call changeLeftMenu: if maxVisibleMenuItems > 0 to handle edge cases where the method might be called before full initialization
  • Future consideration: The double-checking logic in both touchesMoved:withEvent: (line 208) and changeLeftMenu: (lines 273-275) could potentially be consolidated to reduce duplication

Verdict

🟡 Ready to merge with minor items -- Optional cleanup suggestions below (no blockers)

Review Details
  • Files reviewed: 2
  • Lines changed: +26 -10
  • Profile: strict (auto, lines changed: 36)
  • Author: newcomer (adapted tone)
  • Findings: 0 critical, 1 major, 0 medium, 1 minor (includes 2 from summary observations)
  • Review completed: 2026-03-27T19:02:12.162Z
  • Keyword parsing: No keywords detected

@wutschel wutschel force-pushed the fix_playlist_ipad branch 2 times, most recently from 0438503 to efc357e Compare March 28, 2026 10:15
Comment thread XBMC Remote/ViewControllerIPad.h Outdated
Comment thread XBMC Remote/ViewControllerIPad.m Outdated
Comment thread XBMC Remote/ViewControllerIPad.m Outdated
wutschel added 4 commits April 1, 2026 19:56
Excludes topPadding, bottomPadding, toolbar and playlist toolbar.
Avoid moving separator underneath the playlist toolbar, also after rotation or after startup. Snap back to last menu item which places the separator above the playlist toolbar. Simplify use of changeLeftMenu.
Rename property to mainMenuTable, which allows to remove some superfluous casts. In addition, remove synthesize and keep using a property only.
@wutschel
Copy link
Copy Markdown
Collaborator Author

wutschel commented Apr 1, 2026

Squashed and rebased to master.

@kambala-decapitator kambala-decapitator merged commit 243d30b into xbmc:master Apr 1, 2026
1 check passed
@wutschel wutschel deleted the fix_playlist_ipad branch April 2, 2026 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants