Skip to content

Temporarily fix VB App Framework Logging. - #7590

Merged
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging
Aug 15, 2022
Merged

Temporarily fix VB App Framework Logging.#7590
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging

Conversation

@KlausLoeffelmann

@KlausLoeffelmannKlausLoeffelmann commented Aug 14, 2022

Copy link
Copy Markdown
Member

The runtime PR dotnet/runtime#73087 broke the Visual Basic Application Framework Logging.

https://source.dot.net/#Microsoft.VisualBasic.Forms/Microsoft/VisualBasic/Logging/Log.vb,170

image

The original assumption that this would work was, that when GetSupportedAttributes gets called in a class derived from TraceSource that then would tell that the trace source was configured from a config file. This is not only what DefaultTraceSource does, it seems to be the only reason for it existence. Now, as far as I understand it, after the change, GetSupportAttributes gets called unconditionally, so HasBeenConfigured in the VB AppFramework's DefaultTraceSource returns always true, and so a FileLogTraceListener gets never added, since the Log class assumes, the trace source was configured by a config file. And that's why there will never be a value other than nothing for the DefaultFileLogWriter property.

After discussing this with the WinForms Team and the VB PM, we're fixing this temporary in the runtime, by returning false unconditionally for Logging.Log.HasBeenConfigured. This assumes, that the trace source has NOT been configured by a config file. Configuring by a config file remains a broken scenario for VB My.Logging, until we will be getting a fix from the Runtime.

Microsoft Reviewers: Open in CodeFlow

@ghostghost added the draft draft PR label Aug 15, 2022
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(ScaleTopLevelFormMinMaxSizeForDpiSwitchName, ref s_scaleTopLevelFormMinMaxSizeForDpi);
}
private const string AssumeVbLogClassWasConfiguredByConfigFileName = "System.Windows.Forms.AssumeVbLogClassWasConfiguredByConfigFile";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CS applications don't need this switch, why not put it into the VB assembly?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't say this is language related?
It's not that the Application Framework can only be used by VB. So, for example, take a look at this:

https://stackoverflow.com/questions/59268557/where-is-the-winforms-application-framework-in-c-sharp-that-exists-in-vb-net

Also, I often saw even C# WPF apps using a few features from the VB App Framework to ease their life. It's a valid approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Application that benefits from this quirk, references AppFramework, right? Why doesn't this quirk belong to Applicationframework then?

If _listenerAttributes Is Nothing Then
_listenerAttributes = Attributes
End If
Return _hasBeenInitializedFromConfigFile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would had returned false here for the RC and would have worked on the proper fix, i.e. how to get if this was initialized from the config file.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about this first. But then I thought, there is a good chance it will stay like this, and then we could have it right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a good chance it will stay like this,

Maybe runtime can add a protected property for us to look up?

@RussKie

Copy link
Copy Markdown
Contributor

I don't quite understand the use cases, developer and user experiences. It would be great if you could provide more details explaining those. E.g., this is how a .NET Framework app worked, now ported to .NET 7 it'd work like this. Would a developer or use need to opt-in or opt-out?
Thank you

@KlausLoeffelmann

Copy link
Copy Markdown
MemberAuthor

I pretty much explained the breaking change in the runtime which led to this.
I added a few lines of explanation in the introduction, what we did.
This needs to go in quick, to unblock us.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

I would prefer returning a false unconditionally because:
1 quirks are public APIs and it will be difficult to remove them when we have the fix
2. if an application enables logging in the config file, it has also to define a quirk in json file, this is inconvenient
3. we had not exhausted possibilities of a correct fix,

@KlausLoeffelmannKlausLoeffelmann changed the title Fix VB App Framework Logging.Temporarily fix VB App Framework Logging.Aug 15, 2022
@KlausLoeffelmann
KlausLoeffelmann marked this pull request as ready for review August 15, 2022 20:17
@KlausLoeffelmann
KlausLoeffelmann requested a review from a team as a code ownerAugust 15, 2022 20:17
@dreddy-work

dreddy-work commented Aug 15, 2022

Copy link
Copy Markdown
Member

Squash merge please. Or enable auto-merge (that is squash default)

@KlausLoeffelmann
KlausLoeffelmann enabled auto-merge (squash) August 15, 2022 20:32
@KlausLoeffelmann
KlausLoeffelmann merged commit 205f434 into dotnet:mainAug 15, 2022
@ghostghost added this to the 7.0 RC1 milestone Aug 15, 2022
@ghostghost removed the draft draft PR label Aug 16, 2022
End If
Return _hasBeenInitializedFromConfigFile

' TODO: This is a tempory fix, which will break configuring logging via file for the time being. See: https://github.com/dotnet/winforms/pull/7590

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you open a follow up bug to get this fixed?

@RussKie

Copy link
Copy Markdown
Contributor

