[Mac] Fix for DeviceDisplay.MainDisplayInfoChanged event not getting raised#31786
Conversation
|
Hey there @@HarishwaranVijayakumar! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
Fixes missing DeviceDisplay.MainDisplayInfoChanged event firing on Mac Catalyst by switching from iOS status bar orientation notifications to macOS screen parameter notifications and by sourcing display metrics directly from Core Graphics. Key changes include added Core Graphics P/Invokes for Mac Catalyst, revised event subscription logic, and modification of DisplayInfo equality semantics to include RefreshRate.
- Added Core Graphics interop and Mac Catalyst-specific display info retrieval
- Replaced iOS-only orientation notification with NSApplicationDidChangeScreenParametersNotification on Mac Catalyst
- Modified DisplayInfo.Equals to include RefreshRate
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Essentials/src/Types/DisplayInfo.shared.cs | Added RefreshRate to equality comparison |
| src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs | Introduced Mac Catalyst Core Graphics path and new screen change notification handling |
|
/rebase |
69479fc to
51dd6b3
Compare
| // Core Graphics P/Invoke declarations for Mac Catalyst | ||
| // Returns the display ID of the main display | ||
| [DllImport(Constants.CoreGraphicsLibrary)] | ||
| static extern uint CGMainDisplayID(); |
There was a problem hiding this comment.
CGMainDisplayID() only returns the main display, ignoring external monitors. I think that if user drags app window to external monitor and changes that monitor's refresh rate/resolution, the event fires but reports the main display's properties, not the active display's.
| var refreshRate = CGDisplayModeGetRefreshRate(mode); | ||
|
|
||
| // Release the display mode to avoid memory leaks | ||
| CGDisplayModeRelease(mode); |
There was a problem hiding this comment.
While unlikely, if exception thrown here, mode never released
51dd6b3 to
80177ea
Compare
🤖 AI Summary📊 Expand Full Review🔍 Pre-Flight — Context & Validation📝 Review Session — Changed code to use const string ·
|
| File | Type | Changes |
|---|---|---|
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs |
Implementation | +96/-0 |
src/Essentials/src/Types/DisplayInfo.shared.cs |
Shared type | +2/-1 |
No test files added.
PR's Approach
- Mac Catalyst display info: Added P/Invoke for Core Graphics APIs (
CGMainDisplayID,CGDisplayCopyDisplayMode,CGDisplayModeGetWidth/Height/RefreshRate,CGDisplayRotation) to get fresh, non-cached display properties - Notification change: Replaced
DidChangeStatusBarOrientationNotification(iOS-only) withNSApplicationDidChangeScreenParametersNotificationon Mac Catalyst - Equality change: Added
RefreshRatetoDisplayInfo.Equals()so display change detection includes refresh rate
Reviewer Feedback
| File:Line | Reviewer | Comment | Status |
|---|---|---|---|
DeviceDisplay.ios.cs:22 |
jsuarezruiz (CHANGES_REQUESTED) | CGMainDisplayID() only returns main display. If app is on external monitor and user changes that monitor, event fires but reports main display properties ( UNRESOLVED |
wrong). |
DeviceDisplay.ios.cs:85 |
jsuarezruiz (CHANGES_REQUESTED) | DisplayOrientation is hardcoded to ` PARTIALLY ADDRESSED - author says matches Xamarin pattern |
Portrait` |
DeviceDisplay.ios.cs:72 |
jsuarezruiz (CHANGES_REQUESTED) | If exception thrown, mode (CGDisplayMode) never UNRESOLVED |
released |
DisplayInfo.shared.cs:117 |
Copilot | Adding RefreshRate to Equals() is a breaking behavioral UNRESOLVED |
change |
DeviceDisplay.ios.cs:87 |
Copilot | Orientation hardcoded to Portrait | Same as jsuarezruiz issue |
DeviceDisplay.ios.cs:130 |
Copilot | new NSString(...) allocates each time; use static readonly |
Minor |
Additional Code Issues Found
- GetHashCode/Equals inconsistency:
RefreshRateadded toEquals()but NOT toGetHashCode(). This violates the contract that equal objects must have equal hash codes. - Documentation not updated:
<remarks>onEquals()still only lists "Height, Width, Density, Orientation and Rotation" - omits RefreshRate. - Resource leak:
CGDisplayCopyDisplayModeresult not protected with try/finally - if exception occurs between acquisition and release, mode leaks.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #31786 | Core Graphics P/Invoke + NSApplicationDidChangeScreenParametersNotification + RefreshRate in PENDING (Gate) | 2 files (+96) | Original PR | Equals |
🚦 Gate — Test Verification
📝 Review Session — Changed code to use const string · 80177ea
** FAILED - No Tests AddedResult:**
Platform: ios (Mac Catalyst specified, iOS selected as closest available)
Mode: Test Existence Check
Gate Outcome
This PR does NOT include any issue-specific tests for issues #22634 or #22642.
- No test files matching
*22634*or*22642*found anywhere in the test directories - No new Essentials device tests added for Mac Catalyst display change behavior
- The notification subscription logic (
NSApplicationDidChangeScreenParametersNotification) cannot be verified without a Mac Catalyst-specific test
What Exists
Existing tests that were NOT added by this PR:
src/Essentials/test/UnitTests/DeviceDisplay_Tests.cs- TestsEquals()method (doesn't varyRefreshRate, uses default0)src/Essentials/test/DeviceTests/Tests/DeviceDisplay_Tests.cs- Basic screen metrics check (not relevant to the notification fix)
Impact
The core fix (using NSApplicationDidChangeScreenParametersNotification on Mac Catalyst) cannot be automatically verified because:
- No unit test was added for the new notification subscription logic
- No Mac Catalyst device test was added to verify the event fires on display changes
- The fix requires an actual Mac Catalyst environment with a real display change to verify
Gate FAILEDStatus:
Tests are missing. The PR author should add tests to verify:
- On Mac Catalyst,
StartScreenMetricsListenerssubscribes toNSApplicationDidChangeScreenParametersNotification GetMainDisplayInfo()uses Core Graphics on Mac Catalyst (not UIScreen)DisplayInfo.Equals()now considersRefreshRate(unit test with varying RefreshRate)GetHashCode()also needs to includeRefreshRate(consistency bug found)
🔧 Fix — Analysis & Comparison
📝 Review Session — Changed code to use const string · 80177ea
Fix Phase: SKIPPED
Reason: Gate phase failed (no issue-specific tests added by PR). Per workflow rules, Phase 3 (try-fix) is only executed after Gate passes.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #31786 | Core Graphics P/Invoke + NOT VERIFIED | 2 files | Gate failed - no tests | NSApplicationDidChangeScreenParametersNotification |
Exhausted: N/A (skipped)
Selected Fix: N/A - Gate failed; see Report for recommendation
📋 Report — Final Recommendation
📝 Review Session — Changed code to use const string · 80177ea
Final Recommendation: REQUEST CHANGES##
Summary
PR #31786 fixes the DeviceDisplay.MainDisplayInfoChanged event not firing on Mac Catalyst by switching from iOS-only status bar notifications to NSApplicationDidChangeScreenParametersNotification, and uses Core Graphics P/Invokes to get fresh (non-cached) display information. The approach is architecturally sound, but has several unresolved issues flagged by the reviewer (jsuarezruiz already has CHANGES_REQUESTED), plus no tests were added for the new behavior.
Root Cause (Well-Documented)
DidChangeStatusBarOrientationNotificationdoesn't fire on Mac Catalyst (no status bar on desktop)UIScreenreturns cached display values on Mac Catalyst- Both issues prevent
MainDisplayInfoChangedfrom firing andMainDisplayInfofrom returning current values
Gate FAILEDResult:
No issue-specific tests were added to the PR. The core notification behavior change cannot be automatically verified.
Code Issues Requiring Changes
1. GetHashCode() and Equals() are inconsistent after RefreshRate addition
- File:
src/Essentials/src/Types/DisplayInfo.shared.cs:124 - Problem:
RefreshRatewas added toEquals()but NOT toGetHashCode(). This violates the fundamental contract that objects considered equal must produce the same hash code. This would cause silent failures whenDisplayInfois used as a dictionary key or in hash sets. - Fix: Add
RefreshRatetoGetHashCode():public override int GetHashCode() => (Height, Width, Density, Orientation, Rotation, RefreshRate).GetHashCode();
2. Resource leak in GetMainDisplayInfo() on exception
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs:60-72 - Problem: If an exception is thrown between
CGDisplayCopyDisplayModeandCGDisplayModeRelease, the native mode object leaks. This is flagged by jsuarezruiz. - Fix: Wrap in try/finally:
var mode = CGDisplayCopyDisplayMode(displayId); if (mode == IntPtr.Zero) return GetFallbackDisplayInfo(); try { var width = (double)CGDisplayModeGetWidth(mode); ... } finally { CGDisplayModeRelease(mode); }
3. No tests added
- Problem: There are no unit or device tests for the new Mac Catalyst-specific behavior. The
DisplayInfo.Equals()change should have a test showing that two instances with differentRefreshRateare no longer equal. The notification subscription change should be tested.
4. CGMainDisplayID() reports main display only
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs:22 - When the app runs on an external monitor and that monitor changes settings,
NSApplicationDidChangeScreenParametersNotificationfires butCGMainDisplayID()still returns the primary display. The user would receive an event with incorrect display info. - Recommended approach: Consider using
NSScreen.MainScreenon Mac Catalyst to get the actual screen the app is using, or document this limitation.
5. DisplayOrientation hardcoded to Portrait
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs:85 - The author notes this matches Xamarin behavior. While acceptable as a known limitation, it should be documented with a
// TODOcomment explaining the rationale.
6. <remarks> XML doc not updated
- File:
src/Essentials/src/Types/DisplayInfo.shared.cs:110 - The
Equals()remarks still say "Height, Width, Density, Orientation andRefreshRateis missing.Rotation"
7. new NSString(...) repeated allocation
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs:130 - Minor: use a
static readonly NSStringfield to avoid repeated allocations.
PR Title/Description Assessment
Title: [Mac] Fix for DeviceDisplay.MainDisplayInfoChanged event not getting raised
- Acceptable. Could be improved to
[Mac Catalyst] DeviceDisplay: Fix MainDisplayInfoChanged not firing on display changesbut current title is clear.
Description: Good quality. Has NOTE block, issue links, platform test checklist, before/after videos. Well-structured.
Missing from description: The GetHashCode/Equals inconsistency and the CGMainDisplayID limitation are not called out.
What Needs to Be Done
Fix GetHashCode() to include RefreshRate (critical correctness bug)1.
Fix resource leak (wrap mode in try/finally)2.
Add tests for: (a) RefreshRate in Equals, (b) Mac Catalyst notification subscription3.
Resolve jsuarezruiz's CHANGES_REQUESTED review (currently unresolved)4.
Address CGMainDisplayID limitation (or document it explicitly)5.
Update <remarks> XML doc on Equals()6.
Add // TODO: orientation calculation comment explaining Portrait hardcode7.
📋 Expand PR Finalization Review
Title: ✅ Good
Current: [Mac] Fix for DeviceDisplay.MainDisplayInfoChanged event not getting raised
Description: ✅ Good
Description needs updates. See details below.
✨ Suggested PR Description
[!NOTE]
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
When changing the resolution, rotation, or refresh rate on Mac Catalyst, the DeviceDisplay.MainDisplayInfoChanged event is not triggered. Additionally, DeviceDisplay.MainDisplayInfo returns stale (cached) values after display changes.
Root Cause
Mac Catalyst apps do not have a mobile-style status bar, so DidChangeStatusBarOrientationNotification never fires on the desktop. Additionally, UIScreen on Mac Catalyst can return cached display values — meaning even if a notification were triggered, the display properties reported would be stale.
Description of Change
Mac Catalyst display info — Core Graphics P/Invokes (DeviceDisplay.ios.cs):
- Added P/Invoke declarations for Core Graphics APIs (
CGMainDisplayID,CGDisplayCopyDisplayMode,CGDisplayModeGetWidth/Height,CGDisplayModeGetRefreshRate,CGDisplayRotation,CGDisplayModeRelease) to fetch fresh, non-cached display properties directly from the system. - Updated
GetMainDisplayInfo()to use Core Graphics APIs on Mac Catalyst, bypassing UIScreen entirely. Includes orientation/rotation calculation viaConvertRotationDegreesToDisplayRotation(). DisplayOrientationis hardcoded toPortraiton Mac Catalyst — consistent with Xamarin's behavior; Mac desktop apps do not have a meaningful portrait/landscape orientation concept.
Display change event handling (DeviceDisplay.ios.cs):
- On Mac Catalyst,
StartScreenMetricsListeners()now subscribes toNSApplicationDidChangeScreenParametersNotificationinstead ofUIApplication.DidChangeStatusBarOrientationNotification. The new notification covers resolution, refresh rate, color profile, and other display parameter changes. - iOS path is unchanged.
Equality logic (DisplayInfo.shared.cs):
- Updated
DisplayInfo.Equals()to includeRefreshRatein equality checks, ensuring a display refresh rate change triggersMainDisplayInfoChangedcorrectly. GetHashCode()updated to includeRefreshRatefor consistency withEquals().
Known Limitations
CGMainDisplayID()returns the OS-designated primary display (the one with the menu bar). In multi-monitor setups, if the app's window is on a secondary monitor and that monitor's settings change, the event will fire butMainDisplayInfowill report the primary display's properties.
Issues Fixed
Platforms Tested
- Mac Catalyst (primary)
- iOS (no behavior change — verified existing behavior preserved)
- Windows (no change)
- Android (no change)
Code Review: ⚠️ Issues Found
Code Review Findings — PR #31786
🔴 Critical Issues
1. GetHashCode() / Equals() contract violated — RefreshRate missing from GetHashCode
- File:
src/Essentials/src/Types/DisplayInfo.shared.cs(line ~124) - Problem:
RefreshRatewas added toEquals()butGetHashCode()was not updated. The .NET contract requires that objects considered equal (Equals()returnstrue) produce the same hash code. With this PR, twoDisplayInfostructs that differ only inRefreshRatewill be considered unequal byEquals()but may produce the same hash code — breakingDictionary<DisplayInfo, T>,HashSet<DisplayInfo>, and any change-detection logic relying on hash equality. - Current code:
public override int GetHashCode() => (Height, Width, Density, Orientation, Rotation).GetHashCode(); // ← RefreshRate missing
- Fix:
public override int GetHashCode() => (Height, Width, Density, Orientation, Rotation, RefreshRate).GetHashCode();
2. Resource leak — CGDisplayMode native handle not protected with try/finally
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs(lines ~60–72,GetMainDisplayInfo()) - Problem: If any exception is thrown between
CGDisplayCopyDisplayMode()andCGDisplayModeRelease(), the nativeCGDisplayModeobject is never freed, causing a native memory leak. Also flagged by reviewerjsuarezruiz. - Current code:
var mode = CGDisplayCopyDisplayMode(displayId); if (mode == IntPtr.Zero) return GetFallbackDisplayInfo(); var width = (double)CGDisplayModeGetWidth(mode); var height = (double)CGDisplayModeGetHeight(mode); var refreshRate = CGDisplayModeGetRefreshRate(mode); // Release the display mode to avoid memory leaks CGDisplayModeRelease(mode);
- Fix:
var mode = CGDisplayCopyDisplayMode(displayId); if (mode == IntPtr.Zero) return GetFallbackDisplayInfo(); try { var width = (double)CGDisplayModeGetWidth(mode); var height = (double)CGDisplayModeGetHeight(mode); var refreshRate = CGDisplayModeGetRefreshRate(mode); // ... build and return DisplayInfo ... } finally { CGDisplayModeRelease(mode); }
🟡 Suggestions
3. No tests added for new Mac Catalyst-specific behavior
- Problem: The PR introduces a new notification subscription path on Mac Catalyst and modifies
DisplayInfo.Equals()— two behavioral changes — but adds zero tests. At minimum:- A unit test verifying
DisplayInfo.Equals()returnsfalsewhenRefreshRatediffers (andtruewhen all fields includingRefreshRateare equal) - A unit test verifying
GetHashCode()consistency withEquals()once it is fixed - Existing test file
src/Essentials/test/UnitTests/DeviceDisplay_Tests.csalready hasEqualstests and should be extended
- A unit test verifying
- Impact: Without tests, the
GetHashCoderegression (item [Draft] Readme WIP #1 above) could go undetected in the future.
4. CGMainDisplayID() only returns the primary system display (multi-monitor limitation)
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs(line ~22,GetMainDisplayInfo()) - Problem:
NSApplicationDidChangeScreenParametersNotificationfires for any display change on the system. ButCGMainDisplayID()always returns the OS-designated main/primary display. If the user's app window is on an external monitor and the user changes that monitor's resolution or refresh rate, the event fires butGetMainDisplayInfo()returns the primary display's info — which is incorrect and could confuse consumers. - Reviewer: Already flagged by
jsuarezruiz(unresolved). - Recommendation: Either use
NSScreen.MainScreen(which reflects the screen with the active menu bar on Mac) or document this limitation with a code comment:// Note: CGMainDisplayID returns the primary display (with menu bar). // In multi-monitor setups, this may not be the display the app window is on. var displayId = CGMainDisplayID();
5. DisplayOrientation hardcoded to Portrait — missing explanation comment
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs(line ~85) - Problem:
DisplayOrientation.Portraitis hardcoded regardless of actual display configuration. The author confirmed in review discussion this matches Xamarin's behavior and fixing it broke existing tests. This is acceptable as a known limitation, but the code should carry a comment explaining the rationale. - Recommendation:
// Orientation is intentionally hardcoded to Portrait to match Xamarin's Mac Catalyst // behavior. Deriving orientation from dimensions/rotation breaks existing tests and // Mac desktop apps don't have a meaningful orientation concept. orientation: DisplayOrientation.Portrait,
6. XML doc <remarks> on Equals() not updated
- File:
src/Essentials/src/Types/DisplayInfo.shared.cs(line ~110) - Problem: The
<remarks>attribute onEquals()still reads "Equality is established by comparing if Height, Width, Density, Orientation and Rotation are all equal." —RefreshRateis now also compared but not mentioned. - Fix: Update remarks to include
RefreshRate:The/// <remarks>Equality is established by comparing if <see cref="Height"/>, <see cref="Width"/>, /// <see cref="Density"/>, <see cref="Orientation"/>, <see cref="Rotation"/> and /// <see cref="RefreshRate"/> are all equal.</remarks>
GetHashCode()remarks also need updating onceRefreshRateis added to the hash.
7. new NSString(...) allocated on every StartScreenMetricsListeners() call (minor)
- File:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs(line ~130) - Problem:
new NSString(NSApplicationDidChangeScreenParametersNotification)creates a new managed+native string wrapper every time listeners start.StartScreenMetricsListeners()can be called multiple times during app lifetime. - Fix: Define a static field:
And use
#if MACCATALYST static readonly NSString ScreenParametersChangedNotification = new NSString(NSApplicationDidChangeScreenParametersNotification); #endif
ScreenParametersChangedNotificationinStartScreenMetricsListeners().
✅ Looks Good
- Correct use of
#if MACCATALYST/#elseguards — iOS path is fully preserved and unchanged - Notification observer cleanup in
StopScreenMetricsListenersviaobserver?.Dispose()handles the new observer correctly (same pattern as before) - Fallback to
GetFallbackDisplayInfo()whenmode == IntPtr.Zerois a solid defensive pattern - P/Invoke declarations use
DllImport(Constants.CoreGraphicsLibrary)— correct convention for this codebase ConvertRotationDegreesToDisplayRotationhelper method is clean, well-named, and uses a readable switch expressionNSApplicationDidChangeScreenParametersNotificationis the correct macOS notification for screen parameter changes (resolution, refresh rate, color profile, etc.)CGDisplayModeGetRefreshRatefallback of 0.0 for displays without a fixed refresh rate is handled implicitly by the float cast
|
Addressed concerns raised in the AI summary. |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 31786Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 31786" |
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/review -b feature/refactor-copilot-yml |
kubaflo
left a comment
There was a problem hiding this comment.
Can you resolve conflicts?
2c058f8 to
a5edd2f
Compare
I have resolved the conflicts. |
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
|
|
||
| try | ||
| { | ||
| var width = (double)CGDisplayModeGetWidth(mode); |
There was a problem hiding this comment.
[major] Logic and Correctness - CGDisplayModeGetWidth/Height return the display mode logical size on HiDPI Mac displays, but DisplayInfo.Width/Height are documented and previously returned as physical pixels (UIScreen.Bounds * Scale). On Retina/HiDPI modes this reports values that are too small while still reporting a Retina density. Use CGDisplayModeGetPixelWidth/PixelHeight for the pixel dimensions.
There was a problem hiding this comment.
Not a valid concern — we took the reference from Xamarin.Essentials' macOS implementation, which also returns logical units paired with BackingScaleFactor as density (not physical pixel dimensions): xamarin
| // Mac desktop apps don't have a meaningful orientation concept. | ||
| orientation: DisplayOrientation.Portrait, | ||
| rotation: rotation, | ||
| rate: (float)refreshRate); |
There was a problem hiding this comment.
[major] Logic and Correctness - This PR reports the new Mac Catalyst refresh rate, but MainDisplayInfoChanged is still suppressed when only the refresh rate changes because DeviceDisplayImplementationBase.OnMainDisplayInfoChanged gates events through DisplayInfo.Equals, and DisplayInfo.Equals intentionally ignores RefreshRate. A user changing only the display refresh rate will still not receive the event this PR is meant to fix; compare refresh rate in the event-change gate without changing public DisplayInfo.Equals semantics.
There was a problem hiding this comment.
In one of the previous reviews it was suggested to remove the from as this might be a breaking change, so removed that.
This comment has been minimized.
This comment has been minimized.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
I have reviewed the AI suggestion. |
|
/review -b feature/enhanced-reviewer |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| // Mac desktop apps don't have a meaningful orientation concept. | ||
| orientation: DisplayOrientation.Portrait, | ||
| rotation: rotation, | ||
| rate: (float)refreshRate); |
There was a problem hiding this comment.
[major] Logic and Correctness - This now reports the CoreGraphics refresh rate on Mac Catalyst, but DeviceDisplayImplementationBase.OnMainDisplayInfoChanged() only raises MainDisplayInfoChanged when DisplayInfo.Equals() changes, and DisplayInfo.Equals() ignores RefreshRate. A screen-parameter notification caused only by a refresh-rate change will still be suppressed, leaving subscribers unaware even though MainDisplayInfo.RefreshRate changed. Include refresh rate in the change detection path or explicitly raise for this Mac Catalyst notification scenario.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@HarishwaranVijayakumar — new AI review results are available based on this last commit:
2acd1bf.
update fix To request a deterministic rerun after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
UI Tests — Essentials
Detected UI test categories: Essentials
⏭️ Deep UI tests — 0 passed, 0 failed across 1 category on platform-pool agent (replaces in-process counts above). 1 category reported 0 tests.
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Essentials |
0 tests | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
Pre-Flight — Context & Validation
Issue: #22634 / #22642 - DeviceDisplay.MainDisplayInfoChanged and MainDisplayInfo stale on Mac Catalyst
PR: #31786 - [Mac] Fix for DeviceDisplay.MainDisplayInfoChanged event not getting raised
Platforms Affected: Mac Catalyst
Files Changed: 1 implementation, 0 test
Key Findings
- PR replaces the iOS status-bar orientation notification with
NSApplicationDidChangeScreenParametersNotificationunder#if MACCATALYST, which addresses Mac Catalyst not receiving mobile status-bar notifications for display setting changes. - PR bypasses
UIScreenfor Mac Catalyst display metrics by using CoreGraphics display mode APIs and releases the native display mode in afinallyblock. - Gate was already skipped/failed by the caller because the PR adds no tests; no gate verification was rerun.
- Existing review feedback notes the current implementation reports the main display via
CGMainDisplayID()and keeps Mac Catalyst orientation asPortraitfor Xamarin compatibility.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: high
Errors: 1 | Warnings: 1 | Suggestions: 0
Key code review findings:
- ERROR:
src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs:116returns a CoreGraphics refresh rate, butDeviceDisplay.shared.cs:139gates events throughDisplayInfo.Equals(), andDisplayInfo.shared.cs:111-116ignoresRefreshRate; refresh-rate-only changes can still be suppressed. - WARNING: CI is not green in GitHub checks; at least part appears infrastructural, but merge readiness remains blocked until failures are dispositioned.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #31786 | Mac Catalyst CoreGraphics display info plus NSApplicationDidChangeScreenParametersNotification listener |
SKIPPED (Gate: no tests detected) | src/Essentials/src/DeviceDisplay/DeviceDisplay.ios.cs |
Original PR; still misses refresh-rate-only event gate |
Code Review — Deep Analysis
Code Review — PR #31786
Independent Assessment
What this changes: Mac Catalyst DeviceDisplay now listens for NSApplicationDidChangeScreenParametersNotification instead of iOS status-bar orientation changes, and GetMainDisplayInfo() bypasses UIScreen by querying CoreGraphics display mode, rotation, refresh rate, and scale.
Inferred motivation: Mac Catalyst does not receive mobile status-bar orientation notifications, and UIScreen can return stale data after desktop display setting changes.
Reconciliation with PR Narrative
Author claims: The PR fixes DeviceDisplay.MainDisplayInfoChanged not firing for Mac Catalyst resolution, rotation, and refresh-rate changes, and returns fresh display information through CoreGraphics.
Agreement/disagreement: The notification switch matches the root cause for resolution/rotation changes. However, a refresh-rate-only change is still suppressed by the existing DeviceDisplayImplementationBase change gate because DisplayInfo.Equals() ignores RefreshRate (DeviceDisplay.shared.cs:139, DisplayInfo.shared.cs:111-116). This was already raised in an existing inline thread, so inline-findings.json is intentionally empty to avoid duplicate GitHub comments.
Findings
❌ Error — Refresh-rate-only changes still do not raise MainDisplayInfoChanged
DeviceDisplay.ios.cs:116 returns the new CoreGraphics refresh rate, but OnMainDisplayInfoChanged() only invokes subscribers when _currentMetrics.Equals(e.DisplayInfo) is false. DisplayInfo.Equals() currently compares width, height, density, orientation, and rotation only, not RefreshRate. If the user changes only the Mac display refresh rate, the notification fires and the new rate is read, but the event is suppressed, leaving one of the PR's stated scenarios unfixed. This should be addressed with a refresh-rate-aware event-change comparison without changing public DisplayInfo.Equals() semantics.
⚠️ Warning — Current CI is not green
The latest GitHub check runs for head 2acd1bf220d3522dab905c4e3b7f2987d654a128 show maui-pr and Build Analysis failing. Build Analysis reports a macOS Debug Provision JDK failure that looks infrastructural, plus a Windows Helix Unit Tests Debug failure. I did not find evidence these are caused by this one-file Mac Catalyst change, but this PR should not be treated as merge-ready until required CI is green or failures are dispositioned.
Devil's Advocate
The resource-release issue previously raised appears fixed by the current try/finally. The hardcoded Mac Catalyst portrait orientation is a product/API compatibility decision rather than a clear correctness error, especially given the author's Xamarin compatibility rationale. I did not repeat existing inline comments to avoid duplicate feedback.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: The main notification approach is reasonable, but the refresh-rate scenario remains broken because MAUI's existing display-info equality gate ignores RefreshRate. CI is also currently failing, though at least part of it appears unrelated/infrastructure.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | maui-expert-reviewer | Shared internal event-change comparer includes RefreshRate while public DisplayInfo.Equals() remains unchanged |
PASS: focused Essentials DeviceDisplay unit tests | 1 file (DeviceDisplay.shared.cs) |
Fixes refresh-rate-only suppression, but changes event behavior for all platforms |
| 2 | maui-expert-reviewer | Mac Catalyst-only refresh-rate-aware notification path via opt-in shared overload called only from the Catalyst notification callback | PASS: focused Essentials DeviceDisplay unit tests; PASS: net10.0-maccatalyst Essentials build with -p:MauiPlatforms=net10.0-maccatalyst |
2 files (DeviceDisplay.shared.cs, DeviceDisplay.ios.cs) |
Selected: fixes PR gap without changing public equality or default cross-platform event behavior |
| PR | PR #31786 | CoreGraphics display info plus NSApplicationDidChangeScreenParametersNotification |
SKIPPED (Gate: no tests detected) | 1 file (DeviceDisplay.ios.cs) |
Original PR; current code-review found refresh-rate-only changes can still be suppressed |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Add a refresh-rate-aware internal event comparer while preserving public DisplayInfo.Equals() semantics |
| maui-expert-reviewer | 2 | Yes | Narrow the comparer to Mac Catalyst by using an opt-in overload only from NSApplicationDidChangeScreenParametersNotification |
Exhausted: No - stopped because Candidate #2 passed the relevant focused tests and Mac Catalyst compile check and is demonstrably better than the PR's current fix.
Selected Fix: Candidate #2 - it addresses the unresolved refresh-rate-only event suppression while keeping the behavior scoped to Mac Catalyst and preserving public DisplayInfo.Equals() semantics.
Candidate Details
try-fix-1
See ../try-fix-1/content.md.
try-fix-2
See ../try-fix-2/content.md.
Report — Final Recommendation
Comparative Report - PR #31786
Candidate ranking
| Rank | Candidate | Regression status | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
PASS by equivalence to try-fix-2 validation |
Best candidate. Applies the expert reviewer feedback with a Mac Catalyst-only refresh-rate-aware event path, preserving public DisplayInfo.Equals() and default behavior on other platforms. |
| 2 | try-fix-2 |
PASS: focused DeviceDisplay unit tests; PASS: net10.0-maccatalyst Essentials build with -p:MauiPlatforms=net10.0-maccatalyst |
Technically equivalent to pr-plus-reviewer and also correct. Ranked just below because the PR-based candidate can be represented as reviewer feedback applied to the submitted PR. |
| 3 | try-fix-1 |
PASS: focused DeviceDisplay unit tests | Fixes refresh-rate-only suppression, but broadens refresh-rate event behavior for all platforms rather than only the Catalyst notification path that needs it. |
| 4 | pr |
SKIPPED: no tests detected in PR | Improves Catalyst display metric retrieval and notification subscription, but still suppresses refresh-rate-only changes because the shared event gate uses DisplayInfo.Equals(), which ignores RefreshRate. |
Analysis
The raw PR correctly replaces iOS status-bar-orientation notifications with NSApplicationDidChangeScreenParametersNotification for Mac Catalyst and uses CoreGraphics APIs for fresher display mode data. However, the event pipeline still gates MainDisplayInfoChanged through DisplayInfo.Equals(), so a refresh-rate-only screen-parameter notification can be dropped even though the PR now reports RefreshRate.
try-fix-1 addresses that by adding refresh-rate comparison to the shared event gate. That passes the focused regression command, but it changes event behavior globally. try-fix-2 narrows the change by adding an opt-in shared overload and using it only from the Catalyst notification callback; this preserves public equality semantics and avoids changing other platforms. The expert reviewer's actionable feedback maps directly to this scoped approach, so pr-plus-reviewer is the same fix applied as review feedback to the PR.
Winner
pr-plus-reviewer wins. It fixes the identified correctness gap, keeps the change scoped to Mac Catalyst, and is backed by the same passing focused unit and Catalyst build evidence as try-fix-2.
Future Action — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
|
/azp run maui-pr-devicetests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…raised (#31786) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details - When changing the resolution, rotation, or refresh rate, the DeviceDisplay.MainDisplayInfoChanged event is not triggered. ### Root Cause of the issue - DidChangeStatusBarOrientationNotification does not apply to Mac Catalyst apps because they do not have a mobile-style status bar. On desktop, changes such as resolution, refresh rate are not reported through mobile-oriented notifications. ### Description of Change **Mac Catalyst-specific display handling:** * Added P/Invoke declarations for Core Graphics APIs in `DeviceDisplay.ios.cs` to fetch display properties (resolution, refresh rate, rotation) directly from the system, bypassing potentially stale UIKit data. * Updated `GetMainDisplayInfo()` to use Core Graphics APIs on Mac Catalyst, including logic to calculate orientation and rotation based on system-reported values. **Display change event handling:** * On Mac Catalyst, replaced status bar orientation notifications with `NSApplicationDidChangeScreenParametersNotification` to reliably detect display changes (resolution, refresh rate, etc.). **Equality logic improvements:** * Updated the `DisplayInfo.Equals()` method to include `RefreshRate` in equality checks, ensuring all display properties are considered. ### Issues Fixed <!-- Please make sure that there is a bug logged for the issue being fixed. The bug should describe the problem and how to reproduce it. --> Fixes #22634 Fixes #22642 ### Tested the behaviour in the following platforms - [x] - Windows - [x] - Android - [x] - iOS - [x] - Mac ### Output | Before | After | |----------|----------| | <video src="https://github.com/user-attachments/assets/44c1c29a-b6d9-4231-b732-54d874d5ac7d"> | <video src="https://github.com/user-attachments/assets/548ce40f-ee6f-4b7d-ac85-fa445e74f106"> | <!-- Are you targeting main? All PRs should target the main branch unless otherwise noted. -->
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
Root Cause of the issue
Description of Change
Mac Catalyst-specific display handling:
DeviceDisplay.ios.csto fetch display properties (resolution, refresh rate, rotation) directly from the system, bypassing potentially stale UIKit data.GetMainDisplayInfo()to use Core Graphics APIs on Mac Catalyst, including logic to calculate orientation and rotation based on system-reported values.Display change event handling:
NSApplicationDidChangeScreenParametersNotificationto reliably detect display changes (resolution, refresh rate, etc.).Equality logic improvements:
DisplayInfo.Equals()method to includeRefreshRatein equality checks, ensuring all display properties are considered.Issues Fixed
Fixes #22634
Fixes #22642
Tested the behaviour in the following platforms
Output
Resolution_before.mp4
Resolution.mp4