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

Make IRichTextBox public for Dependency Injection #18

Open
LazerFX opened this issue Sep 20, 2021 · 12 comments
Open

Make IRichTextBox public for Dependency Injection #18

LazerFX opened this issue Sep 20, 2021 · 12 comments
Labels
enhancement New feature or request help wanted Extra attention is needed up-for-grabs
Milestone

Comments

@LazerFX
Copy link

LazerFX commented Sep 20, 2021

I'm writing an application (Purely for myself, I don't intend to use this in production or anything), but I'd like to use RichTextBox sink - saves me having to have a console or spin up a file dropper or something like that every time I want to see logger output. However, the application is written in .NET 5 WPF, using Dependency Injection throughout. Serilog is registered through DI.

The issue is that I cannot configure the Logger after application start because it uses the UseSerilog extension on the IHostBuilder interface, but at that point the RichTextBox I want to target isn't instantiated.

My solution would be simple - instantiate an IRichTextBox interface that can be set up as a UseSingleton value. Then, I can inject that into the target Window, add the RichTextBox itself to it, and do a pass-through from the interface to the actual controls on the RichTextBox. I can't do this at the moment, as the IRichTextBox interface and the WriteTo.RichTextBox extension method that uses the IRichTextBox is internal sealed.

Before I go ahead and do this and put a PR together with the changes, I just thought I'd push this out for comment from the team working on this to see if anyone has any suggestions - maybe there's a better way of doing this?

@augustoproiete
Copy link
Member

Thanks @LazerFX for the suggestion and detailed scenario explanation.

Conceptually I think your idea makes sense - i.e. Provide a way to delay providing the RichTextBox control to the sink for scenarios where the logging pipeline is set up before the UI is available.

The IRichTextBox interface specifically is a very raw API that reflects a minimum set of methods needed to write to the control based on the current implementation (which will change for sure as we add more features such as writing paragraphs instead of inlines, for example), so I'd be hesitant to make that particular interface public as it is today.

I'm thinking a way forward could be to create a new factory-like interface that returns the RichBoxControl directly (instead of an abstraction), and adapt RichTextBoxImpl to use that instead of receiving the control instance directly.

public interface IRichTextBoxProvider
{
    System.Windows.Controls.RichTextBox GetRichTextBox();
}

This should allow for the scenario you described above... The only difference is the public API is more high level and less likely to change.

Would that work?


If Yes the next question is what to do with log messages that are written to the log during the start-up of the app, before the IRichTextBoxProvider has been activated with the instance of the control.

  • Are they lost forever and never making it into the RichBoxText control?

or

  • Are we buffering the log messages in-memory and then batch append to the RichBoxText control when it becomes available?

or

  • Something else (?)

@LazerFX
Copy link
Author

LazerFX commented Sep 20, 2021

Thanks for the quick response! I see what you mean - I can provide an IRichTextBoxProvider instantiation within the DI environment which then gets consumed by the sink... All I have to do is in the setup of the Window, take that provider and assign the real RichTextBox to whatever variable holds it within the DI container. That works...

For my scenario, I'm not fussed about the startup logging; however, I can understand that others would want those logged... I can see it getting tricky - if you've a nominally complex WPF app, you're going to have a massive volume of binding events logged if you've even slightly high levels of logging, and that will rapidly eat up cache space - you don't want people asking why suddenly there's a massive memory load during startup when they turn on logging to the RichTextBox... Perhaps allow it to be optionally caching? I mean - it's quite common to have multiple sinks hooked up to Serilog, and I can imagine this being used for specific message outputs that are use targeted - perhaps outputs of a specific type or from a few specific functions (That's what I'm doing). Just spitballing on what the use-cases could be, and whether it's possible to put a simple API to configure as many of those as possible :D

@augustoproiete
Copy link
Member

Cool. Agreed that the caching part needs some more thought.

Let's not get blocked by that then, we can start with a first version that has no buffering and discards the messages if the control is not available (i.e. whenever IRichTextBoxProvider.GetRichTextBox returns null) , and in a future version we can add buffering and toggles to opt-in or opt-out (depending on what gets decided as the default later).

Feel free to send a PR at your convenience.

@LazerFX
Copy link
Author

LazerFX commented Sep 22, 2021