@KlausLoeffelmann is this a breaking change that we need to document?

@merriemcgaw

Copy link
Copy Markdown
Contributor

@RussKie yeah, probably. It would be a minute number of customers impacted but it's good to document it of course.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

app config was not enabled in the previous versions, so this is a regression from .NET framework but not from the previous release of Core

@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2022
@KlausLoeffelmann
KlausLoeffelmann deleted the FixVbLogging branch May 26, 2026 19:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@KlausLoeffelmann@RussKie@Tanya-Solyanik@dreddy-work@merriemcgaw
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Temporarily fix VB App Framework Logging. by KlausLoeffelmann · Pull Request #7590 · dotnet/winforms · GitHub
Skip to content

Temporarily fix VB App Framework Logging. - #7590

Merged
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging
Aug 15, 2022
Merged

Temporarily fix VB App Framework Logging.#7590
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging

Conversation

@KlausLoeffelmann

@KlausLoeffelmannKlausLoeffelmann commented Aug 14, 2022

Copy link
Copy Markdown
Member

The runtime PR dotnet/runtime#73087 broke the Visual Basic Application Framework Logging.

https://source.dot.net/#Microsoft.VisualBasic.Forms/Microsoft/VisualBasic/Logging/Log.vb,170

image

The original assumption that this would work was, that when GetSupportedAttributes gets called in a class derived from TraceSource that then would tell that the trace source was configured from a config file. This is not only what DefaultTraceSource does, it seems to be the only reason for it existence. Now, as far as I understand it, after the change, GetSupportAttributes gets called unconditionally, so HasBeenConfigured in the VB AppFramework's DefaultTraceSource returns always true, and so a FileLogTraceListener gets never added, since the Log class assumes, the trace source was configured by a config file. And that's why there will never be a value other than nothing for the DefaultFileLogWriter property.

After discussing this with the WinForms Team and the VB PM, we're fixing this temporary in the runtime, by returning false unconditionally for Logging.Log.HasBeenConfigured. This assumes, that the trace source has NOT been configured by a config file. Configuring by a config file remains a broken scenario for VB My.Logging, until we will be getting a fix from the Runtime.

Microsoft Reviewers: Open in CodeFlow

@ghostghost added the draft draft PR label Aug 15, 2022
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(ScaleTopLevelFormMinMaxSizeForDpiSwitchName, ref s_scaleTopLevelFormMinMaxSizeForDpi);
}
private const string AssumeVbLogClassWasConfiguredByConfigFileName = "System.Windows.Forms.AssumeVbLogClassWasConfiguredByConfigFile";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CS applications don't need this switch, why not put it into the VB assembly?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't say this is language related?
It's not that the Application Framework can only be used by VB. So, for example, take a look at this:

https://stackoverflow.com/questions/59268557/where-is-the-winforms-application-framework-in-c-sharp-that-exists-in-vb-net

Also, I often saw even C# WPF apps using a few features from the VB App Framework to ease their life. It's a valid approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Application that benefits from this quirk, references AppFramework, right? Why doesn't this quirk belong to Applicationframework then?

If _listenerAttributes Is Nothing Then
_listenerAttributes = Attributes
End If
Return _hasBeenInitializedFromConfigFile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would had returned false here for the RC and would have worked on the proper fix, i.e. how to get if this was initialized from the config file.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about this first. But then I thought, there is a good chance it will stay like this, and then we could have it right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a good chance it will stay like this,

Maybe runtime can add a protected property for us to look up?

@RussKie

Copy link
Copy Markdown
Contributor

I don't quite understand the use cases, developer and user experiences. It would be great if you could provide more details explaining those. E.g., this is how a .NET Framework app worked, now ported to .NET 7 it'd work like this. Would a developer or use need to opt-in or opt-out?
Thank you

@KlausLoeffelmann

Copy link
Copy Markdown
MemberAuthor

I pretty much explained the breaking change in the runtime which led to this.
I added a few lines of explanation in the introduction, what we did.
This needs to go in quick, to unblock us.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

I would prefer returning a false unconditionally because:
1 quirks are public APIs and it will be difficult to remove them when we have the fix
2. if an application enables logging in the config file, it has also to define a quirk in json file, this is inconvenient
3. we had not exhausted possibilities of a correct fix,

@KlausLoeffelmannKlausLoeffelmann changed the title Fix VB App Framework Logging.Temporarily fix VB App Framework Logging.Aug 15, 2022
@KlausLoeffelmann
KlausLoeffelmann marked this pull request as ready for review August 15, 2022 20:17
@KlausLoeffelmann
KlausLoeffelmann requested a review from a team as a code ownerAugust 15, 2022 20:17
@dreddy-work

dreddy-work commented Aug 15, 2022

Copy link
Copy Markdown
Member

Squash merge please. Or enable auto-merge (that is squash default)

@KlausLoeffelmann
KlausLoeffelmann enabled auto-merge (squash) August 15, 2022 20:32
@KlausLoeffelmann
KlausLoeffelmann merged commit 205f434 into dotnet:mainAug 15, 2022
@ghostghost added this to the 7.0 RC1 milestone Aug 15, 2022
@ghostghost removed the draft draft PR label Aug 16, 2022
End If
Return _hasBeenInitializedFromConfigFile

' TODO: This is a tempory fix, which will break configuring logging via file for the time being. See: https://github.com/dotnet/winforms/pull/7590

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you open a follow up bug to get this fixed?

@RussKie

Copy link
Copy Markdown
Contributor

@KlausLoeffelmann is this a breaking change that we need to document?

@merriemcgaw

Copy link
Copy Markdown
Contributor

@RussKie yeah, probably. It would be a minute number of customers impacted but it's good to document it of course.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

app config was not enabled in the previous versions, so this is a regression from .NET framework but not from the previous release of Core

@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2022
@KlausLoeffelmann
KlausLoeffelmann deleted the FixVbLogging branch May 26, 2026 19:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@KlausLoeffelmann@RussKie@Tanya-Solyanik@dreddy-work@merriemcgaw
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Temporarily fix VB App Framework Logging. by KlausLoeffelmann · Pull Request #7590 · dotnet/winforms · GitHub
Skip to content

Temporarily fix VB App Framework Logging. - #7590

Merged
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging
Aug 15, 2022
Merged

Temporarily fix VB App Framework Logging.#7590
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging

Conversation

@KlausLoeffelmann

@KlausLoeffelmannKlausLoeffelmann commented Aug 14, 2022

Copy link
Copy Markdown
Member

The runtime PR dotnet/runtime#73087 broke the Visual Basic Application Framework Logging.

https://source.dot.net/#Microsoft.VisualBasic.Forms/Microsoft/VisualBasic/Logging/Log.vb,170

image

The original assumption that this would work was, that when GetSupportedAttributes gets called in a class derived from TraceSource that then would tell that the trace source was configured from a config file. This is not only what DefaultTraceSource does, it seems to be the only reason for it existence. Now, as far as I understand it, after the change, GetSupportAttributes gets called unconditionally, so HasBeenConfigured in the VB AppFramework's DefaultTraceSource returns always true, and so a FileLogTraceListener gets never added, since the Log class assumes, the trace source was configured by a config file. And that's why there will never be a value other than nothing for the DefaultFileLogWriter property.

After discussing this with the WinForms Team and the VB PM, we're fixing this temporary in the runtime, by returning false unconditionally for Logging.Log.HasBeenConfigured. This assumes, that the trace source has NOT been configured by a config file. Configuring by a config file remains a broken scenario for VB My.Logging, until we will be getting a fix from the Runtime.

Microsoft Reviewers: Open in CodeFlow

@ghostghost added the draft draft PR label Aug 15, 2022
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(ScaleTopLevelFormMinMaxSizeForDpiSwitchName, ref s_scaleTopLevelFormMinMaxSizeForDpi);
}
private const string AssumeVbLogClassWasConfiguredByConfigFileName = "System.Windows.Forms.AssumeVbLogClassWasConfiguredByConfigFile";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CS applications don't need this switch, why not put it into the VB assembly?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't say this is language related?
It's not that the Application Framework can only be used by VB. So, for example, take a look at this:

https://stackoverflow.com/questions/59268557/where-is-the-winforms-application-framework-in-c-sharp-that-exists-in-vb-net

Also, I often saw even C# WPF apps using a few features from the VB App Framework to ease their life. It's a valid approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Application that benefits from this quirk, references AppFramework, right? Why doesn't this quirk belong to Applicationframework then?

If _listenerAttributes Is Nothing Then
_listenerAttributes = Attributes
End If
Return _hasBeenInitializedFromConfigFile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would had returned false here for the RC and would have worked on the proper fix, i.e. how to get if this was initialized from the config file.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about this first. But then I thought, there is a good chance it will stay like this, and then we could have it right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a good chance it will stay like this,

Maybe runtime can add a protected property for us to look up?

@RussKie

Copy link
Copy Markdown
Contributor

I don't quite understand the use cases, developer and user experiences. It would be great if you could provide more details explaining those. E.g., this is how a .NET Framework app worked, now ported to .NET 7 it'd work like this. Would a developer or use need to opt-in or opt-out?
Thank you

@KlausLoeffelmann

Copy link
Copy Markdown
MemberAuthor

I pretty much explained the breaking change in the runtime which led to this.
I added a few lines of explanation in the introduction, what we did.
This needs to go in quick, to unblock us.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

I would prefer returning a false unconditionally because:
1 quirks are public APIs and it will be difficult to remove them when we have the fix
2. if an application enables logging in the config file, it has also to define a quirk in json file, this is inconvenient
3. we had not exhausted possibilities of a correct fix,

