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

Proposal: Unseal all the things #780

Open
ryandemopoulos opened this issue May 30, 2019 · 45 comments
Open

Proposal: Unseal all the things #780

ryandemopoulos opened this issue May 30, 2019 · 45 comments
Assignees
Labels
feature proposal New feature proposal needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Controls Issue for the Controls team wct

Comments

@ryandemopoulos
Copy link
Member

ryandemopoulos commented May 30, 2019

Summary

UnsealAllTheThings

Rationale

  • Enable developers to subclass a wider range of our classes/controls (longstanding request from our community)

Scope

Capability Priority
Unseal all, or at least nearly all classes in the framework Must

Important Notes

  • To WinUI community: Internally our MSFT XAML team has been planning to do this. I'm opening this issue to encourage additional conversation / interaction / questions about this work.
  • This work will require WinUI 3, so I've added the needs-winui-3 label to the issue.
@ryandemopoulos ryandemopoulos added feature proposal New feature proposal needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) labels May 30, 2019
@SavoySchuler
Copy link
Member

This is a fantastic proposal, @ryandemopoulos! I can't wait to see what the community can create with this barrier removed.

@huoyaoyuan
Copy link

Do we need some mechanism to allow custom unsealed controls?
Currently, we can create a control into an unsealed .NET class, and only .NET dependencies can derive from it. Given that most client UI codes are written in .NET, this is not so painful.

@MikeHillberg
Copy link
Contributor

What you can't do today is subclass the built-in types that are sealed. For example you can subclass TextBox, but not TextBlock.

@crhaglun
Copy link

Does this imply that <Style.TargetType> will need to start respecting subclasses? (Or should that perhaps be a separate feature request?)

@MikeHillberg
Copy link
Contributor

@crhaglun, changes to Style.TargetType behavior would be a separate feature. Currently you can set the TargetType to a base class and use it with a derived type, for example you can set a ToggleButton style onto a CheckBox. But you can't define an implicit ToggleButton Style (defining it in a ResourceDictionary without a key) and have it automatically be picked up by CheckBox instances in that tree.

@crhaglun
Copy link

crhaglun commented May 31, 2019

@MikeHillberg right, and that's actually something I ran into today when working with the Photos app. There's a couple of custom controls that derive from AppBarButton that, when placed in a CommandBar, does not look anything like the vanilla AppBarButtons in the same CommandBar :-)
Turns out the CommandBarRevealStyle has a local <Style TargetType="AppBarButton" BasedOn="{StaticResource AppBarButtonRevealStyle}" /> which of course does not apply on controls deriving from AppBarButton.
If all controls become unsealed, wouldn't this become a much more common class of bug?
Or am I actually looking at a bug in the CommandBar template?

@MikeHillberg
Copy link
Contributor

@crhaglun, yes, more controls will have the opportunity for this problem. If you're interested in this it's worth opening a separate issue. We intentionally don't take class hierarchy into account with implicit styles, for fear of mysterious style application. WPF has a style aliasing feature which would be interesting to add here.

@jesbis jesbis added this to the WinUI 3.0 milestone Jun 20, 2019
@verelpode
Copy link

I agree and want this unsealing proposal to proceed, but I also think it'd be worthwhile to consider this alternative proposal to allow derivation of sealed classes:

