remove XML declaration line

[ArsenShnurkov]

fix for the bug
https://bugzilla.xamarin.com/show_bug.cgi?id=36388

full story gitextensions/gitextensions#2710

[xamarin-cla-bot]

Hey @ArsenShnurkov,
Thank you for your Pull Request! We <3 our contributors!

However, it looks like you haven't signed our CLA (Contributor License Agreement) yet. In order for us to accept your pull request, you have to sign our CLA first.
Once you do this, we can check over your pull request. You should only have to do this once.

You can read and sign our full Contributor License Agreement here.

Thanks,

Your friendly Xamarin CLA Bot#

[monojenkins]

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

[xamarin-cla-bot]

Hey @ArsenShnurkov,

Thanks for signing our CLA! We can now look at your pull request.

Always at your service,

Your friendly Xamarin CLA Bot#

[tritao]

Do you mind adding a test case for this change?

[ArsenShnurkov]

I don't have enough skills yet. Just recently I packaged the NUnit, and didn't learned it in depth yet. May be in 3-5 years i will do.

[tritao]

You can learn the basics of NUnit in a couple minutes, you definitely won't need years for that :)

The tests for the code you're changing should be located in https://github.com/mono/mono/blob/master/mcs%2Fclass%2FSystem%2FTest%2FSystem.Configuration%2FSettingsPropertyValueTest.cs.

You will need to add a new method (you can probably base it of one of the existing ones) and check the behaviour that you're changing in this pull request. In this specific case, you should be able to just use one the simple NUnit methods in the Assert class.

[ArsenShnurkov]

these tests looks strange for me, lines 81, 220

I am not sure why these values should contain XML declarations.

i would prefer peer review first

[akoeplinger]

@ArsenShnurkov I tried those tests on .NET and the xml header is there as well, so that's ok.

The test case from your bugzilla bug showed the problem on Mono for me and worked on .NET, which means we do indeed have a bug in Mono :)

I'd suggest adding your test case and making sure the existing tests still pass after your fix.

[ArsenShnurkov]

I don't understand, how to create the full save file in memory for test case (the sample from the bugtracker saves to default location on disk, and i don't see how to intercept that)

I need this, because of 2 observations:

• individual properties output XML declarations
• these declarations don't get into final file (as @akoeplinger confirmed)

so, i suspect there is some code in .Net which removes that declaration, and no such code in Mono. This is my hypothesis.

[alexrp]

build

[esdrubal]

While doing a test case for your pull request I noticed that .NET SettingsPropertyValue.SerializedValue returns XML with a XML declaration, you can see it by running the code below.

SettingsProperty p = new SettingsProperty ("property",
                                   typeof (WindowPositionList),
                                   null,
                                   true,
                                   10,
                                   SettingsSerializeAs.Xml,
                                   null,
                                   true,
                                   false);

SettingsPropertyValue v = new SettingsPropertyValue (p);
v.PropertyValue =  new WindowPositionList ();
Console.WriteLine(v.SerializedValue);

While poking into reference source code I found that they do remove the XML declaration from the XML returned by SettingsPropertyValue.SerializedValue in LocalFileSettingsProvider.

In Mono LocalFileSettingsProvider uses CustomizableFileSettingsProvider. I think this can be fixed by adding same logic as reference sources uses to remove XML declarations from the value returned here

[ArsenShnurkov]

also i don't understood 3 things:

• this check -

  mono/mcs/class/System/System.Configuration/CustomizableFileSettingsProvider.cs

  Lines 692 to 694 in 347f0a2

  	if (collection [element.Name] != null) {
  	if (collection [element.Name].Property.Attributes.Contains (typeof (SettingsManageabilityAttribute)) != isRoaming)
  	continue;

• and why it was removed into alternative branch of #if true directive (instead of copying into main branch)
• why header and rest of XML are separated with "\n" instead of Environment.NewLine:
  (see

  mono/mcs/class/System/Test/System.Configuration/SettingsPropertyValueTest.cs

  Line 220 in 347f0a2

  Assert.AreEqual ("<?xml version=\"1.0\" encoding=\"utf-16\"?>\n<int>10</int>", ((string)v.SerializedValue).Replace ("\r\n", "\n"), "A3");

  )

[esdrubal]

@ArsenShnurkov thanks for your contribution, #2400 contains your fix and test. I also added a message to your commit and fixed a style issue (spaces before parameters parenthesis).

#2400 removed the #if true else dead code, thanks for pointing this out.

It looks like the test you mentioned is expecting the serialized value to use '\n' instead of Environment.NewLine because the serializer is probably using '\n' regardless the value of Environment.NewLine that is "\r\n" in UNIX.