@KlausLoeffelmannKlausLoeffelmann changed the title Fix VB App Framework Logging.Temporarily fix VB App Framework Logging.Aug 15, 2022
@KlausLoeffelmann
KlausLoeffelmann marked this pull request as ready for review August 15, 2022 20:17
@KlausLoeffelmann
KlausLoeffelmann requested a review from a team as a code ownerAugust 15, 2022 20:17
@dreddy-work

dreddy-work commented Aug 15, 2022

Copy link
Copy Markdown
Member

Squash merge please. Or enable auto-merge (that is squash default)

@KlausLoeffelmann
KlausLoeffelmann enabled auto-merge (squash) August 15, 2022 20:32
@KlausLoeffelmann
KlausLoeffelmann merged commit 205f434 into dotnet:mainAug 15, 2022
@ghostghost added this to the 7.0 RC1 milestone Aug 15, 2022
@ghostghost removed the draft draft PR label Aug 16, 2022
End If
Return _hasBeenInitializedFromConfigFile

' TODO: This is a tempory fix, which will break configuring logging via file for the time being. See: https://github.com/dotnet/winforms/pull/7590

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you open a follow up bug to get this fixed?

@RussKie

Copy link
Copy Markdown
Contributor

@KlausLoeffelmann is this a breaking change that we need to document?

@merriemcgaw

Copy link
Copy Markdown
Contributor

@RussKie yeah, probably. It would be a minute number of customers impacted but it's good to document it of course.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

app config was not enabled in the previous versions, so this is a regression from .NET framework but not from the previous release of Core

@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2022
@KlausLoeffelmann
KlausLoeffelmann deleted the FixVbLogging branch May 26, 2026 19:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@KlausLoeffelmann@RussKie@Tanya-Solyanik@dreddy-work@merriemcgaw
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Temporarily fix VB App Framework Logging. by KlausLoeffelmann · Pull Request #7590 · dotnet/winforms · GitHub
Skip to content

Temporarily fix VB App Framework Logging. - #7590

Merged
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging
Aug 15, 2022
Merged

Temporarily fix VB App Framework Logging.#7590
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging

Conversation

@KlausLoeffelmann

@KlausLoeffelmannKlausLoeffelmann commented Aug 14, 2022

Copy link
Copy Markdown
Member

The runtime PR dotnet/runtime#73087 broke the Visual Basic Application Framework Logging.

https://source.dot.net/#Microsoft.VisualBasic.Forms/Microsoft/VisualBasic/Logging/Log.vb,170

image

The original assumption that this would work was, that when GetSupportedAttributes gets called in a class derived from TraceSource that then would tell that the trace source was configured from a config file. This is not only what DefaultTraceSource does, it seems to be the only reason for it existence. Now, as far as I understand it, after the change, GetSupportAttributes gets called unconditionally, so HasBeenConfigured in the VB AppFramework's DefaultTraceSource returns always true, and so a FileLogTraceListener gets never added, since the Log class assumes, the trace source was configured by a config file. And that's why there will never be a value other than nothing for the DefaultFileLogWriter property.

After discussing this with the WinForms Team and the VB PM, we're fixing this temporary in the runtime, by returning false unconditionally for Logging.Log.HasBeenConfigured. This assumes, that the trace source has NOT been configured by a config file. Configuring by a config file remains a broken scenario for VB My.Logging, until we will be getting a fix from the Runtime.

Microsoft Reviewers: Open in CodeFlow

@ghostghost added the draft draft PR label Aug 15, 2022
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(ScaleTopLevelFormMinMaxSizeForDpiSwitchName, ref s_scaleTopLevelFormMinMaxSizeForDpi);
}
private const string AssumeVbLogClassWasConfiguredByConfigFileName = "System.Windows.Forms.AssumeVbLogClassWasConfiguredByConfigFile";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CS applications don't need this switch, why not put it into the VB assembly?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't say this is language related?
It's not that the Application Framework can only be used by VB. So, for example, take a look at this:

https://stackoverflow.com/questions/59268557/where-is-the-winforms-application-framework-in-c-sharp-that-exists-in-vb-net

Also, I often saw even C# WPF apps using a few features from the VB App Framework to ease their life. It's a valid approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Application that benefits from this quirk, references AppFramework, right? Why doesn't this quirk belong to Applicationframework then?

If _listenerAttributes Is Nothing Then
_listenerAttributes = Attributes
End If
Return _hasBeenInitializedFromConfigFile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would had returned false here for the RC and would have worked on the proper fix, i.e. how to get if this was initialized from the config file.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about this first. But then I thought, there is a good chance it will stay like this, and then we could have it right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a good chance it will stay like this,

Maybe runtime can add a protected property for us to look up?

@RussKie

Copy link
Copy Markdown
Contributor

I don't quite understand the use cases, developer and user experiences. It would be great if you could provide more details explaining those. E.g., this is how a .NET Framework app worked, now ported to .NET 7 it'd work like this. Would a developer or use need to opt-in or opt-out?
Thank you

@KlausLoeffelmann

Copy link
Copy Markdown
MemberAuthor

I pretty much explained the breaking change in the runtime which led to this.
I added a few lines of explanation in the introduction, what we did.
This needs to go in quick, to unblock us.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

I would prefer returning a false unconditionally because:
1 quirks are public APIs and it will be difficult to remove them when we have the fix
2. if an application enables logging in the config file, it has also to define a quirk in json file, this is inconvenient
3. we had not exhausted possibilities of a correct fix,

@KlausLoeffelmannKlausLoeffelmann changed the title Fix VB App Framework Logging.Temporarily fix VB App Framework Logging.Aug 15, 2022
@KlausLoeffelmann
KlausLoeffelmann marked this pull request as ready for review August 15, 2022 20:17
@KlausLoeffelmann
KlausLoeffelmann requested a review from a team as a code ownerAugust 15, 2022 20:17
@dreddy-work

dreddy-work commented Aug 15, 2022

Copy link
Copy Markdown
Member

Squash merge please. Or enable auto-merge (that is squash default)

@KlausLoeffelmann
KlausLoeffelmann enabled auto-merge (squash) August 15, 2022 20:32
@KlausLoeffelmann
KlausLoeffelmann merged commit 205f434 into dotnet:mainAug 15, 2022
@ghostghost added this to the 7.0 RC1 milestone Aug 15, 2022
@ghostghost removed the draft draft PR label Aug 16, 2022
End If
Return _hasBeenInitializedFromConfigFile

' TODO: This is a tempory fix, which will break configuring logging via file for the time being. See: https://github.com/dotnet/winforms/pull/7590

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you open a follow up bug to get this fixed?

@RussKie

Copy link
Copy Markdown
Contributor

@KlausLoeffelmann is this a breaking change that we need to document?

@merriemcgaw

Copy link
Copy Markdown
Contributor

@RussKie yeah, probably. It would be a minute number of customers impacted but it's good to document it of course.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

app config was not enabled in the previous versions, so this is a regression from .NET framework but not from the previous release of Core

@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2022
@KlausLoeffelmann
KlausLoeffelmann deleted the FixVbLogging branch May 26, 2026 19:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@KlausLoeffelmann@RussKie@Tanya-Solyanik@dreddy-work@merriemcgaw
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Temporarily fix VB App Framework Logging. by KlausLoeffelmann · Pull Request #7590 · dotnet/winforms · GitHub
Skip to content

Temporarily fix VB App Framework Logging. - #7590

Merged
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging
Aug 15, 2022
Merged

Temporarily fix VB App Framework Logging.#7590
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging

Conversation

@KlausLoeffelmann

@KlausLoeffelmannKlausLoeffelmann commented Aug 14, 2022

Copy link
Copy Markdown
Member

The runtime PR dotnet/runtime#73087 broke the Visual Basic Application Framework Logging.

https://source.dot.net/#Microsoft.VisualBasic.Forms/Microsoft/VisualBasic/Logging/Log.vb,170

image

The original assumption that this would work was, that when GetSupportedAttributes gets called in a class derived from TraceSource that then would tell that the trace source was configured from a config file. This is not only what DefaultTraceSource does, it seems to be the only reason for it existence. Now, as far as I understand it, after the change, GetSupportAttributes gets called unconditionally, so HasBeenConfigured in the VB AppFramework's DefaultTraceSource returns always true, and so a FileLogTraceListener gets never added, since the Log class assumes, the trace source was configured by a config file. And that's why there will never be a value other than nothing for the DefaultFileLogWriter property.

After discussing this with the WinForms Team and the VB PM, we're fixing this temporary in the runtime, by returning false unconditionally for Logging.Log.HasBeenConfigured. This assumes, that the trace source has NOT been configured by a config file. Configuring by a config file remains a broken scenario for VB My.Logging, until we will be getting a fix from the Runtime.

Microsoft Reviewers: Open in CodeFlow

@ghostghost added the draft draft PR label Aug 15, 2022
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(ScaleTopLevelFormMinMaxSizeForDpiSwitchName, ref s_scaleTopLevelFormMinMaxSizeForDpi);
}
private const string AssumeVbLogClassWasConfiguredByConfigFileName = "System.Windows.Forms.AssumeVbLogClassWasConfiguredByConfigFile";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CS applications don't need this switch, why not put it into the VB assembly?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't say this is language related?
It's not that the Application Framework can only be used by VB. So, for example, take a look at this:

https://stackoverflow.com/questions/59268557/where-is-the-winforms-application-framework-in-c-sharp-that-exists-in-vb-net

Also, I often saw even C# WPF apps using a few features from the VB App Framework to ease their life. It's a valid approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Application that benefits from this quirk, references AppFramework, right? Why doesn't this quirk belong to Applicationframework then?

If _listenerAttributes Is Nothing Then
_listenerAttributes = Attributes
End If
Return _hasBeenInitializedFromConfigFile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would had returned false here for the RC and would have worked on the proper fix, i.e. how to get if this was initialized from the config file.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about this first. But then I thought, there is a good chance it will stay like this, and then we could have it right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a good chance it will stay like this,

Maybe runtime can add a protected property for us to look up?

@RussKie

Copy link
Copy Markdown
Contributor

I don't quite understand the use cases, developer and user experiences. It would be great if you could provide more details explaining those. E.g., this is how a .NET Framework app worked, now ported to .NET 7 it'd work like this. Would a developer or use need to opt-in or opt-out?
Thank you

@KlausLoeffelmann

Copy link
Copy Markdown
MemberAuthor

I pretty much explained the breaking change in the runtime which led to this.
I added a few lines of explanation in the introduction, what we did.
This needs to go in quick, to unblock us.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

I would prefer returning a false unconditionally because:
1 quirks are public APIs and it will be difficult to remove them when we have the fix
2. if an application enables logging in the config file, it has also to define a quirk in json file, this is inconvenient
3. we had not exhausted possibilities of a correct fix,

@KlausLoeffelmannKlausLoeffelmann changed the title Fix VB App Framework Logging.Temporarily fix VB App Framework Logging.Aug 15, 2022
@KlausLoeffelmann
KlausLoeffelmann marked this pull request as ready for review August 15, 2022 20:17
@KlausLoeffelmann
KlausLoeffelmann requested a review from a team as a code ownerAugust 15, 2022 20:17
@dreddy-work

dreddy-work commented Aug 15, 2022

Copy link
Copy Markdown
Member

Squash merge please. Or enable auto-merge (that is squash default)

@KlausLoeffelmann
KlausLoeffelmann enabled auto-merge (squash) August 15, 2022 20:32
@KlausLoeffelmann
KlausLoeffelmann merged commit 205f434 into dotnet:mainAug 15, 2022
@ghostghost added this to the 7.0 RC1 milestone Aug 15, 2022
@ghostghost removed the draft draft PR label Aug 16, 2022
End If
Return _hasBeenInitializedFromConfigFile

' TODO: This is a tempory fix, which will break configuring logging via file for the time being. See: https://github.com/dotnet/winforms/pull/7590

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you open a follow up bug to get this fixed?

@RussKie

Copy link
Copy Markdown
Contributor

@KlausLoeffelmann is this a breaking change that we need to document?

@merriemcgaw

Copy link
Copy Markdown
Contributor

@RussKie yeah, probably. It would be a minute number of customers impacted but it's good to document it of course.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

app config was not enabled in the previous versions, so this is a regression from .NET framework but not from the previous release of Core

@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2022
@KlausLoeffelmann
KlausLoeffelmann deleted the FixVbLogging branch May 26, 2026 19:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@KlausLoeffelmann@RussKie@Tanya-Solyanik@dreddy-work@merriemcgaw
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Temporarily fix VB App Framework Logging. by KlausLoeffelmann · Pull Request #7590 · dotnet/winforms · GitHub
Skip to content

Temporarily fix VB App Framework Logging. - #7590

Merged
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging
Aug 15, 2022
Merged

Temporarily fix VB App Framework Logging.#7590
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging

Conversation

@KlausLoeffelmann

@KlausLoeffelmannKlausLoeffelmann commented Aug 14, 2022

Copy link
Copy Markdown
Member

The runtime PR dotnet/runtime#73087 broke the Visual Basic Application Framework Logging.

https://source.dot.net/#Microsoft.VisualBasic.Forms/Microsoft/VisualBasic/Logging/Log.vb,170

image

The original assumption that this would work was, that when GetSupportedAttributes gets called in a class derived from TraceSource that then would tell that the trace source was configured from a config file. This is not only what DefaultTraceSource does, it seems to be the only reason for it existence. Now, as far as I understand it, after the change, GetSupportAttributes gets called unconditionally, so HasBeenConfigured in the VB AppFramework's DefaultTraceSource returns always true, and so a FileLogTraceListener gets never added, since the Log class assumes, the trace source was configured by a config file. And that's why there will never be a value other than nothing for the DefaultFileLogWriter property.

After discussing this with the WinForms Team and the VB PM, we're fixing this temporary in the runtime, by returning false unconditionally for Logging.Log.HasBeenConfigured. This assumes, that the trace source has NOT been configured by a config file. Configuring by a config file remains a broken scenario for VB My.Logging, until we will be getting a fix from the Runtime.

Microsoft Reviewers: Open in CodeFlow

