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

Use ResXFileRef from this project #6

Merged
merged 11 commits into from
Dec 3, 2023

Conversation

bgrainger
Copy link
Contributor

Fixes #5

Detect the fully-qualified type name of the WinForms ResXFileRef type and substitute the one from this project instead.

@farlee2121
Copy link
Owner

Overall, I see the issue and I think I understand the solution.
My main concern is the broken compatibility with Visual Studio tooling.

I'm not quite sure how else we solve it though, seeing as we can't reference winforms from net standard...

@bgrainger
Copy link
Contributor Author

My main concern is the broken compatibility with Visual Studio tooling.

Can you explain what you mean here? I wanted this change to enable this library to be able to read a resx file (that was generated by Visual Studio) without requiring all the WinForms dependencies. (That is, to load the resx XML without requiring any change in the type name that was serialised in the XML.) In my testing, enumerating resources from a .NET Framework app produced the exact same key/value pairs as this library with the PR, so it seemed 100% compatible.

I notice that the tests are broken on non-Windows; I'm not sure why that is but can take a look.

@farlee2121
Copy link
Owner

Your change fixes the library's ability to read Resx files with file references.
But if we then write back to that file, visual studio can't edit that file with it's visual Resx editor anymore.

working vs resx editor

broken vs resx editor

The second image is using the file produced from this file roundtrip test. I adapted this from your test to make sure we could read files we write, considering the ResXFileRef namespace is changed when we resolve types.

[Fact]
public void ResxDataNode_ResXFileRef_RoundTrip()
{
    var referencedFileContent = Example.FileRef;
    
    var nodeInfo = new DataNodeInfo
    {
        Name = "Test",
        ReaderPosition = new Point(1, 2),
        TypeName = "System.Resources.ResXFileRef, System.Windows.Forms",
        ValueData = "TestResources/Files/FileRef.xml;System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089;utf-8",
    };
    var dataNode = new ResXDataNode(nodeInfo, null);
    var typeResolver = new AssemblyNamesTypeResolutionService(Array.Empty<AssemblyName>());
    
    string resxPath = @".\nya-test.resx";
    using (ResXResourceWriter writer = new ResXResourceWriter(resxPath))
    {
        writer.AddResource(dataNode);
    }
    using(ResXResourceReader reader = new ResXResourceReader(resxPath))
    {
        var dictionary = new Dictionary<object, object>();
        IDictionaryEnumerator dictionaryEnumerator = reader.GetEnumerator();
        while (dictionaryEnumerator.MoveNext())
        {
            dictionary.Add(dictionaryEnumerator.Key, dictionaryEnumerator.Value);
        }

        Assert.Equal(referencedFileContent, dictionary.GetValueOrDefault(nodeInfo.Name));
    }
}

@farlee2121
Copy link
Owner

Example.FileRef is essentially just a Resx getter for the Test.xml file you provided, that way we can compare the file contents without copying them to code

@farlee2121
Copy link
Owner

@bgrainger Are you still interested in collaborating on this?

@bgrainger
Copy link
Contributor Author

Yes, I am. Are you waiting for me to solve the "saving back to XML" problem?

@farlee2121
Copy link
Owner

I suppose I was waiting for some kind of feedback.
Solving the "saving back to XML" problem is something I think we should do before releasing.
I happy to collaborate, I just don't want to take over your contribution.

@bgrainger
Copy link
Contributor Author

Sorry for the radio silence.

I'm still interested in having this problem be solved. If you have a solution in mind and know the way forward to implement it, I'm happy for you to take it from here (and use as much or as little of my code as makes sense).

If you don't have the bandwidth to take it on, I'm happy to look at it more (maybe after American Thanksgiving).

Also move type data string to ResXConstants
@farlee2121
Copy link
Owner

Sounds good. I pull requested code for some my comments back to your branch.

@farlee2121
Copy link
Owner

Whenever you're back from vacation, I think I found a good solution to the write issue. I added it to the PR to your branch.

However, I also noticed that we'll run into issues if anyone overwrites the ITypeResolutionService. I'm not sure how likely that is...

I could use some help kicking the tires and poking holes in the test suite.

@bgrainger
Copy link
Contributor Author

I incorporated your changes and added a couple of fixes that I needed to get the tests working locally.

@farlee2121
Copy link
Owner

Weird, I thought I had replaced the test file with a StringWriter

@@ -956,6 +956,13 @@ public Type GetType(string name, bool throwOnError, bool ignoreCase)
return result;
}

// hard-code the assembly name for ResXFileRef and replace it with our own type instead of loading the one from System.Windows.Forms
if (name == "System.Resources.ResXFileRef, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")
Copy link
Owner

Choose a reason for hiding this comment

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

Could you move this string to ResXConstants?
It's nice to keep all the magic strings in one place.

Also, I noticed the example Resx I made with the VS editor didn't have this whole string as it's type

<data name="Test" type="System.Resources.ResXFileRef, System.Windows.Forms">
  <value>TestResources\Files\Test.xml;System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089;utf-8</value>
</data>

Perhaps we can match on a StartsWith System.Resources.ResXFileRef, System.Windows.Forms (but still keep the constant in ResXConstants

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. This also causes the namespace to change when we write the resx.

  1. We should probably make sure we can still read the data after we write the changed namespace
  2. This breaks compatibility with visual editors. Not sure what to do about that

Tests/ResxDataNodeTests.cs Show resolved Hide resolved
{
Name = "Test",
ReaderPosition = new Point(1, 2),
TypeName = "System.Resources.ResXFileRef, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089",
Copy link
Owner

Choose a reason for hiding this comment

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

This TypeName could also lean on the ResXConstants value once moved. Consolidates the special strings a bit.

Directory.Build.props Outdated Show resolved Hide resolved
}
}

Assert.Equal(originalResx, writerOutput.ToString().Replace("\r\n", "\n"));
Copy link
Owner

Choose a reason for hiding this comment

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

This breaks the test on my computer. We can't really write it to favor a particular platform line ending.

Maybe you could use something like ReplaceLineEndings.
We should be able to update the test project's target framework version if need be.

@farlee2121 farlee2121 merged commit bdad919 into farlee2121:main Dec 3, 2023
3 checks passed
@bgrainger bgrainger deleted the fix-resxfileref branch December 3, 2023 16:32
@farlee2121
Copy link
Owner

@bgrainger I just pushed out new package versions

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

Successfully merging this pull request may close these issues.

Can't load XML resource
2 participants