Custom popup placement callback

[maxkatz6]

What does the pull request do?

Implements Placement.Custom for Popup and Popup-based controls - Flyout, Tooltip, ContextMenu.

Differences from WPF implementation:

1. It's not possible to support multiple custom placement options like in WPF due to Wayland. But our positioning is generally flexible with different PopupAnchor/PopupGravity/PopupPositionerConstraintAdjustment combinations, that allows implicit adjustments.
2. In WPF, CustomPopupPlacement has only Point position and primary axis params. In Avalonia it's more consistent to the current API.
3. In WPF, CustomPopupPlacementCallback provides Size targetSize argument. I don't see any limitations to provide full rect here though.

New APIs:

+public delegate CustomPopupPlacement CustomPopupPlacementCallback(Size popupSize, Rect targetRect, Point offset);
+public record struct CustomPopupPlacement
+{
+ public PopupAnchor Anchor { get; init; }
+ public PopupGravity Gravity  { get; init; }
+ public PopupPositionerConstraintAdjustment ConstraintAdjustment { get; init; }
+ public Point Offset { get; init; }
+}

public class Popup
{
+ public static readonly StyledProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public CustomPopupPlacementCallback? CustomPopupPlacementCallback { get; set; }
}

public class PopupFlyoutBase
{
+ public static readonly StyledProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public CustomPopupPlacementCallback? CustomPopupPlacementCallback { get; set; }
}

public class ContextMenu
{
+ public static readonly StyledProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public CustomPopupPlacementCallback? CustomPopupPlacementCallback { get; set; }
}

public class ToolTip
{
+ public static readonly AttachedProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public static CustomPopupPlacementCallback? GetCustomPopupPlacementCallback(Control element);
+ public static void SetCustomPopupPlacementCallback(Control element, CustomPopupPlacementCallback? value);
}

Obsolete APIs changes (no binary breaking changes, except NotClientImplementable interface):

[NotClientImplementable]
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
public interface IPopupHost
{
-void ConfigurePosition(Visual target, PlacementMode placement...);
+void ConfigurePosition(PopupPositionRequest positionRequest);
}

+[PrivateApi]
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
+public class PopupPositionRequest
+{
+internal PopupPositionRequest(Visual target, PlacementMode placement)
+internal PopupPositionRequest(Visual target, PlacementMode placement, Point offset, PopupAnchor anchor, PopupGravity gravity, PopupPositionerConstraintAdjustment constraintAdjustment, Rect? anchorRect, CustomPopupPlacementCallback? placementCallback);
+}

public class OverlayPopupHost
{
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
public void ConfigurePosition(Visual target, PlacementMode placement);
}
public class PopupRoot
{
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
public void ConfigurePosition(Visual target, PlacementMode placement);
}

Checklist

• Added unit tests (if possible)? Note: unit testing here is a bit limited, so it's not 100% covered.
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentation

Breaking changes

None.

Obsoletions / Deprecations

See API changes.

Fixed issues

Fixes #15233

[robloo]

I think this PR is a good example of an old discussion where we need to start organizing controls by feature/control rather than putting them all together. In this case I think Popup needs its own directory.

• Move PopupPositioning contents into the new Popup directory (get rid of that directory entirely)
• Move all Popup related types into that directory too
• Separate out some of the new types into their own files and place in that directory as well

Small projects in the MVVM world organize by type, I get that. But inevitably with large projects you end up organizing by project/namespace and then by feature. All large-scale projects I've worked on evolve to organize by feature.

[robloo]

I would also break this out into a new file.

Additionally, if it was me, I would make is a class so it could actually be extended if ever needed. This is too complex of a type to lock-down to a struct in my opinion.

[maxkatz6]