@ghostghost added the draft draft PR label Aug 15, 2022
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(ScaleTopLevelFormMinMaxSizeForDpiSwitchName, ref s_scaleTopLevelFormMinMaxSizeForDpi);
}
private const string AssumeVbLogClassWasConfiguredByConfigFileName = "System.Windows.Forms.AssumeVbLogClassWasConfiguredByConfigFile";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CS applications don't need this switch, why not put it into the VB assembly?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't say this is language related?
It's not that the Application Framework can only be used by VB. So, for example, take a look at this:

https://stackoverflow.com/questions/59268557/where-is-the-winforms-application-framework-in-c-sharp-that-exists-in-vb-net

Also, I often saw even C# WPF apps using a few features from the VB App Framework to ease their life. It's a valid approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Application that benefits from this quirk, references AppFramework, right? Why doesn't this quirk belong to Applicationframework then?

If _listenerAttributes Is Nothing Then
_listenerAttributes = Attributes
End If
Return _hasBeenInitializedFromConfigFile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would had returned false here for the RC and would have worked on the proper fix, i.e. how to get if this was initialized from the config file.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about this first. But then I thought, there is a good chance it will stay like this, and then we could have it right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a good chance it will stay like this,

Maybe runtime can add a protected property for us to look up?

@RussKie

Copy link
Copy Markdown
Contributor

I don't quite understand the use cases, developer and user experiences. It would be great if you could provide more details explaining those. E.g., this is how a .NET Framework app worked, now ported to .NET 7 it'd work like this. Would a developer or use need to opt-in or opt-out?
Thank you

@KlausLoeffelmann

Copy link
Copy Markdown
MemberAuthor

I pretty much explained the breaking change in the runtime which led to this.
I added a few lines of explanation in the introduction, what we did.
This needs to go in quick, to unblock us.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

I would prefer returning a false unconditionally because:
1 quirks are public APIs and it will be difficult to remove them when we have the fix
2. if an application enables logging in the config file, it has also to define a quirk in json file, this is inconvenient
3. we had not exhausted possibilities of a correct fix,

@KlausLoeffelmannKlausLoeffelmann changed the title Fix VB App Framework Logging.Temporarily fix VB App Framework Logging.Aug 15, 2022
@KlausLoeffelmann
KlausLoeffelmann marked this pull request as ready for review August 15, 2022 20:17
@KlausLoeffelmann
KlausLoeffelmann requested a review from a team as a code ownerAugust 15, 2022 20:17
@dreddy-work

dreddy-work commented Aug 15, 2022

Copy link
Copy Markdown
Member

Squash merge please. Or enable auto-merge (that is squash default)

@KlausLoeffelmann
KlausLoeffelmann enabled auto-merge (squash) August 15, 2022 20:32
@KlausLoeffelmann
KlausLoeffelmann merged commit 205f434 into dotnet:mainAug 15, 2022
@ghostghost added this to the 7.0 RC1 milestone Aug 15, 2022
@ghostghost removed the draft draft PR label Aug 16, 2022
End If
Return _hasBeenInitializedFromConfigFile

' TODO: This is a tempory fix, which will break configuring logging via file for the time being. See: https://github.com/dotnet/winforms/pull/7590

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you open a follow up bug to get this fixed?

@RussKie

Copy link
Copy Markdown
Contributor

@KlausLoeffelmann is this a breaking change that we need to document?

@merriemcgaw

Copy link
Copy Markdown
Contributor

@RussKie yeah, probably. It would be a minute number of customers impacted but it's good to document it of course.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

app config was not enabled in the previous versions, so this is a regression from .NET framework but not from the previous release of Core

@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2022
@KlausLoeffelmann
KlausLoeffelmann deleted the FixVbLogging branch May 26, 2026 19:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@KlausLoeffelmann@RussKie@Tanya-Solyanik@dreddy-work@merriemcgaw
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Temporarily fix VB App Framework Logging. by KlausLoeffelmann · Pull Request #7590 · dotnet/winforms · GitHub
Skip to content

Temporarily fix VB App Framework Logging. - #7590

Merged
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging
Aug 15, 2022
Merged

Temporarily fix VB App Framework Logging.#7590
KlausLoeffelmann merged 14 commits into
dotnet:mainfrom
KlausLoeffelmann:FixVbLogging

Conversation

@KlausLoeffelmann

@KlausLoeffelmannKlausLoeffelmann commented Aug 14, 2022

Copy link
Copy Markdown
Member

The runtime PR dotnet/runtime#73087 broke the Visual Basic Application Framework Logging.

https://source.dot.net/#Microsoft.VisualBasic.Forms/Microsoft/VisualBasic/Logging/Log.vb,170

image

