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

Update Flyout and Popup APIs to make gap between them smaller #10492

Merged
merged 13 commits into from
Mar 7, 2023

Conversation

maxkatz6
Copy link
Member

@kekekeks @amwx

What does the pull request do?

  1. Removes FlyoutPlacementMode and adds all new values to the Popup.PlacementMode.
  2. Makes Flyout use Popup.PlacementMode, as well as Horizontal/Vertical alignment and gravity/anchor properties.
  3. Implements PlacementMode.Center available for both controls (WPF compatibility).
  4. Splits FlyoutBase into two types: FlyoutBase and PopupFlyoutBase. Where FlyoutBase is a base class with limited functionality that can be possibly implemented on top of native flyouts (see NativeMenuFlyout support (native context menu) #8581). And PopupFlyoutBase has the same implementation as old FlyoutBase. MenuFlyout and Flyout classes always inherit PopupFlyoutBase, as they are specifically rendered by avalonia with full control (comparing to potential native flyouts).

Breaking changes

Behavior change.
Popup PlacementMode=Top|Bottom|Left|Right behavior was changed. Now it is centered and matches old flyout placement mode values. For example, previously Popup PlacementMode="Top" was the same as TopEdgeAlignedLeft currently is.

Also, FlyoutPlacementMode enum was removed, but should be a pretty small breaking change.

Fixed issues

Fixes #9583
Fixes #9582 (provides an alternative solution)

@maxkatz6 maxkatz6 added the api API addition label Feb 26, 2023
@maxkatz6 maxkatz6 requested a review from a team February 26, 2023 17:32
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0030795-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@amwx
Copy link
Contributor

amwx commented Feb 26, 2023

An additional thought since you're in this code (though could be done separately): it might be good to add FlyoutPresenterTheme to Flyout and MenuFlyout in addition to FlyoutPresenterClasses, since we have that now (and is closer to WinUI's FlyoutPresenterStyle)

src/Avalonia.Controls/Flyouts/PopupFlyoutBase.cs Outdated Show resolved Hide resolved
public static readonly DirectProperty<PopupFlyoutBase, IInputElement?> OverlayInputPassThroughElementProperty =
Popup.OverlayInputPassThroughElementProperty.AddOwner<PopupFlyoutBase>(
o => o._overlayInputPassThroughElement,
(o, v) => o._overlayInputPassThroughElement = v);
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably could too? (obviously requires the change in Popup)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about this property. Doesn't feel like OverlayInputPassThroughElementProperty is even needs to be styled, as it tied too specific.

src/Avalonia.Controls/Flyouts/PopupFlyoutBase.cs Outdated Show resolved Hide resolved
@jp2masa
Copy link
Contributor

jp2masa commented Feb 26, 2023

Related, maybe it can be fixed in this PR:

From
oxyplot/oxyplot-avalonia#49 (comment)
To
oxyplot/oxyplot-avalonia#49 (comment)

Or should I open a new issue?

@maxkatz6
Copy link
Member Author

An additional thought since you're in this code (though could be done separately): it might be good to add FlyoutPresenterTheme to Flyout and MenuFlyout in addition to FlyoutPresenterClasses, since we have that now (and is closer to WinUI's FlyoutPresenterStyle)

Good idea, added.

Related, maybe it can be fixed in this PR:

Couldn't reproduce it on control catalog for some reason. But I see possible reason, that _lastPointer was reset on pointer leaving the screen. If you can verify, please take a look. Committed in this branch.

@rabbitism
Copy link
Contributor

I believe this is a good change to fix the placement of tooltip, too. Right now, Tooltip placement is not correct imo.
image
image
image
image

@maxkatz6
Copy link
Member Author

I believe this is a good change to fix the placement of tooltip, too. Right now, Tooltip placement is not correct imo.

I want to rewrite tooltips to use Popup internally instead of low level components. Probably in another PR...

maxkatz6 and others added 2 commits March 5, 2023 00:57
@pr8x
Copy link
Contributor

pr8x commented Mar 6, 2023

