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

Add tests to simulate issue 7739. #460

Closed
wants to merge 2 commits into from
Closed

Conversation

cpuwzd
Copy link
Contributor

@cpuwzd cpuwzd commented Oct 8, 2023

Generalize the issue to apply to most situations in which inner text is used in Wix 3 to represent condidtions. These all sufferfrom the mishandling of whitespace othere than the space character gets converted into an attribute value.

The XML parser generally treats all white space the same and reduces contiguous white space to a single space character.

All condition expressions are reduced to single line expressions. The user is free to reformat his conveerted code by inserting white space characters for the optimum readability readability.
The ne

src/wix/WixToolset.Converters/WixConverter.cs Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
@cpuwzd cpuwzd force-pushed the Issue7739 branch 2 times, most recently from 65625d6 to 37250e8 Compare October 15, 2023 19:38
@cpuwzd
Copy link
Contributor Author

cpuwzd commented Oct 15, 2023

Assuming that this pull request is still active, I'm ready for another review. This issue turned out to be considerably larger than first thought. The issue boils down to, "What do you want to know about the inner text?" The processing of inner text has been rethought in every scenario I could come up with. We could still use some more realistic test cases.

src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Outdated Show resolved Hide resolved
src/wix/WixToolset.Core/IOptimizer.cs Outdated Show resolved Hide resolved
Generalize the issue to apply to most situations in
which inner text is used in Wix 3 to represent condidtions. These all suffer from the mishandling of whitespace.

Review other uses of inner text amd pay attention
to the treatment of CTEXT nodes.
Stopped escaping single and double quotes, as well as
backslashes when converting inner text to attributes.

Eliminated all out parameters of TryGetInnerText in favor
of using a context parameter.
Copy link
Member

@robmen robmen left a comment

Choose a reason for hiding this comment

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

Sorry, I got really sick for the last couple weeks so I didn't get to the review until today.

src/wix/WixToolset.Converters/WixConverter.cs Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Show resolved Hide resolved
private class InnerTextContext
{
internal string Value { get; set; } = null;
internal List<XNode> comments = null;
Copy link
Member

Choose a reason for hiding this comment

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

I'd just initialize the Comments property and get rid of this private field. As soon as Comments is accessed it creates the list so there isn't a huge savings not creating it up front.

src/wix/WixToolset.Converters/WixConverter.cs Show resolved Hide resolved
src/wix/WixToolset.Converters/WixConverter.cs Show resolved Hide resolved
internal bool CoalesceWhitespace { get; set; } = true;
internal bool ReportWhitespace { get; set; } = false;

internal InnerTextContext()
Copy link
Member

Choose a reason for hiding this comment

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

Empty constructors are default and not needed. If you do need a constructor, it goes after the fields but before the property declarations.

@cpuwzd cpuwzd closed this Dec 1, 2023
@cpuwzd cpuwzd deleted the Issue7739 branch December 1, 2023 03:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants