Skip to content

Commit

Permalink
Remove TraceSwitch from CodeDomSerializer (dotnet#11062)
Browse files Browse the repository at this point in the history 
Tracing predates debugger trace points, which accomplish what we typically need to do when debugging.

Not having the tracing makes it easier to read the code, as well as easier to make complexity-reducing automated changes (inverting blocks, returning early, etc.).

Some of the affected code here is currently indented over 10 levels deep.
  • Loading branch information
@JeremyKuhne
JeremyKuhne authored Mar 18, 2024
1 parent 544df20 commit 0e35c66
Show file tree
Hide file tree
Showing 23 changed files with 2,245 additions and 3,164 deletions.
100 changes: 7 additions & 93 deletions ...sign/src/System/ComponentModel/Design/DesignerActionUI.DesignerActionToolStripDropDown.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ public void UpdateContainerSize()
Size panelSize = CurrentPanel.GetPreferredSize(new Size(150, int.MaxValue));
if (CurrentPanel.Size == panelSize)
{
// If the panel size didn't actually change, we still have to force a call to PerformLayout to make sure that controls get repositioned properly within the panel. The issue arises because we did a measure-only Layout that determined some sizes, and then we end up painting with those values even though there wasn't an actual Layout performed.
// If the panel size didn't actually change, we still have to force a call to PerformLayout to make
// sure that controls get repositioned properly within the panel. The issue arises because we did a
// measure-only Layout that determined some sizes, and then we end up painting with those values even
// though there wasn't an actual Layout performed.
CurrentPanel.PerformLayout();
}
else
Expand All @@ -50,18 +53,15 @@ public void UpdateContainerSize()
public void CheckFocusIsRight()
{
// fix to get the focus to NOT stay on ContainerControl
DropDownVisibilityDebug.TraceVerbose("Checking focus...");
HWND focusedControl = PInvoke.GetFocus();
if (focusedControl == Handle)
{
DropDownVisibilityDebug.TraceVerbose(" putting focus on the panel...");
_panel?.Focus();
}

focusedControl = PInvoke.GetFocus();
if (CurrentPanel is not null && CurrentPanel.Handle == focusedControl)
{
DropDownVisibilityDebug.TraceVerbose(" selecting next available control on the panel...");
CurrentPanel.SelectNextControl(null, true, true, true, true);
}
}
Expand All @@ -75,13 +75,10 @@ protected override void OnLayout(LayoutEventArgs levent)

protected override void OnClosing(ToolStripDropDownClosingEventArgs e)
{
DropDownVisibilityDebug.TraceVerbose($"_____________________________Begin OnClose {e.CloseReason}");
Debug.Indent();
if (e.CloseReason == ToolStripDropDownCloseReason.AppFocusChange && _cancelClose)
{
_cancelClose = false;
e.Cancel = true;
DropDownVisibilityDebug.TraceVerbose("cancel close prepopulated");
}

// When we get closing event as a result of an activation change, pre-populate e.Cancel based on why we're exiting.
Expand All @@ -93,37 +90,27 @@ protected override void OnClosing(ToolStripDropDownClosingEventArgs e)
if (Handle == hwndActivating && e.CloseReason == ToolStripDropDownCloseReason.AppClicked)
{
e.Cancel = false;
DropDownVisibilityDebug.TraceVerbose("[DesignerActionToolStripDropDown.OnClosing] activation hasnt changed, but we've certainly clicked somewhere else.");
}
else if (WindowOwnsWindow((HWND)Handle, hwndActivating))
{
// We're being de-activated for someone owned by the panel.
e.Cancel = true;
DropDownVisibilityDebug.TraceVerbose("[DesignerActionToolStripDropDown.OnClosing] Cancel close - the window activating is owned by this window");
}
else if (_mainParentWindow is not null && !WindowOwnsWindow((HWND)_mainParentWindow.Handle, hwndActivating))
{
if (IsWindowEnabled(_mainParentWindow.Handle))
{
// The activated windows is not a child/owned windows of the main top level windows let toolstripdropdown handle this
e.Cancel = false;
DropDownVisibilityDebug.TraceVerbose("[DesignerActionToolStripDropDown.OnClosing] Call close: the activated windows is not a child/owned windows of the main top level windows ");
}
else
{
e.Cancel = true;
DropDownVisibilityDebug.TraceVerbose("[DesignerActionToolStripDropDown.OnClosing] we're being deactivated by a foreign window, but the main window is not enabled - we should stay up");
}

base.OnClosing(e);
Debug.Unindent();
DropDownVisibilityDebug.TraceVerbose($"_____________________________End OnClose e.Cancel: {e.Cancel}");
return;
}
else
{
DropDownVisibilityDebug.TraceVerbose($"[DesignerActionToolStripDropDown.OnClosing] since the designer action panel dropdown doesnt own the activating window {hwndActivating.Value:x)}, calling close. ");
}

// What's the owner of the windows being activated?
HWND parent = (HWND)PInvoke.GetWindowLong(
Expand All @@ -133,16 +120,12 @@ protected override void OnClosing(ToolStripDropDownClosingEventArgs e)
// is it currently disabled (ie, the activating windows is in modal mode)
if (!IsWindowEnabled(parent))
{
DropDownVisibilityDebug.TraceVerbose("[DesignerActionToolStripDropDown.OnClosing] modal window activated - cancelling close");
// we are in a modal case
// We are in a modal case
e.Cancel = true;
}
}

DropDownVisibilityDebug.TraceVerbose($"[DesignerActionToolStripDropDown.OnClosing] calling base.OnClosing with e.Cancel: {e.Cancel}");
base.OnClosing(e);
Debug.Unindent();
DropDownVisibilityDebug.TraceVerbose($"_____________________________End OnClose e.Cancel: {e.Cancel}");
}

public void SetDesignerActionPanel(DesignerActionPanel panel, Glyph relatedGlyph)
Expand Down Expand Up @@ -199,7 +182,6 @@ private void PanelResized(object? sender, EventArgs e)

protected override void SetVisibleCore(bool visible)
{
DropDownVisibilityDebug.TraceVerbose($"[DesignerActionToolStripDropDown.SetVisibleCore] setting dropdown visible={visible}");
base.SetVisibleCore(visible);
if (visible)
{
Expand All @@ -214,87 +196,28 @@ protected override void SetVisibleCore(bool visible)
/// </summary>
private static bool WindowOwnsWindow(HWND hWndOwner, HWND hWndDescendant)
{
DropDownVisibilityDebug.TraceVerbose(
$"""
[WindowOwnsWindow] Testing if {hWndOwner.Value:x} is a owned by {hWndDescendant.Value:x}...
OWNER: {GetControlInformation(hWndOwner)}
OWNEE: {GetControlInformation(hWndDescendant)}
OWNEE's CLAIMED OWNER: {GetControlInformation((HWND)PInvoke.GetWindowLong(hWndDescendant, WINDOW_LONG_PTR_INDEX.GWL_HWNDPARENT))}
""");

if (hWndDescendant == hWndOwner)
{
DropDownVisibilityDebug.TraceVerbose("they match, YES.");
return true;
}

while (!hWndDescendant.IsNull)
{
hWndDescendant = (HWND)PInvoke.GetWindowLong(hWndDescendant, WINDOW_LONG_PTR_INDEX.GWL_HWNDPARENT);
if (hWndDescendant == IntPtr.Zero)
if (hWndDescendant.IsNull)
{
DropDownVisibilityDebug.TraceVerbose("NOPE.");
return false;
}

if (hWndDescendant == hWndOwner)
{
DropDownVisibilityDebug.TraceVerbose("YES.");
return true;
}
}

DropDownVisibilityDebug.TraceVerbose("NO.");
return false;
}

/// <summary>
/// Helper function for generating infomation about a particular control.
/// </summary>
internal static string GetControlInformation(HWND hwnd)
{
if (hwnd.IsNull)
{
return "Handle is null.";
}
#if DEBUG
if (!DropDownVisibilityDebug.TraceVerbose)
{
return string.Empty;
}

string windowText = PInvoke.GetWindowText(hwnd);
string typeOfControl = "Unknown";
string nameOfControl = string.Empty;
Control? c = FromHandle(hwnd);
if (c is not null)
{
typeOfControl = c.GetType().Name;
if (!string.IsNullOrEmpty(c.Name))
{
nameOfControl += c.Name;
}
else
{
nameOfControl += "Unknown";

// Some extra debug info for toolstripdropdowns.
if (c is ToolStripDropDown { OwnerItem: { } dropDownOwner })
{
nameOfControl += $"OwnerItem: [{dropDownOwner}]";
}
}
}

return $"""
{windowText}
Type: [{typeOfControl}] Name: [{nameOfControl}]
""";
#else
return string.Empty;
#endif
}

private bool IsWindowEnabled(IntPtr handle)
{
int style = (int)PInvoke.GetWindowLong(this, WINDOW_LONG_PTR_INDEX.GWL_STYLE);
Expand All @@ -306,16 +229,7 @@ private void WmActivate(ref Message m)
if ((nint)m.WParamInternal == PInvoke.WA_INACTIVE)
{
HWND hwndActivating = (HWND)m.LParamInternal;
if (WindowOwnsWindow((HWND)Handle, hwndActivating))
{
DropDownVisibilityDebug.TraceVerbose("[DesignerActionUI WmActivate] setting cancel close true because WindowsOwnWindow");
DropDownVisibilityDebug.TraceVerbose($"[DesignerActionUI WmActivate] checking the focus... {GetControlInformation(PInvoke.GetFocus())}");
_cancelClose = true;
}
else
{
_cancelClose = false;
}
_cancelClose = WindowOwnsWindow((HWND)Handle, hwndActivating);
}
else
{
Expand Down
41 changes: 19 additions & 22 deletions src/System.Windows.Forms.Design/src/System/ComponentModel/Design/DesignerActionUI.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ namespace System.ComponentModel.Design;
/// </summary>
internal partial class DesignerActionUI : IDisposable
{
private static readonly TraceSwitch s_designerActionPanelTraceSwitch = new("DesignerActionPanelTrace", "DesignerActionPanel tracing");

private Adorner _designerActionAdorner; // used to add designeraction-related glyphs
private IServiceProvider _serviceProvider; // standard service provider
private ISelectionService _selectionService; // used to determine if comps have selection or not
Expand All @@ -46,13 +44,10 @@ internal partial class DesignerActionUI : IDisposable
private bool _cancelClose;

private delegate void ActionChangedEventHandler(object sender, DesignerActionListsChangedEventArgs e);
#if DEBUG
internal static TraceSwitch DropDownVisibilityDebug { get; } = new("DropDownVisibilityDebug", "Debug ToolStrip Selection code");
#else
internal static TraceSwitch? DropDownVisibilityDebug { get; }
#endif

/// <summary>
/// Constructor that takes a service provider. This is needed to establish references to the BehaviorService and SelectionService, as well as spin-up the DesignerActionService.
/// Constructor that takes a service provider. This is needed to establish references to the BehaviorService
/// and SelectionService, as well as spin-up the DesignerActionService.
/// </summary>
public DesignerActionUI(IServiceProvider serviceProvider, Adorner containerAdorner)
{
Expand Down Expand Up @@ -307,18 +302,21 @@ private void RecreateInternal(IComponent comp)
if (glyph is not null)
{
VerifyGlyphIsInAdorner(glyph);
// this could happen when a verb change state or suddenly a control gets a new action in the panel and we are the primary selection in that case there would not be a glyph active in the adorner to be shown because we update that on selection change. We have to do that here too. Sad really...
RecreatePanel(glyph); // recreate the DAP itself
UpdateDAPLocation(comp, glyph); // reposition the thing

// This could happen when a verb change state or suddenly a control gets a new action in the panel and we
// are the primary selection in that case there would not be a glyph active in the adorner to be shown
// because we update that on selection change. We have to do that here too.

RecreatePanel(glyph);
UpdateDAPLocation(comp, glyph);
}
}

private void RecreatePanel(Glyph glyphWithPanelToRegen)
{
// we don't want to do anything if the panel is not visible
// We don't want to do anything if the panel is not visible.
if (!IsDesignerActionPanelVisible)
{
DropDownVisibilityDebug.TraceVerbose("[DesignerActionUI.RecreatePanel] panel is not visible, bail");
return;
}

Expand Down Expand Up @@ -562,33 +560,34 @@ private void ToolStripDropDown_Closing(object? sender, ToolStripDropDownClosingE
if (_cancelClose || e.Cancel)
{
e.Cancel = true;
DropDownVisibilityDebug.TraceVerbose("[DesignerActionUI.toolStripDropDown_Closing] cancelClose true, bail");
return;
}

if (e.CloseReason == ToolStripDropDownCloseReason.ItemClicked)
{
e.Cancel = true;
DropDownVisibilityDebug.TraceVerbose($"[DesignerActionUI.toolStripDropDown_Closing] ItemClicked: e.Cancel set to: {e.Cancel}");
}

if (e.CloseReason == ToolStripDropDownCloseReason.Keyboard)
{
e.Cancel = false;
DropDownVisibilityDebug.TraceVerbose($"[DesignerActionUI.toolStripDropDown_Closing] Keyboard: e.Cancel set to: {e.Cancel}");
}

if (e.Cancel == false)
{ // we WILL disappear
DropDownVisibilityDebug.TraceVerbose("[DesignerActionUI.toolStripDropDown_Closing] Closing...");
{
// We WILL disappear
Debug.Assert(_lastPanelComponent is not null, "last panel component should not be null here... " +
"(except if you're currently debugging VS where deactivation messages in the middle of the pump can mess up everything...)");

if (_lastPanelComponent is null)
{
return;
}

// if we're actually closing get the coordinate of the last message, the one causing us to close, is it within the glyph coordinate. if it is that mean that someone just clicked back from the panel, on VS, but ON THE GLYPH, that means that he actually wants to close it. The activation change is going to do that for us but we should NOT reopen right away because he clicked on the glyph... this code is here to prevent this...
// If we're actually closing get the coordinate of the last message, the one causing us to close, is it within
// the glyph coordinate. if it is that mean that someone just clicked back from the panel, on VS, but ON THE GLYPH,
// that means that he actually wants to close it. The activation change is going to do that for us but we should
// NOT reopen right away because he clicked on the glyph. This code is here to prevent this.
Point point = DesignerUtils.LastCursorPoint;
if (_componentToGlyph.TryGetValue(_lastPanelComponent, out DesignerActionGlyph? currentGlyph))
{
Expand Down Expand Up @@ -695,10 +694,8 @@ internal void ShowDesignerActionPanel(IComponent relatedComponent, DesignerActio
if (_behaviorService is not null &&
_behaviorService.AdornerWindowControl.DisplayRectangle.IntersectsWith(glyph.Bounds))
{
if (_mainParentWindow is not null && _mainParentWindow.Handle != IntPtr.Zero)
if (_mainParentWindow is not null && _mainParentWindow.Handle != 0)
{
Debug.WriteLineIf(s_designerActionPanelTraceSwitch.TraceVerbose, "Assigning owner to mainParentWindow");
DropDownVisibilityDebug.TraceVerbose("Assigning owner to mainParentWindow");
PInvoke.SetWindowLong(
_designerActionHost,
WINDOW_LONG_PTR_INDEX.GWL_HWNDPARENT,
Expand Down
Loading

0 comments on commit 0e35c66

Please sign in to comment.