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

Merge core libraries. #5831

Merged
merged 36 commits into from
Apr 14, 2022
Merged

Merge core libraries. #5831

merged 36 commits into from
Apr 14, 2022

Conversation

grokys
Copy link
Member

@grokys grokys commented Apr 23, 2021

What does the pull request do?

We have too much separation of concerns in the Avalonia assembly layout, which has caused (and is causing) various problems with developing Avalonia. Some examples:

  • Hard to write features which animate classes in the Avalonia.Visuals assembly as that assembly was lower than Avalonia.Animation
  • Impossible to unify VisualChildren and LogicalChildren into a single collection as they're defined in different assemblies

This PR merges all assemblies below (but not including) Avalonia.Controls into Avalonia.Base.

Open questions:

  • Should we also put Avalonia.Markup in there?
  • Should we also put the base control classes (Control, TemplatedControl) in there (moving the actual controls to e.g. Avalonia.Controls.Standard maybe?)

Breaking changes

Pretty big ABI breaking change: a lot of classes have moved and assemblies have been deleted.

Everything below `Avalonia.Controls` into `Avalonia.Base`.
@robloo
Copy link
Contributor

robloo commented Apr 24, 2021

My two cents:

Should we also put Avalonia.Markup in there?

Markup makes sense to keep separate IMO. It's entirely possible within 10 years this layer needs to change independently anyway.

Should we also put the base control classes (Control, TemplatedControl) in there (moving the actual controls to e.g. Avalonia.Controls.Standard maybe?)

It seems to make sense to move base classes into the unified base project. Then these base classes would be available from all 'controls' projects. However, I wouldn't then call the controls project Avalonia.Controls.Standard it should remain Avalonia.Controls as the standard part is strongly implied. Any additional controls packages would then be Avalonia.Conrols.WinUI etc.

@maxkatz6
Copy link
Member

maxkatz6 commented Apr 24, 2021

Another question, do we have plans to change nuget packages? Right now we have couple of dlls merged into one "Avalonia" package. Do we want to drop that merging tool and publish simplified nuget packages?

@rstm-sf
Copy link
Contributor

rstm-sf commented Sep 2, 2021

Hi, what is the status of this PR and in what order will it be merged?

@maxkatz6
Copy link
Member

maxkatz6 commented Sep 2, 2021

@rstm-sf it was planned to be merged when we will be ready to with doing 0.11.0 branch with breaking changes instead of backporting everything to 0.10.x.

@kekekeks kekekeks mentioned this pull request Oct 1, 2021
13 tasks
@robloo
Copy link
Contributor

robloo commented Mar 22, 2022

Will this make Avalonia.Base internal members visible to Avalonia.Controls? Access to things such as KnownColors isn't possible otherwise. Internals need to be made available IMO. I already have found a few places it's needed/useful and haven't done much in the code base.

@grokys grokys mentioned this pull request Apr 13, 2022
3 tasks
@robloo
Copy link
Contributor

robloo commented Apr 13, 2022

I see lots of potential issues with a massive commit like this.

First, is there any way you can do it project-by-project first rather than all at once?

Secondly, this change should be done immediately or it's just going to get worse. I would also notify all developers of a hard-stop in submitting PRs until this is done.

Third, all existing PRs are going to need to be re-done. (This is a good reason why the PRs list shouldn't be allowed to get so long)

@grokys grokys changed the title WIP: Merge core libraries. Merge core libraries. Apr 14, 2022
@grokys grokys marked this pull request as ready for review April 14, 2022 09:51
@grokys
Copy link
Member Author

grokys commented Apr 14, 2022

Ok, this should be ready to go. Will be merged shortly.

@grokys
Copy link
Member Author

grokys commented Apr 14, 2022

I see lots of potential issues with a massive commit like this.

Definitely, which is why we've been holding off - it's potentially going to make backporting fixes a lot more difficult.

First, is there any way you can do it project-by-project first rather than all at once?

I'm not sure that would make it any less painful. The plan is to merge this as a squashed commit so that the painful changes are all in one place.

Secondly, this change should be done immediately or it's just going to get worse. I would also notify all developers of a hard-stop in submitting PRs until this is done.

Like I say, we've been holding off until towards the end of the end of the 0.10 maintenance period, but now we're coming up to 11.0 we're ready to merge it.

Third, all existing PRs are going to need to be re-done. (This is a good reason why the PRs list shouldn't be allowed to get so long)

Possibly, yes, though git does cope surprisingly well with these types of moves.

Will this make Avalonia.Base internal members visible to Avalonia.Controls? Access to things such as KnownColors isn't possible otherwise. Internals need to be made available IMO. I already have found a few places it's needed/useful and haven't done much in the code base.

Avalonia.Base's internals are made visible to Avalonia.Controls, yes.

@@ -5,5 +5,6 @@
<!-- https://github.com/dotnet/msbuild/issues/2661 -->
<AddSyntheticProjectReferencesForSolutionDependencies>false</AddSyntheticProjectReferencesForSolutionDependencies>
<MSBuildEnableWorkloadResolver>false</MSBuildEnableWorkloadResolver>
<RunApiCompat>False</RunApiCompat>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR currently disables ApiCompat. This is because even though git handles merges with file moves pretty well, it does not handle changes to ApiCompatBaseline.txt well, and these files are going to be the main cause of merge conflicts after this change.

Should we leave this disabled until 11.0 (because breaking changes are expected at this point)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the only issue I can think is its harder to make a breaking change summary for the release.

grokys added 5 commits April 14, 2022 12:55
Now that more unit tests are present in Avalonia.Base.UnitTests, general `AvaloniaObject` properties are getting registered. Ignore those.
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0019913-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

Copy link
Collaborator

@MarchingCube MarchingCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged!

@grokys grokys merged commit fb0da85 into master Apr 14, 2022
@grokys grokys deleted the refactor/merge-core-assemblies branch April 14, 2022 12:39
@grokys
Copy link
Member Author

grokys commented Apr 14, 2022

Anyone: if you're annoyed that your PR now has merge conflicts, ping me and I'll fix it ;)

@Mrxx99
Copy link
Contributor

Mrxx99 commented Sep 18, 2022

I think this does not have to be a breaking change when using the TypeForwardedTo Attribute, which is used very often in dotnet runtime itself for exactly that purpose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants