Opened 4 years ago

Closed 4 years ago

#11506 closed enhancement (fixed)

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

Reported by: oneeyeman Owned by:
Priority: low Milestone:
Component: GUI-all Version: stable-latest
Keywords: wxComboBox list Cc:
Blocked By: Blocking:
Patch: yes

Description

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.

Attachments (3)

combobox_popupdismiss.patch download (3.7 KB) - added by oneeyeman 4 years ago.
Patch with implementation
combobox_popupdismiss_version2.patch download (3.3 KB) - added by oneeyeman 4 years ago.
Patch (version 2)
combobox_popupdismiss_version1.patch download (4.1 KB) - added by oneeyeman 4 years ago.
Patch (version 1)

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by vadz

  • Status changed from new to infoneeded_new

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

comment:2 Changed 4 years ago by oneeyeman

  • Status changed from infoneeded_new to new

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

comment:3 Changed 4 years ago by vadz

  • Milestone 3.0 deleted
  • Priority changed from normal to low
  • Status changed from new to confirmed
  • Summary changed from Ability to show the list box of the combo box to Add wxComboBox::Popup() and Dismiss() methods

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

comment:4 follow-up: Changed 4 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 4 years ago by 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.

Changed 4 years ago by oneeyeman

Patch with implementation

comment:6 Changed 4 years ago by 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.

comment:7 follow-up: Changed 4 years ago by 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!

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by 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?

  1. 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.

Changed 4 years ago by oneeyeman

Patch (version 2)

Changed 4 years ago by oneeyeman

Patch (version 1)

comment:9 in reply to: ↑ 8 Changed 4 years ago by 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.

  1. 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...

comment:10 Changed 4 years ago by 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.

comment:11 Changed 4 years ago by oneeyeman

  • Patch set

comment:12 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

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

Add implementations for wxMSW and wxGTK.

Closes #11506.

Note: See TracTickets for help on using tickets.