AvaloniaUI / Avalonia

Develop Desktop, Embedded, Mobile and WebAssembly apps with C# and XAML. The most popular .NET UI client technology
https://avaloniaui.net
MIT License
25.37k stars 2.2k forks source link

Refactor Avalonia.Controls #4957

Open robloo opened 3 years ago

robloo commented 3 years ago

I see a combination of organizational structures in the Avalonia.Controls directory.

  1. Some controls have their own folder
  2. Most controls are just in the top-level directory
  3. Some controls are grouped: Selection, Primitives, etc.

This should all be cleaned up in my opinion following the WinUI repo which is more-or-less the standard now days.

Note the following:

Open Questions:

If there are no main objections, I'll do this myself over time control-by-control each getting their own PR making sure to rename the files to preserve history (at least if using git --follow).

kekekeks commented 3 years ago

@grokys

robloo commented 3 years ago

Looking at the controls themselves, it is some of the cleanest code I have ever seen within a public project/framework :) Cleaning up the directory structure will only encourage others to look at the code. It will also speed things up for maintainers as well.

grokys commented 3 years ago

Hi @robloo, thanks for the compliment ;)

However I'm not sure about your proposal (although things could definitely be better).

This makes sense in WinUI where the controls are huge due to the overhead of C++/CX, for example in WinUI these are all the source files needed for ProgressBar: https://github.com/microsoft/microsoft-ui-xaml/tree/master/dev/ProgressBar. However in Avalonia, ProgressBar is a single class of 266 lines. Putting that into its own directory would just increase the amount of navigation to get to it IMO.

Again, makes sense for WinUI where controls can be tens of thousands of lines long in some cases but in Avalonia would IMO just make navigation take more clicks. Another example:

WinUI ScrollPresenter Avalonia ScollContentPresenter

You can see that the WinUI control is an order of magnitude bigger, partly because of the overhead of C++ and partly because it does a lot more. If/when we update our scrolling controls to do everything WinUI's do then we might want to place them in their own directory.

This makes more sense, but mostly when arranged alphabetically they're placed next to each other anyway

--

Another reason not to do this is that git/github doesn't follow renames/moves on renamed files when doing a git blame and we do a lot of git blame. This happened when we did the rename from Perspex -> Avalonia and I end up hitting this every week and cursing ;) This would be a large practical annoyance for day-to-day maintenance of Avalonia.

I personally think the way we do it at the moment gives a good tradeoff between navigation and cleanliness:

robloo commented 3 years ago

@grokys Thanks for the feedback!

I'm a big fan of consistency. Placing all controls in their own directory would help a lot for readability. I'm not so worried about navigation paths getting slightly longer :) But let's consider the Grid control as an example today:

This is ported from WPF and isn't a very clean code base. You also likely don't do much blame here as it was imported code. This is an ideal candidate to put inside it's own Grid directory. You could clean up a lot:

Grid

This certainly must qualify as a "Large controls get their own directory" :) ColumnDefintion and RowDefinition also aren't alphabetically next to each other.

I'm sure I could find a few other controls -- such as ComboBox/DropDown -- that would be useful putting in their own directory right now even if you don't want to do it for everything at this point.

A lot of controls have many properties. Separating out the properties in a partial class is quite useful as a separation of concerns. It significantly cleans up the code-behind file from boilerplate and improves readability of the logic. I promise if you try it out you will like it over time!

It also might help a little with importing code from other frameworks. Avalonia property differences are significant compared to WinUI/WPF. Therefore, breaking these apart right away would just help the migration story.

You can see that the WinUI control is an order of magnitude bigger, partly because of the overhead of C++ and partly because it does a lot more. If/when we update our scrolling controls to do everything WinUI's do then we might want to place them in their own directory.

Yes, a big part of this re-organization would be to future-proof the directory structure. Making the directories now means you wouldn't have to do re-naming or moving things that mess up the history in the future. It's always better to do these things sooner rather than later for the same reasons you gave -- to preserve history and blame as simply as possible. If you think you will change it in the future (and all other comparable XAML frameworks discovered after a while it was a good idea). Do it now! It gives you room to grow which you will need :)

Another reason not to do this is that git/github doesn't follow renames/moves on renamed files when doing a git blame and we do a lot of git blame

Well, yes, it has been a never-ending request for GitHub to support --follow in History and in Blame. I know history is just a missing feature in GitHub. I don't know if Blame preserves the information in git itself but would assume it does. Regardless, its possible to browse the repo at a previous commit online and see the blame as before. Reorganizing directories is really a long-term improvement though and there would be a -- in my opinion very minor -- tradeoff in the beginning where blame/history wouldn't be as usable with GitHub (but should still work in command-line).

robloo commented 2 years ago

@grokys With the merge of Automation peers this is even more relevant. You are moving all automation peers into a directory like this src/Avalonia.Controls/Automation/Peers/ComboBoxAutomationPeer.cs. However, it makes more sense to keep these with the control implementations. Every control would get it's own directory and the automation peers would go with the control code.

robloo commented 2 years ago

I think fundamentally, when projects are smaller, it makes sense to organize by type. You don't have that many classes and putting them in separate folders just adds more directory depth and clicks required to get a file. However, as projects grow and become larger in size, it makes sense to instead organize by logical feature/function. Most large-scale projects evolve to organize this way.