WindowsNotifications / NotificationsExtensions

Generate tile, toast, and badge notifications for Windows 10 via code, with the help of IntelliSense, instead of directly using XML.
MIT License
96 stars 21 forks source link

Consider XmlSerializer/DataContractSerializer + XSLT #8

Open Mike-E-angelo opened 8 years ago

Mike-E-angelo commented 8 years ago

It appears that this solution contains the same challenge addressed in https://github.com/ScottIsAFool/AdaptiveTileExtensions/pull/6

I am looking at this line: https://github.com/anbare/NotificationsExtensions/blob/master/Windows%2010/NotificationsExtensions/Common.cs#L383

And I am curious why this is being used, rather than:

var serializer = new XmlSerializer( GetType() );
serializer.Serialize( writer, this );

Because of this, there appears to be a tremendous amount of code overlap between the concerns addressed in XmlSerializer and the Xml namespace.

Would it be possible to consider a design where objects are emitted through XmlSerializer/DataContractSerializer, and then run through an XSLT like this one to emit the desired XML?

I would be interested in assisting with this. I cannot stand seeing DOM-tree walking when it can be taken care of with the lines above.

I realize this project supports different platforms, so I am looking to gain understanding here to see if there is something obvious I am missing. Thanks in advance. :)

andrewleader commented 8 years ago

I don't think NETFX Core supports XmlSerializer? So it wouldn't work in ASP.NET 5 projects. Otherwise yeah, I would have used that instead of manually doing it with XmlWriter.

Mike-E-angelo commented 8 years ago

Whalah presto change-o!

:)

Mike-E-angelo commented 8 years ago

FYI, the referenced assembly is here: C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETCore\v4.5\System.Xml.XmlSerializer.dll

andrewleader commented 8 years ago

Ah neat, maybe it works.

Switching to that might be expensive though. There are a few key things I need supported...

  1. Don't write properties that simply have the default value (we need to conserve space by not writing unnecessary properties, since the generated XML needs to be under 5 KB, which is actually pretty easy to exceed when you have all four tile sizes and an advanced tile like Weather)
  2. Avoid writing whitespace when possible (don't have newlines or tabs between elements, everything should be on one line, as minimal as possible) (but don't strip whitespace from the user's text attributes or values!)
  3. Work in all portable libraries, so that ASP.NET devs can use our library, UWP devs can, Win32 console devs can, people who have their own portable library to share code between their UWP and server, etc.
  4. Default property values need to be shared from the public facing API and the behind-the-scenes converter. By that I mean, for example, Element_TileText (the behind-the-scenes model) declares the default value of the text style. Then, TileText (the public-facing one) shares that default value. That ensures that if we have to change the default value, we don't have to change it in two places. The behind-the-scenes model that checks if the value matches the default value and decides whether to write the attribute or not is referencing the same value that the public-facing object is initialized with by default.

If you really want to make this change to switch to the XmlSerializer (or DataContractSerializer), feel free to submit a pull request. I don't have unit tests to check for preventing whitespace, but otherwise my unit tests cover ensuring that default values aren't written, and all those existing unit tests should pretty much ensure there aren't any regressions.

However, the current system works, and already supports the above points. I know I pretty much re-created exactly what the XmlSerializer does, but at the time I didn't think all the portable libraries supported it (I also didn't know anything about XSLT), and my way just seemed like the easiest way to achieve what I wanted. And now that it's written, I don't see a purpose for switching to XmlSerializer. Mine's already easy to use with the [attributes], does everything we need, currently doesn't have any bugs.

Mike-E-angelo commented 8 years ago

So just so you understand, this change gets rid of all coupling to XML, thereby simplifying it greatly. This would get rid of Element_* classes, as the *'s are the objects that get serialized into XML now (and would be the "source of record" for defaults), if that makes sense. So, Element_TileText goes away and there's just TileText, where the defaults are assigned.

As for the purpose of switching to XMLSerializer, the concern here is the "power of responsibility" that comes with a MSFT-ordained project. More specifically, it is with the guidance and best practices that are being presented here, as developers will no doubt copy and paste code from this project to use in their own. Obviously, by duplicating functionality from another accessible assembly that is built to do the same thing, the principle of DRY is being violated.

Additionally, with less code in your project, you decrease the surface area for maintenance and bugs. I know it was easier to do (ignorance is bliss!) but now you do have more unnecessary exposure in your codebase when a more mature (and tested in its own right) solution exists baked right in there and can do it for you.

I would also like to say that XSLT/XmlSerializer can address the issues you bring up above, no problem. However, with all of this said, we do have a problem with my ambitious plan here, as the XslProcessor does not exist from PCLs. :( :( :(

So, in order to do this, I will have to investigate further and provide perhaps an interface-based solution that will have to be used between different platforms.

andrewleader commented 8 years ago

Makes sense! If you figure out a way to have the XSLT work in all the portable libraries, then sounds like it should work!

The previous NotificationsExtensions library for Windows 8 generated the XML completely by string appending lol. I think my use of XmlWriter was at least better practice than that? :P

Mike-E-angelo commented 8 years ago

Yes, it is. ;) You're definitely meeting my pet peeve here. :P It's pretty amazing how much code you save with taking this route and even more amazing to see how many projects do not use it, LOL! I've obviously have seen more than my fair share of them, so when I do see it I make sure to point it out. I really can count on one hand the amount of times that XmlSerializer/DataContractSerializer have been used properly in projects; it's extremely rare.

andrewleader commented 8 years ago

Hahah. Yeah, I never knew about XSLT till recently, which without that, I don't think its possible to produce the exact XML required in this case. So that's also another reason why some people might not use the serializers, they don't know they can modify the XML with XSLT.

But for general web API's that take serialized data in XML format, I'd certainly hope everyone uses DataContractSerializer for that!!

Mike-E-angelo commented 8 years ago

I'd certainly hope everyone uses DataContractSerializer for that!!

You'd hope, but no one is safe. Poor serialization practices are a plague. Worse, a disease. Please do keep aware and mindful of this dark source, and save others from it and steer them to the light if you have the chance!!! ;P