Remove internal access modifier from IWindowImpl.GetWindowsZOrder

[stevemonaco]

What is the current behavior?

External platform implementations requiring IWindowImpl cannot be created by third-parties (affects 11.1.0 and 11.1.1 releases).

Fixed issues

Fixes #16553

[avaloniaui-bot]

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

[robloo]

Is there any other place where there are two lists/spans in a public API returned together where indexes are expected to match?

I'm pretty sure this is an API design violation. A single list should be returned with a container containing the window/zindex. Otherwise all consumers have to check and verify indexing.

This was internal for a reason I think.

[robloo]

On second thought, I don't think this should even be here. This interface is for a single window. We can have a GetZIndez that returns for the window instance ONLY. And higher level functionality should be some place else like a window manager.

[maxkatz6]

@robloo that's a private API.

Public API method has different definition:

Avalonia/src/Avalonia.Controls/Window.cs

Line 849 in 4c2d9fc

public static void SortWindowsByZOrder(Window[] windows)

[BAndysc]

@robloo you can read why there is no GetZIndex method and why Sort method is a static method in Window class in the original PR: #14909

---

Sorry for my oversight with the internal member here, I didn't realize I should have checked the interface attributes.

[avaloniaui-bot]

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

[robloo]

@maxkatz6 @MrJul This is "public" in the sense that 3rd parties are implementing it already. Although I realize Impl are considered unstable. I'm still really disappointed in this design. It's a mistake to have two arrays indexed together like this. It would have been better to use a Tuple within the Span at least.

Moreover, the architecture is very questionable. First, I think we got lazy adding this to the IWindowImpl interface. We are starting to have a number of issues in this area IMO. TopLevel, App, Lifetime and now Window are convoluted and the architecture and functionality was not designed properly between these. I ran across this already with the shutdown logic. Now here with Window being aware of and containing higher-level functionality. It's certainly not properly following a control hierarchy in the case of Window...

@BAndysc

This whole thing feels hacky to me. Your first ideas in the original PR seem much more in-line with what I am thinking here. I'm really surprised the core team asked you to change it. I disagree with the reasoning.

1. I would have added this functionality to the lifetime and kept sorting internal.
2. In fact, I would have the existing Windows property always return the windows sorted by ZOrder. Then the actual sorting logic like this doesn't need to be exposed to the application which is very strange especially as a static method in the Window.
3. The actual functionality to get OS window Z order should have been in IWindowingPlatform.
4. Proper API design should have been the number 1 consideration in the original PR and it seems it was not.

So long story short I still think all of this is a design mistake even though I agree the functionality is very important to have.

[stevemonaco]

@robloo I suggest opening a new issue for redesign discussion. This PR is only meant to remedy a breaking change for third-party platform implementers that can't be worked around. This makes Avalonia better today with minimal effort. There aren't contract guarantees, so the API could be moved or reworked. I have no strong opinions about this design as I don't use the functionality.

[MrJul]

@robloo

I'm answering here a last time, but let's open an issue or discussion if you want to push this further.

It's a mistake to have two arrays indexed together like this. It would have been better to use a Tuple within the Span at least.

While it's not the best API ever, I personally think it's fine enough for a low-level API, especially an internal one. For example Array.Sort(keys, items) (https://learn.microsoft.com/en-us/dotnet/api/system.array.sort?view=net-8.0#system-array-sort-2(-0()-1()) which is very similar has existed forever.

Moreover, the architecture is very questionable. First, I think we got lazy adding this to the IWindowImpl interface.
The actual functionality to get OS window Z order should have been in IWindowingPlatform.

Here I completely agree, it should definitely have been part of IWindowingPlatform or equivalent, it's quite odd on IWindowImpl.

TopLevel, App, Lifetime and now Window are convoluted and the architecture and functionality was not designed properly between these.

I also agree. Lifetime is a bit separate (it's really a higher optional level API compared to the *Impl one), but ITopLevelImpl and IWindowImpl are becoming a bit messy are are due for a cleanup. (For example, I've wished several times that Window wasn't a TopLevel.) But we can discuss that separately.

This is "public" in the sense that 3rd parties are implementing it already

I disagree with this point: these are marked unstable for a reason. Believe me, I've mentioned before v11 that I was disappointed in so many public API being made "semi-internal". But that's also the strength of the system: we're able to change them. People have been warned, and if anyone implemented them, they should be prepared for potential breaking changes. And I'm including myself in those people. It's not a perfect system, it's a difficult balance, but I much prefer this to having everything internal (I'm looking at you, WPF).

That being said, I really hope that in the future their shape would be stable enough to make them officially public again.

Regarding the ZOrder API itself:

Your first ideas in the original PR seem much more in-line with what I am thinking here

The original PR, while being better at first glance, was posing problems in both performance (which isn't good but can sometimes be accepted), and more importantly in correctness. Z-order stability wasn't guaranteed, which completely defeated the purpose of a Z-order.

In fact, I would have the existing Windows property always return the windows sorted by ZOrder.

So paying the overhead of iterating through most windows in the operating system to simply list the app's windows? For an order most people won't care about? Which could change between two properties calls? I'm sorry, but that's a recipe for disaster: a simple for loop without capturing the property could potentially return the same window twice.

I believe that every feature should be "pay for play" as much as possible.

Proper API design should have been the number 1 consideration in the original PR and it seems it was not.

Correctness is probably the number 1 consideration, but I agree that that the API design could have been better. Honestly, I wish we could have a proper API review process, that would work for both the core team and contributors. I know this has been discussed briefly in the past, but I think it's becoming more and more important. Even though I don't agree with everything you said, you raise an important point here.