Krypton-Suite / Standard-Toolkit

An update to Component factory's krypton toolkit to support .NET Framework 4.6.2 - 4.8.1 to .NET 8 - 9
BSD 3-Clause "New" or "Revised" License
408 stars 62 forks source link

[Bug]: KryptonComboBox Enabled and DropDownStyle call order quirk #1697

Closed cuppm closed 2 months ago

cuppm commented 3 months ago

Describe the bug Probably not a huge deal as it's most likely never used this way by others, but if you can get an unusable KryptonComboBox control if you change both the Enabled status and DropDownStyle in the wrong order. It does not behave this way with a regular ComboBox control

To Reproduce Steps to reproduce the behavior:

  1. Create a new form and add a 'KryptonComboBox'
  2. Add a button with a handler that toggles the Enable value and DropDownStyle in different order (see code below)
  3. Run the form and click the toggle button to disable then enable.
  4. The KryptonComboBox that had the DropDownStyle property set before the Enabled property does not get fully re-enabled.

Code for the toggle button with 2 KryptonComboBox controls and 2 ComboBox controls:

private void btnToggleEnabled_Click(object sender, EventArgs e) {
    bool bEnabled = !kryptonComboBox1.Enabled;

    // Does not fully re-enables
    kryptonComboBox1.DropDownStyle = bEnabled ? ComboBoxStyle.DropDown : ComboBoxStyle.DropDownList;
    kryptonComboBox1.Enabled = bEnabled;

    // Fully re-enables
    kryptonComboBox2.Enabled = bEnabled;
    kryptonComboBox2.DropDownStyle = bEnabled ? ComboBoxStyle.DropDown : ComboBoxStyle.DropDownList;

    // Fully re-enables
    comboBox1.DropDownStyle = bEnabled ? ComboBoxStyle.DropDown : ComboBoxStyle.DropDownList;
    comboBox1.Enabled = bEnabled;

    // Fully re-enables
    comboBox2.Enabled = bEnabled;
    comboBox2.DropDownStyle = bEnabled ? ComboBoxStyle.DropDown : ComboBoxStyle.DropDownList;
}

Expected behavior The controls should return to their original state and not be disabled on the inner control (?).

Screenshots Initial loading of the form with items selected: image

After clicking the toggle button to set Enabled = false and DropDownStyle = DropDownStyle.DropDownList for each combobox: image

After clicking the toggle button again to set Enabled = true and DropDownStyle = DropDownStyle.DropDown for each combobox: image

Desktop (please complete the following information):

Additional context

Smurf-IV commented 3 months ago

@cuppm Can you add your repro project ("zipped") to this bug please

giduac commented 3 months ago

I did the provided instructions to recreate the toggle code and can confirm the error as described.

giduac commented 2 months ago

Hi @Smurf-IV,

I'd like you to have a look at this possible solution.

Context:

First option:

Second option:

        /// <summary>
        /// Gets and sets the appearance and functionality of the KryptonComboBox.
        /// </summary>
        [Category(@"Appearance")]
        [Description(@"Controls the appearance and functionality of the KryptonComboBox.")]
        [Editor(typeof(OverrideComboBoxStyleDropDownStyle), typeof(UITypeEditor))]
        [DefaultValue(ComboBoxStyle.DropDown)]
        [RefreshProperties(RefreshProperties.Repaint)]
        public ComboBoxStyle DropDownStyle
        {
            get => _comboBox.DropDownStyle;

            set
            {
                if (_comboBox.DropDownStyle != value)
                {
                    if (value == ComboBoxStyle.Simple)
                    {
                        throw new ArgumentOutOfRangeException(nameof(_comboBox.DropDownStyle), @"KryptonComboBox does not support the DropDownStyle.Simple style.");
                    }

                    if (Enabled)
                    {
                        MessageBox.Show($"DropDownStyle | Enabled: _comboBox.DropDownStyle = _deferredComboBoxStyle is null: {_deferredComboBoxStyle}");
                        _comboBox.DropDownStyle = _deferredComboBoxStyle is null
                            ? value
                            : _deferredComboBoxStyle.Value;

                        UpdateEditControl();
                    }
                    else
                    {
                        _deferredComboBoxStyle = value;
                        MessageBox.Show($"DropDownStyle | Disabled: _comboBox.DropDownStyle = _deferredComboBoxStyle is null: {_deferredComboBoxStyle}");
                    }
                }
            }
        }

        private ComboBoxStyle? _deferredComboBoxStyle;

        public new bool Enabled
        {
            get => base.Enabled;

            set
            {
                MessageBox.Show("Enabled called");
                if (base.Enabled != value)
                {
                    base.Enabled = value;

                    if (value)
                    {
                        if (_deferredComboBoxStyle is not null)
                        {
                            MessageBox.Show($"Enabled: DropDownStyle = _deferredComboBoxStyle.Value: {_deferredComboBoxStyle}");
                            DropDownStyle = _deferredComboBoxStyle.Value;
                            _deferredComboBoxStyle = null;
                        }
                    }
                }
            }
        }

