gui-cs
C# 9 & View.cs NRT migration
Please look at this. Migrating to latest C# is not a problem (it's always been this way - it's just that certain features require either special type or runtime support, lang-only features have always worked well).
So C# 9 just requires LangVersion attribute set to 9. Somefeatures require either manual code copy-pasting or a nuget.
For NRT I've utilized https://github.com/manuelroemer/Nullable package
I don't have all the context about gui.cs sadly, but this is not meant to be "the great refactoring", morel ike a poco. There are many questions we need to decide when migrating to NRT - mainly what really can be null and should be validated and what is not.
NRT really brings out the fact that there are lots of null landmines, sadly.
I would really like to bring out "Empty" versions of type like Dim.Empty, View.Empty etc but the problem is that they are not readonly. So I guess we have to stick with nulls. But at least I believe we have to make some things null-clear.
@BDisp @tig @tznind
Thanks for taking the time to look into this. I don't think I am equipped to review this PR but thought I would share some general thoughts.
Using C#9 and C#8 features is cool especially if it eases readability of code. So for example the null invoke operator ?.SomeMethod() or the fancy switch statements stuff that they brought out. The later language version doesn't change the code that gets compiled so it still works with Dotnet Framework etc (as you say).
That said we do need to consider a few things. First thing we need to remember is that Terminal.Gui is a shipped library used by lots of people which means we have a responsibility not to break the API. That can take many soft forms as well as hard breaking changes
- If a method throws a different Exception Type than before (can break consumer
catchblocks) - If a method can no longer take null parameters
For example making a public property non-nullable that was previously nullable means anyone who has code that sets it to null will have to rewrite after updating the package.
I definitely think using the nullable feature on new classes makes a lot of sense assuming it doesn't disrupt older code. I'd also recommend being careful when 'migrating' old code to use nullables as anytime you are rewriting old code there is a chance of introducing bugs.
There are also other ways of detecting null related issues for example the CI tool LGTM is pretty great at detecting unused variables and missing null checks (and that requires no code change - just some CI yaml files).
Finally I don't think View.Empty makes sense. Empty is typically used on structs and value types (and Enums) but null for a class means empty so there's not a lot of reason to do it for View. Especially since this would be a breaking change.
I don't mean to be negative just want to make sure we approach this refactoring responsibly and carefully.
Good stuff, thanks. Please, do a full format with Ctrl+K+D, if you are using VS2019, to accomplish the .editorconfig.
The annoying is the Roslynator warnings. He should ignoring them if the #nullable enable directive is set.
It must be compatible with .Net.Framework to work with Mono, as @migueldeicaza need.
But I've to agree with @tznind. It could break with some users codes. Class type needs to be nullable and not empty to take some desired actions.
But, we need to considering that all unit tests have passed and the UICatalog run fine.
@En3Tho I made changes to fix the warnings. See here https://github.com/BDisp/gui.cs/tree/csharp-9-and-nrt.
Well, for now I don't see a problem with breaking signatures as migrating to NRT will most probably cause warning in projects which have NRT enabled. But it's up to people if they choose to use it.
Exceptions are different thing - throwing argument null exceptions in null cases will be a breaking change. But I believe it's better to throw ArgNull than NullRef.
It's just that in some cases we end up with nullrefs and user won't event know if he had done something wrong or it's a library bug.
I think one of possible NRT transitions can be just annotating everything with possible nullability and then slowly taking it away? But these are breaking changes too...
@tznind As for "Empty" - these are usually present in immutable types. For example: Dim - if i'm not mistaken, it is an immutable type actually. At least as I see, all of it's descendants are immutable. So Empty concept could be good for it instead of null. But this requires program logic to be aware that Dim can be "Empty" and I guess it's too late for this.
@En3Tho Pos and Dim for me are mutable, because they depend from theirs derived classes. It can be null, PosFactor, Percent, PosAnchorEnd, PosAnchorEnd, PosCenter, etc.
Empty isn't the better way to differentiate between a default instance class and a class not instantiated at all (null).
For example the Rect struct has a Empty field that default all values to zero and a IsEmpty method which returns true if (X == 0) && (Y == 0) && (Width == 0) && (Height == 0). So if we create a new Rect(0, 0, 0, 0), it was instantiated but is still Empty. If we want to check if the Rect object was provided on a parameter method or not, we can't and must accept the Empty. But this is a struct and acceptable. For a class we need frequently check if the object was created or is null, since Empty only will indicate it have the defaults values and not if it was been created or not.
@BDisp I see. Well, that's why I said that I do not posess the full knowledge and can only guess. I think can provide a small file by file pull requests like (but I do not think it will be too often as I have work/other projects to attend) and we need to test it on different platforms, ensure that netfw is working properly.
Do you guys think we need a separate nrt branch or it is better to just merge to main bit by bit?
Do you guys think we need a separate nrt branch or it is better to just merge to main bit by bit?
It's better to wait for @tig saying something.
Do you guys think we need a separate nrt branch or it is better to just merge to main bit by bit?
It's better to wait for @tig saying something.
First, I love learning about and leveraging new C# and runtime features. Learning is one of the main reasons I maintain this project. So, from that perspective, I really want to do the NRT thing in Terminal.Gui.
However, we have promised the users of this library we'd hold a high bar for breaking changes. Thus just accepting this PR now, isn't going to work.
What I suggest is we start a Terminal.Gui v2.x.x branch. What do y'all think of that?
It's fine with me. I can contribute when I have spare time. I believe leveraging NRT will help other people get their head around the "scary" parts of the code.
Generally the issue is that we don't have a way of determining where gui.ve users are, on Framework or .net core.
I personally still use framework on Mac as it has a faster launcher and debugger.
I am willing to move to core, but not if we end up dropping users. We would need a plan for that.
I understand your concerns but it's important to know that some of newer C# features like NRT do not require modern runtime. Some of them so and some don't. NRT requires only fee special attributes to be present.
Although NRT (Nullable reference types) is very Interesting, the most needed for using C# 8.0 or greater is the ability to have many others features, using statements like protected, virtual, protected set, private set, etc... on Interface to have much more control of the code. But if we only using netstandard2.1 or net6.0 the .NET Framework users will don't have access to Terminal.Gui API. I think today is possible to use WinForms, ASP.NET, WPF, etc... with Net5.0 or greater. The only problem is migrate to the newer version without big problems. A survey to investigate this will be good, but I think the change will be inevitable some day.
We need to create an Issue for this set to point to the v2 Project (https://github.com/orgs/gui-cs/projects/1) and retarget this PR at the v2_develop branch.
Closing as I believe we've agreed to not migrate completely to NRT in v2, but use nullable sparingly.