Output to Paragraphs or Inlines

[TonyValenti]

• Outputting to Paragraphs instead of inlines
• Auto Scrolling
• Auto Trimming

[CLAassistant]

All committers have signed the CLA.

[TonyValenti]

@augustoproiete - Would you please merge this? Also, please merge the commit that has the thread processing loop. If there are bugs in it, I will fix them.

[augustoproiete]

@TonyValenti Thanks for the PR! Could you split these into 3 separate PRs for each feature, and update the coding style to match the .editorconfig (eg. braces)

[TonyValenti]

@augustoproiete - I can update the style but I can't split out the changes into separate PRs. I introduce a new interface/class (IRichTextBoxOutputAppender) and it handles these features. Updated PR coming soon.

[TonyValenti]

@augustoproiete - How do I make VS respect the custom editor config?

[TonyValenti]

@augustoproiete - I just took a look and this is difficult because the current editorconfig is extremely lenient and doesn't really seem to do much. Can you please either:
a) accept the PR as is
or
b) update with the editorconfig of your choice and I'll reformat the code to match?

[augustoproiete]

@augustoproiete - I can update the style but I can't split out the changes into separate PRs. I introduce a new interface/class (IRichTextBoxOutputAppender) and it handles these features. Updated PR coming soon.

Send a base PR that has new the interface + the paragraph changes, I can get that reviewed & merged, and you can send the next two follow-up PRs rebased on top of those changes

[augustoproiete]

@augustoproiete - How do I make VS respect the custom editor config?

I'd expect Edit -> Advanced -> Format Document to do the job and fix the curly braces, etc...

[TonyValenti]

@augustoproiete - Please take a look at the code. The new interface incapsulates adding text to the text box. Two classes implement the interface: a legacy "inlines" provider that is the default and mimics old behavior and a new "paragraph" provider that adds paragraphs. It isnt' really possible to break the interface out from the impelmentations into separate commits because the one implementation is necessary in order for anything to work at all.

Also doing format document doesn't change anything.

Would you please review and merge?

[augustoproiete]

@TonyValenti To be clear, what I mean by 3 PRs is removing anything related to auto-scrolling and auto-trimming from this PR... Everything else stays which seems to be all the things you're mentioning above.

We can address these two other features in follow-up PRs.

Re: Format document, It's possible you have a non-default configuration on your machine (or ReSharper, or similar) getting in the way. Anyway, fix the curly brackets manually then, so they follow the C# convention and that should cover most of it

[TonyValenti]

@augustoproiete - I just added a commit that does the following:

1. I enhanced the editorconfig to apply newlines and braces in line with other code.
2. I reformatted my existing commit to match the style.
3. I merged the code for the thread processing loop in.

Regarding the request to split the PR into multiple, I think it would be helpful to explain as to why that isn't really feasible:
I refactored RichTextBoxImpl.Write to convert the text to XAML and then call into the specified RichTextBoxOutputAppender. The default OutputAppender is the InlinesOutputAppender which preserves the legacy behavior.

For example, here is the code for that appender:

    public record InlinesRichTextBoxOutputAppenderArgs : RichTextBoxOutputAppenderArgs
    {
        public bool ScrollOnChange { get; init; }
    }

public class InlinesRichTextBoxOutputAppender : RichTextBoxOutputAppenderBase<InlinesRichTextBoxOutputAppenderArgs>
    {
        public InlinesRichTextBoxOutputAppender(InlinesRichTextBoxOutputAppenderArgs args) : base(args)
        {

        }

        protected override void Append(System.Windows.Controls.RichTextBox richTextBox, FlowDocument document, Paragraph paragraph)
        {
            if (document.Blocks.LastBlock is Paragraph { } target)
            {
                var inlines = paragraph.Inlines.ToList();
                target.Inlines.AddRange(inlines);
            }
            else
            {
                document.Blocks.Add(paragraph);
            }

            if (Args.ScrollOnChange)
            {
                richTextBox.ScrollToEnd();
            }

        }
    }

You'll notice that the args have a flag that controlls whether it is scrolled or not.

The ParagraphRichTextBoxAppender follows a similar model and has args that control other things too:

    public record ParagraphRichTextBoxOutputAppenderArgs : RichTextBoxOutputAppenderArgs
    {
        public bool ScrollOnChange { get; init; }
        public bool Prepend { get; init; } //Add to top of textbox?
        public long? MaxItems { get; init; } //Auto-Trim paragraphs
    }

Would you please merge?

[TonyValenti]

@augustoproiete have you had a chance to review?

[augustoproiete]

Thanks @TonyValenti I had a chance to review the code and run some manual tests locally and overall the features you're trying to bring in are mostly working. There seems to be something going on with the first few events that are logged versus future events, as they don't seem to be added to paragraphs (see screenshot below):

I'd like to release a v2.0.0 of this Sink where writing to Paragraphs is the default (rather than the legacy) to address the performance issues you found, update the tests and sample projects, and include docs on how to style the RichTextBox to behave visually the way that legacy does today, and then after that think about bringing other features such as Prepending and how to do that.

For this v2.0.0 I'd like not to include Trimming, or Scrolling, or Prepending - these are separate features that can that can come later if it makes sense v2.1.0, etc. I'm not totally convinced AutoScrolling is a job of a Serilog Sink, for instance, seems like this should be a responsibility of the control. Also if xxxOutputAppender does something other than to Append then maybe it needs a better name - or perhaps the name is correct, but we're doing more than what a sink should be doing... 🤔

This branch needs to be rebased on top of develop (that now includes that background worker and other things, and there's a few minor formatting issues with the code (brackets, spacing, empty lines) that needs fixing. I tried to do it myself, but I don't have permission to write to your repo - You can give me permission or fix it yourself and push --force

remote: Permission to MediatedCommunications/serilog-sinks-richtextbox.git denied to augustoproiete.
fatal: unable to access 'https://github.com/MediatedCommunications/serilog-sinks-richtextbox.git/': The requested URL returned error: 403

[TonyValenti]

Hi @augustoproiete - Thanks for taking the time to review.

I've just pushed a commit of my code that makes a number of adjustments. Specifically, there were some major issues with the threading code from the previous author: namely, it would create a thread for each logger and then each thread would basically spin while waiting for data.

Also, RE trimming/scrolling:
We had previously experimented with scrolling being part of the control but it did not work out very well. The problem is that the controls basically just expose a "ContentChanged" event so even keypresses change the events.

Anyways, the code is there if you want to check it out. We're using this commit with a lot of intensity and it is working great now.

[thompcd]

I'm looking at adding this package in the next few days. This will be for a debug feature but will be in a production app. Is this going to get merged?

[TonyValenti]

You should definitely not use the current version until this gets merged. The current version leaks threads and uses spinning threads which will really hurt your performance and impact stability.

[Skaptor]

Is there a new update ?

[Skaptor]

@augustoproiete @TonyValenti ?

[TonyValenti]

I'm not sure what happened to @augustoproiete . He controls the repo and seems to have disappeared.

[Skaptor]

How can I help you @TonyValenti ?

[augustoproiete]

@Skaptor @TonyValenti I've outlined the issues with this PR in my comment above.

The best course of action from here is to tackle the different issues / feature in separate individual pull-requests that can be easily reviewed & tested, and likely released separately:

1. Improve async batching / background threading
2. Consider using multiple paragraphs instead of multiple inlines in a single paragraph
3. Add an option to auto-trim the RichTextBox contents (i.e. fixed number of lines or paragraphs)
4. Consider implementing auto-scrolling of the RichTextBox contents

Let me know if you are keen on getting those over the line.

I'm closing this PR as-is based on the above.

[jonmotos]

@augustoproiete, I am tackling breaking these up into multiple Pull Requests.
How best would you suggest that be handled?
My current plan is as follows, but I believe this creates a lot of dependent PRs (and maybe that's OK).

1. Rebase this PR on master
2. Implement Threading Improvements from this PR
3. (Build on 2) Add IRichTextBoxOutputAppender
4. (Build on 3) Add Appenders for NewLines or Paragraphs (defaulting to NewLine appendar)
5. Build on 4) Add support for ScrollToEnd
6. (Build on 4) Add support for Trimming

If necessary, I can break out change 3, and instead build 3 on 1 to segregate the two changes. They seem fairly disconnected, so I guess that's OK.

If that is OK, I will go ahead and submit PRs for each. Should be done today.

[augustoproiete]

Hey @JonAdamsFromNC thanks for your interest in reviving this one.

The memory leak issue is the most important one to solve - and release a new version as soon as it's tested & merged.

In terms of sequence, your suggestion seems fine - though it might be easier to skip step 1, branch from current master and manually bring the changes for the threading improvements and others - but I'll leave it to you to decide

Because of the dependencies between most of these PRs, you might want to wait for PR 1 to be merge before working on PR 2, etc. to simplify rebasing all the dependent PRs - but also up to you

ps: If you use the code in this PR, make sure you add @TonyValenti as co-author, so he gets credit as well.