Proposal: Extension/derivation of sealed classes (dotnet/coreclr#26465)

@verelpode
Copy link

verelpode commented Sep 2, 2019

On second thought, forget my above-mentioned proposal, because it falls into the domain of the .NET Foundation and this means the proposal becomes enormously difficult and time-consuming for non-technical reasons. This enormous non-technical difficulty and giant test of patience/determination can be completely avoided by taking the easier path of unsealing the classes.

@verelpode
Copy link

Unseal all, or at least nearly all classes in the framework

OK, not every class will be unsealed, says the proposal. Whenever the unseal decision for any particular class is borderline or difficult to decide, I'd like to suggest that "partial unsealing" be considered. The policy could be that borderline cases should fall in favor of "partial unsealing" if not full unsealing. I'm using the words "partial unsealing" to mean the following:

Fully sealed:

sealed class MyDerivedClass : MyBaseClass
{
    public override void Method1() { }
    public override void Method2() { }
    public override void Method3() { }
}

Fully unsealed:

class MyDerivedClass : MyBaseClass
{
    public override void Method1() { }
    public override void Method2() { }
    public override void Method3() { }
}

Partially unsealed:

The class is unsealed but every (or many) virtual members are sealed:

class MyDerivedClass : MyBaseClass
{
    public sealed override void Method1() { }
    public sealed override void Method2() { }
    public sealed override void Method3() { }
}

For example, let's hypothetically pretend that the unseal decision for TextBlock is borderline and the final decision is partial unseal. If every member inside TextBlock is sealed, then obviously subclasses cannot override any members, but this doesn't mean that subclassing becomes useless.

The following example class doesn't override any virtual members, but nevertheless succeeds in achieving a goal of making a derived class of TextBlock that auto-shrinks the font size when the TextBlock is resized to a smaller size (just for example). Furthermore, it also succeeds in its goal of adding support for a fictional interface IConvertibleToRichText that allows objects or UI elements to be converted to RichEditTextDocument.

class MyExtendedTextBlock : Windows.UI.Xaml.Controls.TextBlock, IConvertibleToRichText
{
    public MyTextBlock()
    {
        // Subscribe to public event "SizeChanged" defined in a base class:
        base.SizeChanged += this.OnSizeChanged;
    }

    private void OnSizeChanged(object sender, SizeChangedEventArgs e)
    {
        if (this.IsAutoShrink)
            base.FontSize = XXXXX;
    }

    public bool IsAutoShrink { get { ... } set { ... } }

    RichEditTextDocument IConvertibleToRichText.ConvertToRichText()
    {
        return XXXXX;
    }
}

Thus a "partially unsealed" class still supports a useful degree of derivation, albeit with restrictions. So, if partial unseal is also on the table in addition to full unseal, then a wider range of classes/Controls could be subclassable; wider than if only full-unseal is on the table.

@MikeHillberg
Copy link
Contributor

Xaml's surface area is in WinRT APIs, and WinRT today only supports sealing of classes, not members.

@verelpode
Copy link

@MikeHillberg -- oh, pity. It would have been nice to have the "middle ground" option available, especially because Windows is not a theoretical OS. "Perfect in theory" == "Imperfect in reality".

If the argument is that member sealing is barely beneficial, then it can also be said that class sealing is barely beneficial, therefore both kinds of sealing could be removed (this is debatable). Anyway, I still like the unsealing proposal regardless of having no option for partial unsealing.

@weltkante
Copy link

weltkante commented Sep 5, 2019

It doesn't work in .NET either, you can't seal interface methods only class methods, subclasses are always allowed to override interface methods by reimplementing the interface. So while I like the idea in general, it would have required .NET runtime and possibly language changes for it to truely protect the base implementation.

@verelpode
Copy link

@weltkante -- Smart point about the interface reimplementing!

Re the protection point:

to truely protect the base implementation.

Who is really protecting who? The usual assumption is that it's about protecting the base implementation, but shouldn't this assumption be questioned?

The sealed base class says to the disallowed derived class: "I've decided to protect you from potentially breaking in future, therefore you're banned!"

The banned derived class replies: "Err... thanks, I guess. Do you also want money in return for generously providing me with this protection service that I didn't request? Do I have any choice in the matter? Can't you just warn me and give me the freedom to make my own decision about whether to take the risk? I'm over 18 years old, you know."

The sealed base class replies angrily: "I'm your parent class and you will do as you're told! Is that clear?!?!"
😃

@verelpode
Copy link

Performance loss associated with unsealing?

So far, nobody here in this repo has complained about a loss of performance/optimization associated with unsealing these WinUI classes, but someone is bound to mention it soon. I have zero complaints about this seeming loss because this oft-repeated sealing-optimization argument doesn't hold water, as far as I know. The oft-repeated claim is that when a class is sealed, the CLR/JIT knows with certainty that zero subclasses exist, and this enables various optimizations. This argument doesn't pass the test of logic, as far as I know, but everyone is welcome to correct me if I'm mistaken -- I'd be fascinated to hear the logic.

I believe it's a case of logic dependency. The argument can only pass the logic test if the dependency also passes. If the argument's dependency fails the test, then the argument also fails the test. The dependency is the JIT compiler. The JIT fails the logic test, therefore the sealing-optimization argument also fails the logic test.

i.e. the JIT exists for primarily non-technical reasons: Sun invested heavily in promotion of its Java Virtual Machine (JVM), and engaged in marketing claiming various advantages of new tech ideas such as JIT etc. Thus for marketing/business reasons, it become necessary for Microsoft to likewise produce such a VM with JIT. Sun said, "We have JIT tech!" and Microsoft was then able to reply, "We have JIT tech also!" If you set aside the business reasons and consider only the technical reasons, then the JIT makes no sense, except in uncommon special cases.

JIT is a highly jittery claim therefore JIT is an appropriate name for such tech :-) Thus the logic chain for "sealed" breaks: The "sealed" keyword helps the JIT perform optimizations, but this is invalid because the JIT serves no purpose in normal apps. (To be precise, no technical purpose. It served a marketing purpose in the past.)

When a UWP app is compiled with the Release config instead of Debug, meaning when the .NET Native toolchain is used, the performance is (or should be) identical regardless of whether a class is explicitly marked "sealed" or not, because the .NET Native toolchain is able to determine that a class has zero subclasses regardless of whether the class is marked "sealed". Thus the "sealed" keyword should be irrelevant in regards to performance/optimization.

Normally JIT shouldn't be used. The .NET Native toolchain represents the way it should have always worked from the beginning, but this is easier to justify today than in the past, because today it's no longer necessary for Microsoft to battle Sun's Java marketing.

The CLR is useful when a UWP app is compiled with the Debug config, because it makes the app faster to compile and startup repeatedly -- a frequent activity in the testing and debugging phase. Although the "sealed" keyword allows the CLR+JIT to run parts of the app very slightly faster, this optimization serves no purpose because the Debug config doesn't need max performance, and the Release config shouldn't use JIT anyway.

Now that the Sun/Java marketing problem is out of the way, it's great to see that MS is using the .NET Native toolchain instead of jittery tech. Thanks for this excellent improvement! 👍

@weltkante
Copy link

weltkante commented Sep 5, 2019

When I was talking about "protecting the base implementation" I mostly meant "protecting the invariants required by the base implementation to be correct". When programming the programmer makes assumptions, being able to inject 3rd party logic in arbitrary points will usually break some of those assumptions and introduce bugs in the base class. Making things overrideable is a design decision introducing additional effort if you care about correct code.

Your concept of partial unsealing (or subclassing without overriding anything) is nice because theoretically it could be made safe to not break any invariants of the base class, but I think it needs support from the language/runtime to actually work as a building block which can compose classes in a safe way (safe as in "not introducing bugs by breaking assumptions")

@MikeHillberg
Copy link
Contributor

Re: protecting the base implementation

The concern we've always had about unsealed classes is that it has to be well-designed to tolerate subclasses overriding a random number of members, maybe calling base or maybe not, maybe calling base before or after doing some work, etc.

For example, maybe Base has OnFoo and OnBar virtuals, OnBar assumes that OnFoo ran first (it's a virtual not an abstract), but the subclass overrode and didn't call OnFoo. So Base needs to write OnBar to tolerate this, and have test code to validate it, etc.

That's a somewhat false argument because it's not caused by an unsealed class, it's caused by having unsealed members on an unsealed class. Sealing a class just helps because you don't have to figure out which virtual members you need to seal.

The truth to that argument though is that you can override interfaces, and all class members are internally implemented as interfaces in WinRT. So the unsealed, activatable types in Xaml today are susceptible to this. But there are hundreds of those today, it just hasn't been an issue, and there's real value to unsealing.

@verelpode
Copy link

@weltkante

I mostly meant "protecting the invariants required by the base implementation to be correct". When programming the programmer makes assumptions, being able to inject 3rd party logic in arbitrary points will usually break some of those assumptions and introduce bugs in the base class.

That's the standard answer, but isn't this one of those situations in life as follows?

  1. The highly-educated professionals express their knowledge of an advanced theory.
  2. A child of one of the professionals (or a layperson or beginner or apprentice) overhears the conversation between the professionals and states an observation or solution that sounds simple-minded, inexperienced, ingenuous.
  3. The professionals chuckle among themselves and smile at the child/layperson/apprentice, amused by the simple-minded comment. One or two of them replies condescendingly to the child, while the others hold back.
  4. A few days or weeks later, the professionals eventually realize that the child's observation was fundamentally correct!

Thus sometimes it pays to push myself to try to think like the child. Therefore, I now present the seemingly amusing simple-minded answer from the child.
Firstly, you said: "being able to inject 3rd party logic in arbitrary points will usually break some of those assumptions and introduce bugs in the base class."
The child replies: "When not even one single character of the base class .cs file is modified, no bug is introduced in the base class!"

@weltkante

This comment has been minimized.

@verelpode
Copy link

verelpode commented Sep 5, 2019

@weltkante -- I wanted to make the conversation lighter while simultaneously communicating valid serious points. I'm certainly interested in your opinion and I understand that you clearly made very good points there. I agree with your points. I'm just saying that I think those points aren't the end of the story. I believe there's more to the issue.

You've probably heard about the difficulty of encouraging people to think outside of the box, in order to find solutions or gain better understanding of a challenging problem. It is difficult to achieve this outside-of-the-box goal. I don't claim to be an expert in this, but I can say that I've heard that silly allegories (or the like) can assist teams to think outside of the box.

So it's not intended as disinterest in your opinion, rather it's actually more like the opposite: I'm so interested in your opinion that I want to expand upon your opinion by using an outside-of-the-box technique.

I understand if you don't want to try this outside-of-the-box technique -- it doesn't suit everyone, and it has no guarantee of success. If you (or anyone else) would like to try out the technique, then you can reply to what the "child/apprentice" said, and we can see where it goes, and see whether the technique succeeds this time. But it's certainly understandable if you don't want to.

@verelpode
Copy link

@weltkante -- Oh, yes, I see what you mean. Yes, you're right to be bothered about the fact that I ignored this comment of yours:

Your concept of partial unsealing (or subclassing without overriding anything) is nice because theoretically it could be made safe to not break any invariants of the base class, but I think it needs support from the language/runtime to actually work as a building block which can compose classes in a safe way (safe as in "not introducing bugs by breaking assumptions")

I'd also be bothered the same as you are, if I wrote the above and it was simply ignored. Fair complaint. What you said is an idea worth exploring but it was unfairly ignored. Yes your irritation makes sense.

The reason I ignored it is not about you. The reason is actually unrelated to you. I agree with you that the concept would benefit from support from the language/runtime, but this idea was already declined by the CoreCLR team recently. (See the proposal that I linked in an earlier message.)

That's why I ignored it -- it's a dead end unfortunately. My agreement with your opinion doesn't change the fact that the CoreCLR team closed the issue and that's the end of it. I've accepted their decision regardless of whether I disagree with it. Therefore, the discussion here is limited to discussing unsealing in ways that don't require support from the runtime.

@weltkante

This comment has been minimized.

@robloo
Copy link
Contributor

robloo commented Aug 4, 2020

Can this be done in the next WinUI 3.0 preview? Seems very simple to do and would close a few issues and help people porting from WPF.

@jtbrower
Copy link

I am here to show support for as many unsealed classes as practically possible. Virtual methods are always a huge plus too.

My main concern today is with respect to the RepeatButton.

ButtonBase, ToggleButton, HyperLinkButton, DropDownButton, SplitButton and ToggleSplitButton, are all defined to be inheritable classes while RepeatButton is marked with the Sealed modifier. I will now need to re-invent the RepeatButton to achieve my goal today.

@marcelwgn
Copy link
Collaborator

@ryandemopoulos Do you have any updates on this issue? Is it already known which steps there are to unseal all the things?

Maybe this is something the community can help with when WinUI 3 is open source.

@robloo
Copy link
Contributor

robloo commented Sep 20, 2020

I just saw on the community call that this may not happen for 3.0 and would then be blocked until 4.0.

Simply put, thats not acceptable for a number of scenarios. Could a little background on why this takes so long to do be provided? It seems relatively simple.

If the code is made open source before 3.0 release this is also something the community might be able to contribute. As I said, I dont expect this is too difficult. I also can see winui 3.0 open sourcing not done until launch -- a full year behind schedule.

@jtbrower
Copy link

So far I'm glad I changed course and evaluated Avalonia. I have been able to achieve much more in a shorter timeframe so far.

For WPF centric developers writing Desktop applications who seek better rendering performance with CSS style selectors, it's a great choice. Cross platform capabilities are a huge bonus considering that wasn't my goal for finding a WPF alternative. (I initially chose it over Uno because Uno seemed to have a dependency directly on WinUI.)

This isn't meant to downplay WinUI or any teammate working tirelessly on the project. However, there's just too many signs that more time is needed for them to provide a decoupled and mature enough framework for enterprise application use.

My desperation for what WinUI could provide caused me to dive in too early. At the pace the project is moving, I have to wonder how relevant it will be for direct consumption by modern applications when it's ready. By the time it achieves the milestones we all need to see, it still won't provide cross-platform capabilities and would seem to affect downstream products like MAUI and Uno? Not facts, just wondering out loud since that's all we can ever do.

I know none of this is without challenge, but I just can't get over the fact that it's nearly 2021 and we still don't have the ability to write enterprise desktop applications using a UI framework started in 2012! There are countless half-baked ways to somewhat meet our needs, but WPF is still the best choice for Desktop developers if you want to stay with a Microsoft backed product, a goal I always try to achieve.

I believe in Microsoft's ability to execute a plan, just look at how amazing dotnet5.0 is! Knowing the brain power they have aboard, it makes it harder to accept that one of the most important pieces of application development is missing. We are missing WinUI.

Sorry to be a pessimist, I know COVID isn't helping anyone's schedule. I just wish Microsoft would throw a huge pile of cash at this and get it done. (If resources are the issue, I wouldn't know and don't try to pretend I do.)

Come on WinUI, we believe in you! You got this, give us the goodies so we can write some absolutely amazing Microsoft applications! We depend on you, are downstream from you and cannot shine without you.

I'm super impressed with the alternative I chose, I feel relieved I have a solution. However, I still have my bucket of popcorn and am eager to see this succeed.

@robloo
Copy link
Contributor

robloo commented Sep 21, 2020

Getting a little side-tracked but to continue your line of discussion: I wish Avalonia nothing but success. In the end it may win out against all the others. Innovation drives the future. The vast majority of design changes Avalonia made from WPF/UWP were done for justifiable and good reasons. That is not something that can be said for UWP (WinUI 2.x). UWP has been a thorn in the side of developers since its inception. However, it very much can be used to create WPF-level desktop apps. It just takes a bit more work in a few places -- less in others. Stability is the real issue though.

Uno has no dependency on WinUI other than it is following the same API and choose not to deviate. That is the correct choice for their use case and its what Maui should have done. I also wish Uno great success and am banking on it now myself.

As you said on another issue, it will be very interesting to see where we are in a few years time. There will be 3 cross-platform frameworks for writing c# apps with .net 5. It's a great place to be... we just have to get there before the competition.

Since XP, Microsoft's problem has always been the same thing: overpromise and under-deliver, then cancel when you can't get users and never understand why. I've worked for some very large companies and this is almost always the result of management turnover and too few people doing the work vs. managing the work -- among other reasons I won't add. I have nothing against the WinUI team and they do a lot of great things and I appreciate the transparency, even schedule slips. But I can't easily accept 1 year delays from a company this size.

@knat
Copy link

knat commented Jul 12, 2021

I want to derive from TextBlock/Border/..., I don't override ANY base methods, just add fields/methods/properties!

@michael-hawker
Copy link
Collaborator

Have been refactoring our GridSplitter in the Toolkit and was wondering why we never tried inheriting from Thumb as it is in WPF... finally just realized looking at it again that it's sealed... 🤦‍♂️ But it's a primitive...

Not sure if I should file an issue specifically for that or just add on to this one as I am doing here. But figured I'd call this out again, as it's a lot of duplicated effort to replicate some of the nuances that Thumb may be able to handle for us automatically.

@sylveon
Copy link
Contributor

sylveon commented Jul 8, 2022

Looking again at this. I've been wanting to build some reusable MenuFlyoutSubItems (because I have the same submenu a few times in my app, so wanted to avoid copying the XAML and code behind all the time), and was disappointed to see that it's sealed so there is no real way to avoid duplicating code.

@SkyeHoefling
Copy link

I was trying to inherit from Popup and noticed it is sealed 😢

@AdriaanLarcai
Copy link

Almost 4 years on, and it doesn't seem like this will happen... Quite frustrating.

@LeonSpors
Copy link

Could we get an update about the current state of the proposal if it is still open or declined.

@codendone
Copy link
Contributor

@LeonSpors Yes, this is still open and a desired feature. It still needs investigation to determine if it would be a breaking change (see the "OnFoo" comment above) and how to address that if it is.

@robloo
Copy link
Contributor

robloo commented Jul 19, 2023

Commenting to be absolutely sure this doesn't get auto-closed. This has been needed for years.

@michael-hawker
Copy link
Collaborator

Running into this issue again, specifically to help adapt code between platforms for UWP/WASDK/Uno Platform and RichTextBlock.

@peter0302
Copy link

Yes please.

Trying to make a DataGrid (no I don't want to use community toolkit) and would love to derive from Selector but instead have to reinvent the wheel.

@mrlacey
Copy link
Contributor

mrlacey commented Nov 22, 2024

Any update on this?

I wrote my just first bit of WinUI code (for a few months), and I'd forgotten how painful creating UIs was (compared to MAUI & WPF) because of this.

A quick search through the code shows the following sealed classes:

  • AutoSuggestBox
  • Border
  • ColumnDefinition
  • ColumnDefinitionCollection
  • ControlTemplate
  • DatePickerFlyout
  • DatePickerFlyoutItem
  • DatePickerFlyoutPresenter
  • HubSectionCollection
  • Image
  • ItemCollection
  • ItemCollectionTransition
  • ItemCollectionTransitionProgress
  • ItemContainerGenerator
  • ItemsPanelTemplate
  • ItemsPresenter
  • ItemsRepeaterScrollHost
  • ItemsStackPanel
  • ListPickerFlyout
  • ListPickerFlyoutPresenter
  • ListViewPersistenceHelper
  • MediaTransportControlsHelper
  • MenuFlyoutSubItem
  • PasswordBox
  • PickerFlyout
  • PickerFlyoutPresenter
  • RichTextBlock
  • RichTextBlockOverflow
  • RowDefinition
  • RowDefinitionCollection
  • ScrollContentPresenter
  • ScrollViewer
  • ScrollViewerView
  • SemanticZoom
  • SemanticZoomLocation
  • SymbolIcon
  • TextBlock
  • TimePickerFlyout
  • TimePickerFlyoutPresenter
  • ToggleSwitch
  • ToolTipService
  • UIElementCollection
  • VariableSizedWrapGrid
  • Viewbox
  • VirtualizingStackPanel
  • WrapGrid
  • XamlControlsResources

Of these, unsealing the following as a priority would enable writing code that was dramatically, shorter and easier to understand & maintain:

  • TextBlock
  • Border
  • Image
  • SymbolIcon
  • RichTextBlock
  • PasswordBox

I identify these as some of the most widely used controls, that we can't create subclasses from. As such, working with them is unnecessarily more complicated than I'd like.

@sylveon
Copy link
Contributor

sylveon commented Nov 22, 2024

A few of these have alternative extension means:

  • SymbolIcon is a glorified FontIcon, and you can extend from that (or just directly use FontIcon).
  • For XamlControlResources, you can simply insert it to the MergedDictionaries of any ResourceDictionary.
  • ToolTipService and MediaTransportControlsHelper are purely static classes for attached properties. There is no instance to extend.
  • UIElementCollection, ColumnDefinitionCollection and RowDefinitionCollection are not much more than an implementation of IVector<T>, you can just use List<T> or winrt::single_threaded_vector<T>.

I haven't looked much into the others.

I'm personally quite annoyed about the lack of inheritance for MenuFlyoutSubItem because it means a lot of repeated XAML.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature proposal New feature proposal needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Controls Issue for the Controls team wct
Projects
None yet
Development

No branches or pull requests