Personally I favor option 1.

Your thoughts on this please....

Smurf-IV commented 2 months ago

Hmm. 1) is the simplest, but is more of "Not nice" solution, and will come back due to users not realising / remembering 2) is the way, but "does not make sense" until it is all documented in the code as to why, so that it is not accidentally removed.

Note, I would change if (_deferredComboBoxStyle is not null) to if (_deferredComboBoxStyle.HasValue)

giduac commented 2 months ago

@Smurf-IV

When going for 2. The getter can be adapted so it does reflect the changed state while disabled.

Changing the property while disabled will return the state when it became disabled. That will not be reflected in a property editor because of the caching....

Not sure how to go about this as it can cause questions... ??

Smurf-IV commented 2 months ago

Maybe something like this should solve

public ComboBoxStyle DropDownStyle
        {
            get => _deferredComboBoxStyle ?? _comboBox.DropDownStyle;

and then some simplification in the setter

giduac commented 2 months ago

@Smurf-IV Still needs testing in the designer cause that code will be active there possibly causing questions?

giduac commented 2 months ago

Hi @cuppm, How did you came across this flaw?

cuppm commented 2 months ago

This code was originally written with the commercial version of Krypton, so it's been a while since I've touched/thought about the code (so please forgive my memory fog on it). This is also a bit of a long story that may be completely irrelevant.

My application sets enabled/readonly state of several controls based on the model's state. It sets controls displaying the model properties to ideally readonly (looks normal but is not editable) on certain conditions and unsets on others. With Textbox controls this is easy (set ReadOnly = true). ComboBoxes don't have a readonly property. These ComboBoxes were being used as free text entry with the Items containing previous values used for easy access.

I originally tried implementing the method described in this Code Project article to add in read only functionality: http://www.codeproject.com/KB/vb/CompleteReadOnlyComboBox.aspx (which I got working with a standard WinForms ComboBox very well). But since the Krypton control contains an internally derived ComboBox class (InternalComboBox), I was unable to use this method. (Notes in my code: There is no way to intercept the commands that occur when a user selects an item from the drop down menu list if not directly inheriting from that control. Since the Krypton control contains the ComboBox object and does not inherit from it, there is no way to prevent the changes in this example's style. These cannot be intercepted with Win32 hooks either, only notified that they are going to occur.)

So to get the read only look when disabled, I was setting the KCBB to disabled, changing the DropDownStyle to DropDownList (after making sure the currently entered text was in the list of items), and changing StateDisabled to look Normal. All of this might be overwrought, but worked at the time for what I wanted.

The control would work as intended by re-enabling when the model state change specified it to be editable until a newer version of Krypton changed the underlying behavior. Then my user noted that those specific controls wouldn't be editable again when they unlocked the model.

giduac commented 2 months ago

@cuppm

Thanks for the response. Would the proposed change discussed above fit your scenario?

cuppm commented 2 months ago

🤷‍♂️ I think so? As long as it honors what the DropDownStyle is set to, I suppose it works.

In your example solution property setter code to the KCBB:

if (Enabled)
{
    MessageBox.Show($"DropDownStyle | Enabled: _comboBox.DropDownStyle = _deferredComboBoxStyle is null: {_deferredComboBoxStyle}");
    _comboBox.DropDownStyle = _deferredComboBoxStyle is null
        ? value
        : _deferredComboBoxStyle.Value;

    UpdateEditControl();
}

I did wonder if your deferring to the _deferredComboBoxStyle value over the value explicitly set by the user was what you meant. Wouldn't it default to the potentially old value instead of what's currently being set?

Maybe? (message boxes removed for brevity)

/// <summary>
/// Gets and sets the appearance and functionality of the KryptonComboBox.
/// </summary>
[Category(@"Appearance")]
[Description(@"Controls the appearance and functionality of the KryptonComboBox.")]
[Editor(typeof(OverrideComboBoxStyleDropDownStyle), typeof(UITypeEditor))]
[DefaultValue(ComboBoxStyle.DropDown)]
[RefreshProperties(RefreshProperties.Repaint)]
public ComboBoxStyle DropDownStyle
{
    get => _deferredComboBoxStyle ?? _comboBox.DropDownStyle;

    set
    {
        if (this.DropDownStyle != value)
        {
            if (value == ComboBoxStyle.Simple)
            {
                throw new ArgumentOutOfRangeException(nameof(_comboBox.DropDownStyle), @"KryptonComboBox does not support the DropDownStyle.Simple style.");
            }

            if (Enabled)
            {
                _comboBox.DropDownStyle = value;
                _deferredComboBoxStyle = null;

                UpdateEditControl();
            }
            else
            {
                _deferredComboBoxStyle = value;
            }
        }
    }
}

private ComboBoxStyle? _deferredComboBoxStyle;

public new bool Enabled
{
    get => base.Enabled;

    set
    {
        if (base.Enabled != value)
        {
            base.Enabled = value;

            if (value)
            {
                if (_deferredComboBoxStyle.HasValue)
                {
                    this.DropDownStyle = _deferredComboBoxStyle.Value;
                }
            }
        }
    }
}
giduac commented 2 months ago

@Smurf-IV

I got the fix done so far, this is what's been done:

Changes in the source are tagged with: // #1697 Work-around

KryptonComboBox.zip

giduac commented 2 months ago

This code was originally written with the commercial version of Krypton, so it's been a while since I've touched/thought about the code (so please forgive my memory fog on it). This is also a bit of a long story that may be completely irrelevant.

My application sets enabled/readonly state of several controls based on the model's state. It sets controls displaying the model properties to ideally readonly (looks normal but is not editable) on certain conditions and unsets on others. With Textbox controls this is easy (set ReadOnly = true). ComboBoxes don't have a readonly property. These ComboBoxes were being used as free text entry with the Items containing previous values used for easy access.

I originally tried implementing the method described in this Code Project article to add in read only functionality: http://www.codeproject.com/KB/vb/CompleteReadOnlyComboBox.aspx (which I got working with a standard WinForms ComboBox very well). But since the Krypton control contains an internally derived ComboBox class (InternalComboBox), I was unable to use this method. (Notes in my code: There is no way to intercept the commands that occur when a user selects an item from the drop down menu list if not directly inheriting from that control. Since the Krypton control contains the ComboBox object and does not inherit from it, there is no way to prevent the changes in this example's style. These cannot be intercepted with Win32 hooks either, only notified that they are going to occur.)

So to get the read only look when disabled, I was setting the KCBB to disabled, changing the DropDownStyle to DropDownList (after making sure the currently entered text was in the list of items), and changing StateDisabled to look Normal. All of this might be overwrought, but worked at the time for what I wanted.

The control would work as intended by re-enabling when the model state change specified it to be editable until a newer version of Krypton changed the underlying behavior. Then my user noted that those specific controls wouldn't be editable again when they unlocked the model.

@cuppm

On the side: The KCBB (and most of the K-Controls) expose the inner control as a property giving full access to the inner control including it's handle. That handle can be used to hook-up a NativeWindow.

I had a quick look at that VB code and this should be possible with the KCBB

Smurf-IV commented 2 months ago

KryptonComboBox.zip

Missing the following @ line 1633: _deferredComboBoxStyle = null; Must ensure that the value once used is reset to null, so that the next time HasValue is checked it is not used..

Having looked at the rest of changes, I think all of the edge cases that I came up with (In my head) have been covered ;-)

giduac commented 2 months ago

KryptonComboBox.zip

Missing the following @ line 1633: _deferredComboBoxStyle = null; Must ensure that the value once used is reset to null, so that the next time HasValue is checked it is not used..

Having looked at the rest of changes, I think all of the edge cases that I came up with (In my head) have been covered ;-)

We both were close... The reset only takes place when the control becomes enabled. So only needed in KCBB.OnEnabledChanged. The ternary in DropDownStyle is therefore redundant and DropDownStyle property becomes:

image

Tested all the cases that came up, also behaves solid in the designer. Nice and compact too. Pretty pleased with that...

giduac commented 2 months ago

V85 coming soon...