Fix SafeAreaEdges comments and access modifiers#34397
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34397Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34397" |
ba0c5f0 to
83ed49b
Compare
There was a problem hiding this comment.
Pull request overview
Updates XML documentation for SafeAreaEdges defaults across several Controls types to better reflect current safe-area behavior and to standardize wording.
Changes:
- Adjusts documented default safe-area behavior for
Layout,ContentView,ContentPage, andBorder. - Standardizes “edge-to-edge” wording in the
ScrollViewsafe area docs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/src/Core/ScrollView/ScrollView.cs | Doc wording tweak for the SafeAreaEdges default description. |
| src/Controls/src/Core/Layout/Layout.cs | Updates documented default SafeAreaEdges value for Layout. |
| src/Controls/src/Core/ContentView/ContentView.cs | Updates documented default SafeAreaEdges value for ContentView. |
| src/Controls/src/Core/ContentPage/ContentPage.cs | Updates documented default SafeAreaEdges value for ContentPage. |
| src/Controls/src/Core/Border/Border.cs | Updates documented default SafeAreaEdges value for Border. |
Comments suppressed due to low confidence (2)
src/Controls/src/Core/ContentView/ContentView.cs:42
- The remarks for
SafeAreaEdgesrefer toSafeAreaRegions.Keyboard, butSafeAreaRegionsdoes not defineKeyboard(it usesSoftInput). Please update the remarks to use the correct enum value name (and ensure the guidance matches the actual values supported bySafeAreaRegions).
/// The default value is SafeAreaEdges.None (edge-to-edge).
/// </summary>
/// <remarks>
/// This property controls which edges of the content view should obey safe area insets.
/// Use SafeAreaRegions.None for edge-to-edge content, SafeAreaRegions.All to obey all safe area insets,
src/Controls/src/Core/Border/Border.cs:70
- The remarks for
SafeAreaEdgesmentionSafeAreaRegions.Keyboard, butSafeAreaRegionsdoesn't have aKeyboardvalue (it usesSoftInput). Please update the remarks to use the correct enum value name so consumers aren't pointed at a non-existent option.
/// The default value is SafeAreaEdges.None (edge-to-edge).
/// </summary>
/// <remarks>
/// This property controls which edges of the border should obey safe area insets.
/// Use SafeAreaRegions.None for edge-to-edge content, SafeAreaRegions.All to obey all safe area insets,
You can also share your feedback on Copilot code review. Take the survey.
| /// <summary> | ||
| /// Gets or sets the safe area edges to obey for this scroll view. | ||
| /// The default value is SafeAreaEdges.Default (None - edge to edge). | ||
| /// The default value is SafeAreaEdges.Default (None - edge-to-edge). |
|
@jeremy-visionaid I see Copilot has some minor comments. Can you please double-check and let me know? :) |
|
@jfversluis This maybe opens up a can of worms... 😅 I'd probably accept Copilot's first suggestion - even though it was the thing that wrote it in the first place. ScrollView behavior is special, so it doesn't map cleanly to the abstracted values. It's not wrong about the internal reference, but it also implemented that incorrectly too. e.g. The comment is incorrect, and it's internal, when it should probably be public. It is the value that's actually used as a default (you can only create an equivalent struct and not use that member directly). It is referenced from the MAUI docs (which also incorrectly refer to it as an enum): For the scope of this PR, I was only intending to address the thing that caught me and another team member out - the behavior of the controls wasn't what we expected since IntelliSense told us the wrong defaults. There's prob more AI slop that needs fixing after this - there was a bunch of stuff that looked off to me but I didn't know where I should stop 🤷 I think it's reasonable to fix up the SafeAreaEdges.Container bit here too, but it does affect the public API... |
|
@kubaflo I've changed it to satisfy Copilot, and added a few other bits that it missed. Unfortunately, the comments also reference members that appear to be unintentionally internal, so it also needs an update to the public API... |
|
@PureWeen what do you think? |
PureWeen
left a comment
There was a problem hiding this comment.
retarget to net11.0
Otherwise looks good
|
|
|
@jfversluis we can merge this to net11 |
Default SafeAreaEdges values are set by a delegate, so SafeAreaEdges.Default is not used in most cases. Also make SafeAreaEdges.Container public as it is referenced in the API documentation.
c202daf to
7f5addc
Compare
|
🚨 API change(s) detected @davidbritch FYI |
|
🚨 API change(s) detected @davidortinau FYI |
Layout.SafeAreaEdges documents the default value to be "None", but the default value delegate actually returns "Container".