Add wxComboBox::Popup() and Dismiss() methods

[oneeyeman]

This ticket will add an ability to show the list box of the combo box.
It is useful when, for example, you want to show the choice editor in a grid.

When this will be approved I will send a patch for MSW/GTK/documentation.

Thank you.

[vadz]

Sorry, what exactly do you mean by this? Adding Popup() and Dismiss() methods to wxComboBox or something else?

[oneeyeman]

Hi, Vadim,
Yes, that's what I mean.

[vadz]

Yes, it would be nice to have this, TIA!

[oneeyeman]

Vadim,
I know it's not in "wx coding standards" to use boolean parameters.
However, I think this is the case where it should be broken.

I think I will do it like this:

wxComboBox::PopupDismiss(bool popup = true);

and I will impplement it as non-virtual in every port, as it doesn't make sense to override it.

Any objections?

Do you want this functionality to be present in the sample?

Thank you for supporting this and hopefully you apply it with same quick way.

[vadz]

Replying to oneeyeman:

I know it's not in "wx coding standards" to use boolean parameters.
However, I think this is the case where it should be broken.

Why? I see absolutely no good reason to do it like you suggest while we already have two different methods with these names in wxComboCtrl and so we should use them if only for consistency.

Do you want this functionality to be present in the sample?

Yes, it would be nice to show it in the combobox page of the widgets sample for example.

[oneeyeman]

Patch with implementation

[oneeyeman]

Patch attached.
However, the patch breaks compilation of the wxUniversal on Windows using VC(6?) It gives linker error on missing symbols, but the symbols are there.

Please test compilation before applying.

Thank you.

[vadz]

The reason your patch breaks linking of wxUniv (and all ports other than wxMSW and wxGTK, including wxOSX) is that you never provide the implementation of the functions in the base class. You must do it, please define them either as just empty or as wxFAIL_MSG("not implemented").

2 other minor comments:

1. Please repeat virtual when overriding a virtual function in the derived class for clarity.
2. Although public API is 2 functions (Popup() and Dismiss()), there is nothing wrong with implementing them both as a call to MSWDoPopup(bool) in wxMSW to avoid code duplication, of course.

Thanks!

[oneeyeman]

Replying to vadz:

The reason your patch breaks linking of wxUniv (and all ports other than wxMSW and wxGTK, including wxOSX) is that you never provide the implementation of the functions in the base class. You must do it, please define them either as just empty or as wxFAIL_MSG("not implemented").

Thanks, Vadim.

2 other minor comments:
1. Please repeat virtual when overriding a virtual function in the derived class for clarity.

One question though: does this gives an impression that this function can be overriden?
Maybe in this case it's easier to not use a base class/virtual functions at all?

2. Although public API is 2 functions (Popup() and Dismiss()), there is nothing wrong with implementing them both as a call to MSWDoPopup(bool) in wxMSW to avoid code duplication, of course.

OK, will do.

Thanks!

Thank you for the review.

[oneeyeman]

Patch (version 2)

[oneeyeman]

Patch (version 1)

[oneeyeman]

Vadim,

Replying to oneeyeman:

Replying to vadz:

The reason your patch breaks linking of wxUniv (and all ports other than wxMSW and wxGTK, including wxOSX) is that you never provide the implementation of the functions in the base class. You must do it, please define them either as just empty or as wxFAIL_MSG("not implemented").

Thanks, Vadim.

Fixed.

2 other minor comments:
1. Please repeat virtual when overriding a virtual function in the derived class for clarity.

One question though: does this gives an impression that this function can be overriden?
Maybe in this case it's easier to not use a base class/virtual functions at all?

Attached please find 2 implementation versions of the patch: version 1 it is implemented using virtual functions from the base class, and version 2 without using this approach.
I'd prefer version 2 (see my comment), but I will leave it up to you as you are main wx developer and you can decide which approach is better.

2. Although public API is 2 functions (Popup() and Dismiss()), there is nothing wrong with implementing them both as a call to MSWDoPopup(bool) in wxMSW to avoid code duplication, of course.

OK, will do.

Fixed.

Thanks!

Thank you for the review.

Hopefully this patch will be applied soon.

Thank you.

P.S.: What about adding the type: "easy" as discussed on wx-dev. I think this can be clarified as one of them...

[oneeyeman]

Also, it looks like the wxMac won't be able to provide this functionality, according to
 http://developer.apple.com/mac/library/documentation/Cocoa/Reference/ApplicationKit/Classes/NSComboBox_Class/Reference/Reference.html

unless I missed something.

Mac gurus, any comments?

Thank you.

[vadz]

Thanks for the patch and I'll commit it but I'd be really grateful if you could compile your patches before submitting them. I'd be even more grateful if you could test them a little too as the patch resulted in immediate crashes under MSW if you actually called these methods as I now do in the widgets sample.

[VZ]

(In [63242]) Add wxComboBox::Popup() and Dismiss() to manually show or hide its popup.

Add implementations for wxMSW and wxGTK.

Closes #11506.