I can see PopupPositionRequest being complex (which is why it's a class already), but why CustomPopupPlacement? Essentially, it's just a three enums and a Point property.

[robloo]

In my mind struct is for primitives types (int, size, etc.) this is more complex. The reason to make it a class is to not limit yourself in the future (keep the door open for future extension by developers). There also doesn't appear to be a performance reason for this to be a struct.

So my criteria would be

1. Everything is a class by default in an object-oriented language
2. Is it a primitive type? then make it a struct
3. Is it performance critical? then make it a struct.
4. Is it a special-case?

It fails both of these struct tests and isn't a special case so my convention would be to make it a class. By default types should be a class in an object oriented language like C# -- not a struct. I do think Avalonia went a little too far with struct usage elsewhere.

[billhenn]

I haven't tried the current prototype implementation yet, but my original thought after digging in when writing the issue was that we could improve over WPF's implementation by still allowing default positioning logic to run but also give the callback, if specified, a way to further adjust it.

After doing a quick review of the code changes for this PR, this is a summary of the public API changes:

• PlacementMode.Custom value was added.
• public delegate CustomPopupPlacement CustomPopupPlacementCallback(Size popupSize, Rect targetRect, Point offset); was added.
• struct CustomPopupPlacement was added for a result of the CustomPopupPlacementCallback.
• Popup, PopupFlyoutBase, ContextMenu, and ToolTip added CustomPopupPlacementCallback properties.
• PopupPositionerExtensions.BuildParameters is where the meat of the logic is for positioning everything.

My thoughts on the updates are:

• I don't love how WPF and the changes above pass multiple arguments to CustomPopupPlacementCallback. It would be cleaner if it could pass a single object that had the data in it. Such as a pre-initialized CustomPopupPlacement. This would also allow for possible future options to be passed in without breaking changes.
• Instead of PopupPositionerExtensions.BuildParameters having a separate code path for PlacementMode.Custom, I'd suggest removing the PlacementMode.Custom value altogether.
• In PopupPositionerExtensions.BuildParameters, create a CustomPopupPlacement with all the values from whatever Placement is set instead of setting those values on positionerParameters right away.
• Then always invoke the CustomPopupPlacementCallback if one is specified and pass it the CustomPopupPlacement. This allows the callback to use the pre-initialized values directly if it wants.
• Some properties on CustomPopupPlacement could be read-only if appropriate (like PopupSize and AnchorRectangle) but allow others like Anchor, Gravity, ConstraintAdjustment, and Offset to be get/set.
• CustomPopupPlacement should probably be a class instead of struct.
• An updated delegate could be: public delegate void CustomPopupPlacementCallback(CustomPopupPlacement placement);
• After the possible call to CustomPopupPlacementCallback, PopupPositionerExtensions.BuildParameters would update positionerParameters with the get/set property values from CustomPopupPlacement. This should all be done before the FlowDirection logic kicks in that could possibly mirror things.
• You may want to rename the CustomPopupPlacement type and CustomPopupPlacementCallback delegate to something else if the above API changes are made since the behavior would be different than before.

The main reasons that I suggest the changes above are that it keeps the APIs simple and open for possible future expansion. But more importantly, the callback would receive all the pre-initialized anchor/gravity/constraint/offset values and the callback could leave them as-is or adjust them. Consider a case where we set Placement=PlacementMode.Bottom. If we have callbacks like above, it would already have the anchor/gravity set appropriately and the callback could do something like bump the offset down another few pixels.

It's very open ended with the suggested changes without requiring the developer to do too much either.

I'm happy to discuss further.

[robloo]

But more importantly, the callback would receive all the pre-initialized anchor/gravity/constraint/offset values and the callback could leave them as-is or adjust them.

This is a very good idea!

[maxkatz6]

I might end up wrong here, but positioning API wasn't changed in couple of years, without any need or plans to change it either. Especially when current positioning API is mirroring wayland as a common denominator.

I also not sure about reusing the same class (CustomPopupPlacement) for callback input and output. Since input will always include additional parameters - anchor rect and popup size. While these values are never returned from the callback. Marking them readonly helps a little, but is it really worth it?

PlacementMode.Custom was added not only to mirror WPF API, but also mirror current Avalonia PlacementMode.AnchorAndGravityAPI - which enables other two optional positioning parameters.

I am going to leave this PR as is for now, as I am currently on vacation. More opinions are welcomed here.

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0048338-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]