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

Make XmlnsDefinitionAttribute Public #2691

Closed
peter0302 opened this issue May 12, 2018 · 14 comments · Fixed by #2782

Comments

@peter0302
Copy link

commented May 12, 2018

Summary

Make XmlnsDefinitionAttribute Public

API Changes

Change XmlnsDefinitionAttribute accessibility to public.

Intended Use Case

Control library authors would be able to use their own custom namespace schema definitions (e.g. http://mycompany.com/xaml rather than having to specify the exact assembly name and CLR namespace in the xmlns definitions. It would permit using a single namespace prefix for all custom controls in a library package even if the controls are in different assemblies / namespaces.

@pauldipietro pauldipietro added this to New in Triage May 12, 2018

@dansiegel

This comment has been minimized.

Copy link
Contributor

commented May 12, 2018

This would have a lot of great uses including for Frameworks, Toolkits, really any library built around Forms

@peter0302

This comment has been minimized.

Copy link
Author

commented May 13, 2018

Interestingly it was originally public, and then changed to internal:
#557
I don't see any linked issue or explanation as to why it was changed. Not really sure what the harm would be in having it public.

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented May 14, 2018

It is internal because the parser only looks for those attributes in known assemblies. Making it public won't make it work, it requires some changes. Not impossible, not even hard...

@StephaneDelcroix StephaneDelcroix removed this from New in Triage May 14, 2018

@peter0302

This comment has been minimized.

Copy link
Author

commented May 14, 2018

Ah, that makes sense and also explains this comment!

//this could be extended to look for [XmlnsDefinition] in all assemblies

Would this be something that y'all would entertain a PR on?

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented May 14, 2018

PR is welcome

should be fixed in the XamlParser and in Xamarin.Forms.Build.Tasks/XmlTypeExtensions.cs for XamlC support

static void GatherXmlnsDefinitionAttributes()

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Now that I think about it, it was made internal, and only looking into known assemblies, so we're sure it doesn't collide with UWP's (and now WPF's) XmlnsDefinitionAttribute.

I don't think it's a problem, as we define our own XmlnsDefinitionAttribute (not part of netstandard2.0), but it's worth checking

@peter0302

This comment has been minimized.

Copy link
Author

commented May 14, 2018

Well I've made good progress but hit a stumbling block in XamlGenerator.cs at GetClrNamespace:
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Build.Tasks/XamlGenerator.cs

This gets called during the XAML build process to generate the code-behind .g.cs files.

The problem is that at this point the assembly doesn't exist yet, so I can't use reflection to find the assemblies referenced in order to look for XmlnsDefinitionAttribute and thus find the full qualified Type names. I'm unclear how, or if it's even possible, to do so at this point in the build process. The existing code is hard-wired to only look for http://xamarin.com/schemas/2014/forms and http://schemas.microsoft.com/winfx/2006/xaml.

I'm gonna check out the .NET Reference Source to see how WPF handles this but would love any suggestions. Thanks!

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@peter0302 oh, yes, there is that too... lemme think about it

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@peter0302 iirc, my conclusion at the time was that XamlG would need to be rebased to work on top of Roslyn to allow that.
option 2 is to pass all the project references, and all the files of the current project to the XamlG task.

I have an experimental branch around, doing Xaml-related work while having all the Roslyn context. I'll see if I can do something with it...

@peter0302

This comment has been minimized.

Copy link
Author

commented May 15, 2018

I'm making some progress on the MSBuild Task front. I finally figured out how to pass the @(ReferencePath) variable into the XamlG task in Xamarin.Forms.targets.. For some reason it refused to work without me first assigning @(ReferencePath) to a new variable in a PropertyGroup and then referencing the new variable when initializing XamlGTask. Meanwhile XamlCTask had no problem taking @(ReferencePath) directly... Weird.

Anyway, now that I have those reference paths I can look for the XmlnsDefinitionAttribute on the assemblies and use that in GetClrNamespace in XamlGenerator.

The only thing that bothers me is that there'll now be three different places where it'll be checking for the NS definitions: XamlGenerator, XamlParser, and XmlTypeExtensions. Trying to minimize code changes but probably a little refactoring will be needed to avoid this getting too messy.

@peter0302

This comment has been minimized.

Copy link
Author

commented May 16, 2018

Ok it's looking good! I have custom namespaces working with XamlParser, XamlG, and XamlC and all the unit tests pass. I should have a PR ready in a couple more days; I just want to test on VS for Mac first to make sure there are no gotchas with the MSBuild Task.

My work is here if anyone wants to take a look and comment first:

https://github.com/peter0302/Xamarin.Forms/tree/2691-xmlnsdefinitionattribute-public

@StephaneDelcroix did you need to mark this proposal as accepted first?

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented May 16, 2018

@StephaneDelcroix did you need to mark this proposal as accepted first?

no, that's fine

@peter0302 peter0302 referenced this issue May 17, 2018
4 of 4 tasks complete
@peter0302

This comment has been minimized.

Copy link
Author

commented May 18, 2018

@StephaneDelcroix just a quick update. Was finally able to test successfully in VS for Mac yesterday with iOS and MacOS platforms and that's working fine. Just cleaning up the commits in my own fork and should have a PR to you today or tomorrow.

All XAML unit tests are passing (and I added three directed to this issue) but not all UITests are, and I haven't been able to run the Windows UI unit tests at all. This is true in master even without my changes. Is this normal/expected? It looks like some of them are looking for network locations that don't exist so I'm wondering if they're out of date or just need to be run on your internal Network?

Also I don't think I can make a meaningful UI unit test for this because - correct me if I'm wrong - it looks like they all build their UI in C# code and not in XAML so there wouldn't be any point. I did add the issue to the control gallery though so it can be looked up and a demonstration viewed when you run it as an app. I also added XAML unit tests like I mentioned that test XamlG, XamlC, and XamlParser, respectively.

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented May 18, 2018

this doesn't require uitests. it can be all unittested

@legistek legistek referenced this issue May 19, 2018
4 of 4 tasks complete

@samhouts samhouts added this to In Progress in v3.6.0 May 21, 2018

@StephaneDelcroix StephaneDelcroix added this to Done in Enhancements via automation Nov 29, 2018

StephaneDelcroix added a commit that referenced this issue Nov 29, 2018
GH2691: Make XmlnsDefinitionAttribute public (#2782)
* Make XmlnsDefinitionAttribute public. Change XamlC, XamlG, and XamlParser to search for all referenced/available assemblies for attribute and referenced types. Add XAML unit tests and control gallery entry.

* change ns for xmlnsdefattribute

- fixes #2691

@samhouts samhouts moved this from In Progress to Done in v3.6.0 Nov 29, 2018

@samhouts samhouts removed this from Done in Enhancements Nov 29, 2018

@samhouts samhouts removed this from Done in v3.6.0 Jan 3, 2019

@samhouts samhouts added this to In Progress in v3.5.0 Jan 11, 2019

@samhouts samhouts moved this from In Progress to Done in v3.5.0 Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.