Forum Xamarin.Forms

Remove 'internal' and 'sealed' modifiers from Forms source code

DavidDancyDavidDancy AUMember ✭✭✭✭

Apologies if this is not the correct place for this suggestion.

In the past it has been argued that maintaining the framework is easier with internal and sealed classes because the implementation of things can change without altering the public API surface.

Now that Xamarin.Forms is Open Source there seems to me little point in maintaining this practice. While it may be "good design" (and that's debatable) there's simply nothing to stop interested parties from forking the source and treating it all as public regardless of the designer's intentions. In addition, internal and sealed have never prevented anyone from accessing the internals of Forms by reflection, so their usefulness is moot.

One of the value-adds of the Forms framework is as a wrapper over the native layer that abstracts concepts and provides a basic implementation that's usually "good enough". However on the occasions when it's not "good enough" the internal and sealed classes in the framework do a massive disservice to anyone wanting to adjust the details of the implementation. They usually cause one to question one's wisdom in building anything with the Forms framework in the first place, since they make things either unreasonably difficult or downright impossible.

It would be much better if the framework were designed as a starting point (i.e. with extension in mind, everything public or protected, virtual where possible) rather than an ending point (i.e. internal and sealed classes everywhere).

Tagged:

Open · Last Updated

Posts

  • JoeMankeJoeManke USMember ✭✭✭✭✭

    +1. So much renderer code spent writing around internals and private members and methods.

  • AdrianKnightAdrianKnight USMember ✭✭✭✭

    In general, I avoid forking other people's project to modify it to my tastes. Maintaining a large 3rd party project is not an easy task, and many people won't know/understand the architecture in detail. If there is a need for somebody to inherit from a sealed class, first we should understand why and maybe the XF team can adjust the original codebase accordingly. Similarly, while reflection "works", it's not the intended way to interact with lower level code. I'd rather have the team surface APIs so I achieve what I want. Also, CLR makes JIT optimizations to sealed classes though the benefits might be minimal (one has to run tests in the case of XF + mobile OSes + various devices). There might also be reasons to seal classes such as when using unsafe code.

    That said, I'm generally okay with getting rid of both internal and sealed modifiers, but I would like the team to properly document their code and design their APIs in light of more "open" modifiers. The team has fallen behind in terms of documentation while fighting fires and adding new features.

  • VelocityVelocity NZMember ✭✭✭

    I think you both make excellent points @DavidDancy @AdrianKnight. I have on occasion found the sealed or internal modifiers inconvenient when trying to get at core XF code/renderers, but to be honest it would be fairly rare. Perhaps a compromise can be made where unnecessary closed modifiers can be changed to be more "open", but maintained where they are needed to protect core framework code.

    A review would need to be done, and perhaps this can be handled on a case-by-case basis. I would be strongly in favour however of continuing to surface and enhance existing APIs to address any shortfalls, rather than 'opening all the taps' so to speak. A balance needs to be found here though - as access to some currently "closed" classes would be useful.

  • ChaseFlorellChaseFlorell CAInsider, University mod

    A situation that hits me often is the VisualElement.IsFocusedPropertyKey field. It's internal and impossible to set focus (which in turn raises the events) without using reflection.

    Here's the hackey work-around that I had to do in order to set IsFocused from within a ViewRenderer
    https://github.com/XamFormsExtended/Xfx.Controls/commit/af4c8f1677a8756527dee5bebeffb89656cf8ae8

  • DavidDancyDavidDancy AUMember ✭✭✭✭

    @ChaseFlorell That one bit me too. Also WebViewRenderer on iOS has a whole suite of internal/sealed helper classes that are needed-but-not-available if you subclass the control. And ImageSource - the entire ecosystem is sealed.

    To me this is about a design philosophy more than anything else. If you're making a framework that's designed to be extensible, internal and sealed will be a last resort. If you're making something that's trying to be the last word in cross-platform frameworks, internal and sealed will be everywhere. Currently we fall somewhere between the two stools. On the one hand we have things we're encouraged to subclass (renderers) but on the other we're prevented from doing so by the internal/sealed parts of those classes. How any class designer can predict the usage pattern of any object they make is beyond me. My users always do things I haven't planned for.

  • RobLanderRobLander GBMember ✭✭

    +1. Writing own custom renderers is a nightmare because of internal fields. Just make them protected

Sign In or Register to comment.