Skip to content

Conversation

@SeraphimaZykova
Copy link
Contributor

Summary of the Pull Request

What is this about:

Added logger to the FZ Editor.
It also could help with issues such as #13105 to analyze repro steps.

What is include in the PR:

How does someone test / validate:

Quality Checklist

  • Linked issue: [FancyZones Editor] Add logger #13751
  • Communication: I've discussed this with core contributors in the issue.
  • Tests: Added/updated and all pass
  • Installer: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Docs: Added/ updated
  • Binaries: Any new files are added to WXS / YML

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.


var methodName = stackTrace.GetFrame(3)?.GetMethod();
var className = methodName?.DeclaringType.Name;
return className + " :: " + methodName?.Name;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "::" instead of " :: ".
[16:51:56.7245037] [Info] LayoutPreview::OnLoaded
looks better than
[16:51:56.7245037] [Info] LayoutPreview :: OnLoaded
IMHO

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, when searching logs, I would expect no space there

_eventHandle = new EventWaitHandle(false, EventResetMode.AutoReset, interop.Constants.FZEExitEvent());
if (_eventHandle.WaitOne())
{
Logger.LogInfo("FancyZones disabled, exit");
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct? I believe FZ is not disabled here? -> e.g "FanzyZones Editor exit event processed"

Copy link
Contributor

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

Looks good! This should help us a lot with reproducing bugs! Let's not forget to add more logging when we change/add some code or see that log could be useful at some place

Copy link
Contributor

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

Rubberstamp LGTM!
Using logger instead of manually is good.

@SeraphimaZykova SeraphimaZykova merged commit 91ed50d into microsoft:master Oct 25, 2021
@SeraphimaZykova SeraphimaZykova deleted the 13751-logger branch October 25, 2021 07:56
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

Successfully merging this pull request may close these issues.

3 participants