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

Please make Control.DesiredVisibility public #12641

Open
rickbrew opened this issue Dec 13, 2024 · 3 comments
Open

Please make Control.DesiredVisibility public #12641

rickbrew opened this issue Dec 13, 2024 · 3 comments
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation untriaged The team needs to look at this issue in the next triage

Comments

@rickbrew
Copy link

rickbrew commented Dec 13, 2024

Background and motivation

Same as #4989 but that was closed. cc @trivalik

I also think that having DesiredVisibility public would make many layout calculations a lot easier.

The setter for Visible specifies the desired visibility, but the getter provides you with the effective visibility. This isn't symmetric and is bad API design (not to shame the original devs, we've all learned a lot in the last 25 years!). There's currently no easy way, short of reflection or [UnsafeAccessor], to get this information unless you keep track of it yourself on the side. Subsequent MSFT UI frameworks such as WPF did the right thing with Visibility vs. IsVisible.

I have had some nasty, difficult to figure out bugs in the past where Control.Visible was incorrectly being assumed to mean "desired visibility" instead of "effective visibility." The code worked fine for years until some other capabilities were added -- in this case, adding the ability to put things into nested tabs (here are some examples of that new functionality, cc @BoltBait). A Control within a tab is not Visible unless the tab container is Visible and the containing tab is selected, and the layout calculations for the containing Form need to calculate the maximum layout height for all tabs so that the height of the tab container and the Form can be correctly determined. This needs to happen before the Form is shown so that it can be sized and positioned (centered) correctly. This was all very buggy, such as tabs being truncated or having scrollbars when they really shouldn't, until I realized that Visible was telling me the effective visibility, and my layout calculations were actually only taking the first tab into account.

These layout bugs obviously do not happen when a Control and its containers, including the Form, are visible, and this makes it very difficult to figure out why layout does not work correctly before showing the dialog or adding the containing Control. It's also difficult to reach the conclusion that these layout bugs are because of desired vs. effective visibility. Keeping track of this on the side adds a lot of complexity and a lot more potential for bugs that are tricky to debug, especially when adding new functionality (vs. the initial development of some UI). It's a major bug hazard for developers who do not (yet) know that Visible is not symmetric with respect to its getter and setter.

Here's another more recent example. I have a custom dialog for when an error or unhandled exception (crash) happens. It starts off in a non-expanded state, and the user can click "Show Details" to pop open the Details RichTextBox which has the exception and diagnostics information they can copy to the clipboard and post on the forum for help, or e-mail it to me, etc.

Image

Image

In my latest update, v5.1, there have been a lot of changes to how my app (Paint.NET) uses Direct2D/3D/DXGI/etc and this has resulted in hangs and crashes for some folks. It is almost always some other app that was injecting DLLs and hooking DirectX or causing other crashes or hangs for any number of other reasons (ever hear of Sentinel One, or Nahimic/A-Volute? 😬). In these cases there was no crash at all (because it was a hang), or no crash at a time when the error dialog (with diagnostics info) could be shown, so we had to advise people to use the portable release of the previous version of the app in order to go to Settings -> Diagnostics -> Copy to Clipboard and get this information. The diagnostics information includes the list of managed assemblies and native modules (DLLs) loaded in the process, and the presence of various DLLs is a very visible smoking gun towards what the user needs to do to fix the problem.

I'm now adding a /diagnostics command-line parameter so that this information can be retrieved without needing to boot up the app, which means hangs at startup shouldn't complicate troubleshooting as much. I want this error dialog w/ diagnostics info to open up pre-expanded, but the layout calculations use Control.Visible, so it ends up being centered incorrectly:

Image

This happens because I need to asynchronously simulate a click on the Show Details button in an event handler for Form.Shown. If I just set my IsDetailsTextVisible property to true then the resulting layout code does all sorts of wrong things because it uses Control.Visible for various purposes.

Fixing this is easy if I access the DesiredVisibility property via [UnsafeAccessor] and switch away from using Visible in this layout code. I can then do all the layout before the Form is visible which then results in it being centered correctly when it is shown. I initially tried to keep track of this info on the side but quickly realized I would have to spend a few hours debugging to get it to work, and to then re-test and debug all the scenarios that this dialog is used for. So for now I'm just using [UnsafeAccessor] because I do not want this functionality to soak up a full day or three of my time. I would much rather use a public property that doesn't have the risk of breaking because it got renamed, refactored, or removed.

API Proposal

namespace System.Windows.Forms;

public class Control
{
    public bool DesiredVisibility { get; } // (I don't actually care what the property is named)
}

API Usage

This will permit layout code to work before the Control and its containers and Form are visible, e.g.

public bool IsDetailsBoxVisible
{
    get => this.detailsTextBox.DesiredVisibility;

    set => this.detailsTextBox.Visible = true; // might also need a `PerformLayout()` call
}

protected override void OnLayout(LayoutEventArgs levent)
{
    if (this.detailsTextBox.DesiredVisibility)
    {
        ... set size, text, and visibility of related controls, including the size of the Form itself ...
    }
    
    base.OnLayout(levent);
}

Alternative Designs

No response

Risks

This functionality already exists, this proposal is just changing internal to public, so I'm not sure there's much risk. This property is already used internally by WinForms so I expect that it is working as expected.

Will this feature affect UI controls?

I expect this will mostly need some extra details in the documentation for Visible saying that you need to use DesiredVisibility to retrieve the same value you passed to set_Visible. Since it's a read only property I'm not sure the designer will be affected.

@rickbrew rickbrew added api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation untriaged The team needs to look at this issue in the next triage labels Dec 13, 2024
@paul1956
Copy link
Contributor

@rickbrew thanks for explaining why my app sometimes misbehaves.

I agree this would be a great addition.

@BoltBait
Copy link

I still wish the program's version number was in the title bar of that box:

Image

@rickbrew
Copy link
Author

For those interested in using Control.DesiredVisibility until we (hopefully!) have something official:

public static class ControlExtensions
{
    // Setting Control.Visible sets whether it is visible, or will be visible when its parent is actually visible. ("desired" visibility)
    // Getting Control.Visible gets whether the control is actually visible. ("effective" visibility)
    // Getting the desired visibility, which is the value that set_Visibility is given, is not publicly accessible.
    // https://github.com/dotnet/winforms/issues/12641
    public static bool GetDesiredVisibility(this Control control)
    {
        return ControlGetDesiredVisibility(control);
    }

    [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_DesiredVisibility")]
    private static extern bool ControlGetDesiredVisibility(Control self);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation untriaged The team needs to look at this issue in the next triage
Projects
None yet
Development

No branches or pull requests

3 participants