The original assumption that this would work was, that when GetSupportedAttributes gets called in a class derived from TraceSource that then would tell that the trace source was configured from a config file. This is not only what DefaultTraceSource does, it seems to be the only reason for it existence. Now, as far as I understand it, after the change, GetSupportAttributes gets called unconditionally, so HasBeenConfigured in the VB AppFramework's DefaultTraceSource returns always true, and so a FileLogTraceListener gets never added, since the Log class assumes, the trace source was configured by a config file. And that's why there will never be a value other than nothing for the DefaultFileLogWriter property.

After discussing this with the WinForms Team and the VB PM, we're fixing this temporary in the runtime, by returning false unconditionally for Logging.Log.HasBeenConfigured. This assumes, that the trace source has NOT been configured by a config file. Configuring by a config file remains a broken scenario for VB My.Logging, until we will be getting a fix from the Runtime.

Microsoft Reviewers: Open in CodeFlow

@ghostghost added the draft draft PR label Aug 15, 2022
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => GetCachedSwitchValue(ScaleTopLevelFormMinMaxSizeForDpiSwitchName, ref s_scaleTopLevelFormMinMaxSizeForDpi);
}
private const string AssumeVbLogClassWasConfiguredByConfigFileName = "System.Windows.Forms.AssumeVbLogClassWasConfiguredByConfigFile";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CS applications don't need this switch, why not put it into the VB assembly?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't say this is language related?
It's not that the Application Framework can only be used by VB. So, for example, take a look at this:

https://stackoverflow.com/questions/59268557/where-is-the-winforms-application-framework-in-c-sharp-that-exists-in-vb-net

Also, I often saw even C# WPF apps using a few features from the VB App Framework to ease their life. It's a valid approach!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Application that benefits from this quirk, references AppFramework, right? Why doesn't this quirk belong to Applicationframework then?

If _listenerAttributes Is Nothing Then
_listenerAttributes = Attributes
End If
Return _hasBeenInitializedFromConfigFile

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would had returned false here for the RC and would have worked on the proper fix, i.e. how to get if this was initialized from the config file.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I thought about this first. But then I thought, there is a good chance it will stay like this, and then we could have it right away.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a good chance it will stay like this,

Maybe runtime can add a protected property for us to look up?

@RussKie

Copy link
Copy Markdown
Contributor

I don't quite understand the use cases, developer and user experiences. It would be great if you could provide more details explaining those. E.g., this is how a .NET Framework app worked, now ported to .NET 7 it'd work like this. Would a developer or use need to opt-in or opt-out?
Thank you

@KlausLoeffelmann

Copy link
Copy Markdown
MemberAuthor

I pretty much explained the breaking change in the runtime which led to this.
I added a few lines of explanation in the introduction, what we did.
This needs to go in quick, to unblock us.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

I would prefer returning a false unconditionally because:
1 quirks are public APIs and it will be difficult to remove them when we have the fix
2. if an application enables logging in the config file, it has also to define a quirk in json file, this is inconvenient
3. we had not exhausted possibilities of a correct fix,

@KlausLoeffelmannKlausLoeffelmann changed the title Fix VB App Framework Logging.Temporarily fix VB App Framework Logging.Aug 15, 2022
@KlausLoeffelmann
KlausLoeffelmann marked this pull request as ready for review August 15, 2022 20:17
@KlausLoeffelmann
KlausLoeffelmann requested a review from a team as a code ownerAugust 15, 2022 20:17
@dreddy-work

dreddy-work commented Aug 15, 2022

Copy link
Copy Markdown
Member

Squash merge please. Or enable auto-merge (that is squash default)

@KlausLoeffelmann
KlausLoeffelmann enabled auto-merge (squash) August 15, 2022 20:32
@KlausLoeffelmann
KlausLoeffelmann merged commit 205f434 into dotnet:mainAug 15, 2022
@ghostghost added this to the 7.0 RC1 milestone Aug 15, 2022
@ghostghost removed the draft draft PR label Aug 16, 2022
End If
Return _hasBeenInitializedFromConfigFile

' TODO: This is a tempory fix, which will break configuring logging via file for the time being. See: https://github.com/dotnet/winforms/pull/7590

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you open a follow up bug to get this fixed?

@RussKie

Copy link
Copy Markdown
Contributor

@KlausLoeffelmann is this a breaking change that we need to document?

@merriemcgaw

Copy link
Copy Markdown
Contributor

@RussKie yeah, probably. It would be a minute number of customers impacted but it's good to document it of course.

@Tanya-Solyanik

Copy link
Copy Markdown
Contributor

app config was not enabled in the previous versions, so this is a regression from .NET framework but not from the previous release of Core

@ghostghost locked as resolved and limited conversation to collaborators Sep 17, 2022
@KlausLoeffelmann
KlausLoeffelmann deleted the FixVbLogging branch May 26, 2026 19:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@KlausLoeffelmann@RussKie@Tanya-Solyanik@dreddy-work@merriemcgaw