fix: title empty if AddSegment is not called

[PiemP]

Fixes #14284

[hishamco]

Can you add a unit tests for this?

[PiemP]

Can you add a unit tests for this?

I try....pls be patient with me :D

[PiemP]

Test added... I could ask to you to give me a feeback about a discussion that I have open a month ago? I don't link it here because is something completely off topic.

Thank you

PS. Is something that seem simple...

[hishamco]

Thanks again @PiemP

[hishamco]

There's no fail in the build now

[Skrypt]

Make sure to not break the use case where someone wants to reset the page title by intentionally passing a string empty to RenderTitleSegments.

[PiemP]

Make sure to not break the use case where someone wants to reset the page title by intentionally passing a string empty to RenderTitleSegments.

What do you intend with reset the page title? I have look at RazorPage.cs and Page.cs in the first case passing the empty string cause the print of the _title property of the PageTitleBuilder, in the second case this cause the reset of the _title to null and the regeneration of the title from the _titleParts (but in this case we add a separator to the end of the string).

Would you like that I make RenderTitleSegments works in the same way when pass an empty string?

[jtkech]

See #14284 (comment)

[PiemP]

converted to draft see #14284 (comment)

[jtkech]

@PiemP So based on our discussion, see #14284 (comment), here my suggestion.

    private HtmlContentBuilder _title;
    ...
    ...

    public IHtmlContent GenerateTitle(IHtmlContent separator)
    {
        ...
        ...

        if (_title is { Count: > 0 })
        {
            return _title;
        }

        ...
        ...

[PiemP]

Have I to remove HtmlContentExtensions or exist some interest around it?

[jtkech]

Yes for now you can remove the extension and only use the mentioned changes.

Otherwise would need to be optimized, for example by using a ZStringWriter.

[PiemP]

For the tests purpose I prefer to maintain it if it's possible. Why exists 2 ZStringWriter? (one in OrchardCore.Abstractions.Pooling and one in Cysharp.Text)

[jtkech]

Maybe would need to be named .ToString().

But not sure it's good to encourage people to use it.

If you need it in the tests, why not just using a dedicated private method in the test class.

[sebastienros]

Can we replace this code:

https://github.com/OrchardCMS/OrchardCore/pull/14295/files#diff-11e828c0a0bb3d4f6e17cbd3033ee185913d21d718955e3cee3fee161b87fca4R61-R66

by

            if (_titleParts.Count == 0)
            {
                return HtmlString.Empty;
            }

            // _titleParts.Count * 2 because we add a separator for each entry
            var htmlContentBuilder = new HtmlContentBuilder(_titleParts.Count * 2);

[Piedone]

Is this something you'd like to revisit any time soon @PiemP? Or perhaps @hishamco you can finish the nits so we can merge this, since otherwise it's all approved?

[PiemP]

😅 ... maybe I have understand the changes requested by @hishamco ... sorry guys I'm pretty new to the things related to tests ... hope to have done all right.

[hishamco]

I might revise this one

[Piedone]

Great, thank you!