Been looking through this - might take me a while to put a PR together for this; got pulled onto some things at work which means personal projects are slipped onto the back burner. Will close this issue as there's a plan if I want to go ahead on this, and say thanks for the support in deciding the best route :)

@LazerFX LazerFX closed this as completed Sep 22, 2021
@augustoproiete
Copy link
Member

No worries at all.

I think others might be interested in this feature as well, so let's leave the issue open as up-for-grabs in case anyone wants to take a stab at it in the short-term. Hacktoberfest is just around the corner and this one looks like low-hanging fruit.

@augustoproiete augustoproiete added enhancement New feature or request help wanted Extra attention is needed up-for-grabs labels Sep 22, 2021
@augustoproiete augustoproiete added this to the Future milestone Sep 22, 2021
@KayakFisher205
Copy link

I'm not a WPF programmer but I took the sample for the net core 5 winforms/WPF and incorporated it into an existing winform (csproj edited as with the code from WinFormsHostNet50Sample) that works with DI of Serliog. I was able to get the DI to the MainForm, from the program.cs. Here is what I did.

I made a static public reference for the RichTextBox and the ElementHost in program.cs.
I populated most of it in the Main().

I setup Serilog in the ConfigureServices(), along with using the Reference to the RIchTextBox and reading my other Serilog setting form my appsettings.json.

In the MainForm, where I had placed the _richTextBoxPanel (see sample code for WinFormsHostNet50Sample), after the InitializeComponent() line, I added

        _richTextBoxPanel.Controls.Add(Program.richTextBoxHost);

So the partial header of MainForm.cs is:
public partial class MainForm : Form
{

    private readonly Settings _settings;
    private readonly ILogger<MainForm> _logger;
    public BackgroundWorker bgw;

    public bool Developer;
    public int SleepFor;
    public bool DelayStart = false;
    public bool StartUp = true;

    public MainForm(IOptions<Settings> settings,  ILogger<MainForm> logger)
    {
        InitializeComponent();

        _richTextBoxPanel.Controls.Add(Program.richTextBoxHost);

}
}

I was able to use _logger.LogInformation from this point. All of the other classes have the ILogger Injection as Ilogger
I have attached my cleaned up program.cs file.

program.zip

@LazerFX
Copy link
Author

LazerFX commented Oct 6, 2021

If I'm understanding you right, you basically made a global static singleton RichTextBox reference that's outside any DI framework? That's not Dependency Injection, that's a global variable. Which, if you're looking for a testable, maintainable environment is... very much a no-no; though I might be misunderstanding you're demonstration here, and I'll be honest I've not looked at the code yet.

@augustoproiete
Copy link
Member

Thanks @KayakFisher205. Interesting hack, thanks for sharing! I will say though it's not exactly something I'd recommend as seems too early in the process for creating UI controls, but if it works for you.

@LazerFX That's it. The code sample holds a global static instance of a detached RichTextBox control that is created at the start and later shared with the view that will display it. That said, it would technically be possible to avoid using globals by registering the RichTextBox instance with the container, and having it as a dependency of the view (resolved by the DI container) - still a hack and not ideal, but "testable"

@KayakFisher205
Copy link

Agreed, I misspoke about DI. As you both said, it is definitely a hack and not good for production. Hopefully, it will be a catalyst for the really smart people to figure something out.

@frankhommers
Copy link

I would like this feature as well.

What also would be an option is to supply a Func to the WriteTo Extension method, so it can be resolved.

@augustoproiete
Copy link
Member

@frankhommers I'm not sure a Func would work in this scenario because at the time you set up the Serilog pipeline, you might not have access to the RichTextBox control yet (e.g. It has not yet been created).

Either way, feel free to send a PR at your convenience.

@thargy
Copy link

thargy commented Oct 23, 2022

If I'm understanding you right, you basically made a global static singleton RichTextBox reference that's outside any DI framework? That's not Dependency Injection, that's a global variable. Which, if you're looking for a testable, maintainable environment is... very much a no-no; though I might be misunderstanding you're demonstration here, and I'll be honest I've not looked at the code yet.

You can always add the RichTextBox as a 'named instance' to the service provider, that way the RTB will collect logs from the start, and you can retrieve the RTB in the constructor of the window/control you want to add it to. If named instances aren't supported in your DI framework, then you can create a wrapped object to hold the RTB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed up-for-grabs
Development

No branches or pull requests

5 participants