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

Refactor Avalonia.Controls #4957

Open
robloo opened this issue Oct 28, 2020 · 6 comments
Open

Refactor Avalonia.Controls #4957

robloo opened this issue Oct 28, 2020 · 6 comments
Labels
area-infrastructure Issues related to CI/tooling infrastructur enhancement

Comments

@robloo
Copy link
Contributor

robloo commented Oct 28, 2020

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.

  • Avalonia.Controls
    • Grid
      • Grid.cs
      • Grid.Properties.cs
      • Grid.Events.cs
      • ColumnDefinition.cs
      • RowDefinition.cs
    • ComboBox
      • ...
    • ...

Note the following:

  • Each control has its own directory
  • Each control is split apart into components using partial classes:
    • .Properties.cs contains all Direct/Styled properties as well as get/set accessors. Arrange alphabetically.
    • .Events.cs contains all events. Arrange alphabetically.
    • .cs contains all control logic
  • Separate data structures related to the control are also placed together. (ComboBoxItem, ColumnDefinition, etc.)

Open Questions:

  • Some controls share components. These, like primitives, should probably move to a "Common" directory. Edit: It might be best in this case to leave shared components in the Avalonia.Controls directory. Generally speaking directory organization should follow the namespace.
  • In other XAML frameworks .xaml is included in the directory as well. This is great and allows everything to be together. Avalonia seems to be doing the monolithic styling files like WPF did though. Would be great in my opinion to split these apart and just reference them from a monolithic generic.xaml file. Edit: I found the Themes directories which do already have files for each separate control. This totally makes sense to keep all themes together and then have different directories for different theme styles (Fluent, etc.). It's just a different way of organizing but no way is more correct here.

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
Copy link
Member

@grokys

@robloo
Copy link
Contributor Author

robloo commented Oct 29, 2020

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
Copy link
Member

grokys commented Nov 2, 2020

Hi @robloo, thanks for the compliment ;)

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

  • Each control has its own directory

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.

  • Each control is split apart into components using partial classes

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.

  • Separate data structures related to the control are also placed together.

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:

  • Large controls get their own directory
  • Smaller controls sit by themselves

@robloo
Copy link
Contributor Author

robloo commented Nov 2, 2020

@grokys Thanks for the feedback!

  • Each control has its own directory
  • Separate data structures related to the control are also placed together.

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
- Grid.cs
- Grid.Properties.cs : The Grid code base is quite ugly in comparison and finding properties is difficult.
- ColumnDefinition
- ColumnDefinitions
- RowDefinition
- RowDefinitions
- GridLength
- GridSplitter

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.

  • Each control is split apart into components using partial classes

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
Copy link
Contributor Author

robloo commented Mar 2, 2022

@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
Copy link
Contributor Author

robloo commented Mar 2, 2022

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.

@maxkatz6 maxkatz6 added enhancement area-infrastructure Issues related to CI/tooling infrastructur labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Issues related to CI/tooling infrastructur enhancement
Projects
None yet
Development

No branches or pull requests

4 participants