Would be nice to add support for Popup.InheritsTransform in Flyouts

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0031609-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@jp2masa
Copy link
Contributor

jp2masa commented Mar 7, 2023

Couldn't reproduce it on control catalog for some reason. But I see possible reason, that _lastPointer was reset on pointer leaving the screen. If you can verify, please take a look. Committed in this branch.

Now after moving the window it shows at the old location the first time, then at the "correct" location (not pointer, but center of control), so it's much better, but not completely fixed.

Isn't it possible to update the pointer position before processing pointer events here: https://github.com/AvaloniaUI/Avalonia/pull/10492/files#diff-1c13e366d9f7efa03e332d42d84e61e1901fdd6d05a616c223edb87bcc02330fR151-R170? Or would that break other code?

@maxkatz6
Copy link
Member Author

maxkatz6 commented Mar 7, 2023

@jp2masa yes, it should work.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0031625-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@jp2masa
Copy link
Contributor

jp2masa commented Mar 7, 2023

@maxkatz6 It's fixed, thanks!

@maxkatz6 maxkatz6 added this pull request to the merge queue Mar 7, 2023
Merged via the queue into master with commit f247e89 Mar 7, 2023
@maxkatz6 maxkatz6 deleted the flyout-api branch March 7, 2023 09:13
@robloo
Copy link
Contributor

robloo commented Mar 8, 2023

@maxkatz6 Flyout has Placement, Popup has PlacementMode properties. Is this intentional? If so why?

/// <summary>
/// Gets or sets the placement mode of the popup in relation to the <see cref="PlacementTarget"/>.
/// </summary>
public PlacementMode PlacementMode
{
get { return GetValue(PlacementModeProperty); }
set { SetValue(PlacementModeProperty, value); }
}

/// <summary>
/// Gets or sets the desired placement.
/// </summary>
public PlacementMode Placement
{
get => GetValue(PlacementProperty);
set => SetValue(PlacementProperty, value);
}

robloo added a commit to robloo/Avalonia that referenced this pull request Mar 8, 2023
@maxkatz6
Copy link
Member Author

maxkatz6 commented Mar 8, 2023

@robloo historical differences I suppose. Same with Tooltip.
I think it would make sense to have a PR to change to PlacementMode everywhere.

@timunie
Copy link
Contributor

timunie commented Mar 8, 2023

@maxkatz6 I fully agree.

@amwx this maybe also applies to FluentAvalonia (for example the FlyoutButton)

@robloo
Copy link
Contributor

robloo commented Mar 8, 2023

I think Placement is a bit better of a name personally. Microsoft did too since that's what they changed it to in UWP and WinUI. The issue is it isn't a mode of placement, it is actually placement.

If you don't want to break existing apps ToolTip though, that is understandable.

@maxkatz6
Copy link
Member Author

maxkatz6 commented Mar 8, 2023

@robloo problem with "Placement" is that we have "PlacementGravity", "PlacementAnchor", "PlacementRect", "PlacementTarget"... "PlacementMode" fits here without any confusion as one of equally configurable properties.
I don't have strong opinion here though.

@robloo
Copy link
Contributor

robloo commented Mar 8, 2023

@maxkatz6 Well, I don't have a strong preference here either so perhaps compatibility with existing code wins.

"Mode" just seems like a little misnomer to me based on what the property does. It still has the Placement prefix to associate with all other related members as well. I realize you are referring to "Placement" as more of a grouping term though so naming a memer the same as the member group prefix name may be strange.

@robloo
Copy link
Contributor

robloo commented Mar 14, 2023

@maxkatz6 ToolTip has a Placement property as well. Then I rechecked Popup in WPF: https://learn.microsoft.com/en-us/dotnet/api/system.windows.controls.primitives.popup.placement?view=windowsdesktop-7.0. It also has a Placement property.

Therefore PlacementMode in Avalonia appears to have been a mistake. Placement is the correct terminology and I'll standardize the API to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flyouts should share PlacementMode with popups FlyoutShowOptions
9 participants