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

Support Layoutable properties #110

Closed
304NotModified opened this issue May 25, 2022 · 3 comments
Closed

Support Layoutable properties #110

304NotModified opened this issue May 25, 2022 · 3 comments
Milestone

Comments

@304NotModified
Copy link
Member

No description provided.

@304NotModified 304NotModified added this to the 5.0 milestone May 25, 2022
Fr33dan added a commit to Fr33dan/NLog.Windows.Forms that referenced this issue May 26, 2022
Make all properties layoutable as per issue NLog#110.
FormName and ControlName of RichTextBoxTarget are still strings, see above referenced issue for more information.
@Fr33dan
Copy link
Contributor

Fr33dan commented May 26, 2022

I wanted to do something productive this morning and thought I would give this a whack. ToolStripMenuItemTarget and MessageBoxTarget's already support them, FormControlTarget was trivial to update, but layoutable properties has some larger implications for the RichTextBoxTarget.

RichTextBoxTarget was designed with the presumption that each target would specify a single RichTextBox. If the control and form name are Layout's instead of fixed strings, there is the possibility that the layout could render differently each time the same target is used and the 1 to 1 contract of target to RichTextBox is broken.

The target can still be reworked to find/create the appropriate RichTextBox on each log call, but this is a significant change to functionality.

The object references to the target's form and RichTextBox would have to be removed, as well as the static functions to find RichTextBoxTarget from a RichTextBox. The tests rely on these and would have to be reworked and could impact any users who might be using these.

I'm inclined to say it's worth it to add the flexibility of layoutable properties, but given y'alls rigorous testing strategy, maintaining compatibility could be a higher priority.

304NotModified pushed a commit that referenced this issue May 29, 2022
* Support Layoutable Properties

Make all properties layoutable as per issue #110.
FormName and ControlName of RichTextBoxTarget are still strings, see above referenced issue for more information.

* Initialize layouts properly, add missed properties.

* Render only once for form retrieval.

* Render only once for form retrieval.

* Compare layouts as strings.

Proper comparisons would be better, but makes the test pass.

* Use `RenderLogEvent` instead of `Layout.Render` where possible

* Can't cache compiled regex since it could change with layout rendering.

Co-authored-by: JTignor <Joseph.Tignor@isotemp.com>
@304NotModified
Copy link
Member Author

The target can still be reworked to find/create the appropriate RichTextBox on each log call, but this is a significant change to functionality.

We could still use layouts, but only support them when doing a new initialize or something like that.

@304NotModified
Copy link
Member Author

I wanted to do something productive this morning and thought I would give this a whack. ToolStripMenuItemTarget and MessageBoxTarget's already support them, FormControlTarget was trivial to update, but layoutable properties has some larger implications for the RichTextBoxTarget.

Thanks for that!

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

No branches or pull requests

3 participants