[wxWidgets/wxWidgets] wxGetSingleChoice*/wxGetSelectedChoices/wxGetTextFromUser: overloads that actually use position and size (PR #26688)
LukasBuricin
wxGetSingleChoiceIndex() takes x, y, width and height parameters, but they're silently ignored - the implementation marks them all WXUNUSED and the dialog is always shown at its default position and sizer-fitted size. The same is true of wxGetSingleChoice(), wxGetSingleChoiceData() and wxGetSelectedChoices(), and, to a lesser extent, wxGetTextFromUser(). My choice items were not fitting so I needed wxGetSingleChoiceIndex() that will fit them, but since the other functions had the same issue I added those too for completeness.
This isn't a new bug — it's been the documented behaviour since these wrapper functions were written (the docs already say as much: "create a wxSingleChoiceDialog object instead ... and change its size before showing it"). But since the underlying dialog classes (wxSingleChoiceDialog, wxMultiChoiceDialog, wxTextEntryDialog) already support being positioned and sized via their own Create()/ctor, it seemed worth closing the gap in the convenience wrappers rather than just working around it locally.
I've put together a small patch series (against master, targeting 3.3.4) that adds new overloads alongside the existing ones:
wxGetSingleChoice(), wxGetSingleChoiceIndex(), wxGetSingleChoiceData() and wxGetSelectedChoices() each gain an overload taking
(..., wxWindow* parent, const wxPoint& pos, const wxSize& size = wxDefaultSize, bool centre = true, ...)
that genuinely honours pos/size: the dialog is created at pos (unless centre is true), and grown — never shrunk below its sizer-fitted minimum — to at least size.
wxGetTextFromUser() gains an overload taking (..., const wxPoint& pos, int width = wxDefaultCoord, bool centre = true) — just a width, not a wxSize. wxTextEntryDialog already hard-clamps its max height to the fitted size for single-line entry
(SetMaxSize(wxSize(-1, minSize.GetHeight()))),
... so a height parameter would be a silent no-op; width is the only dimension that can actually have an effect here.
wxGetMultiLineTextFromUser() is a new function, the multiline counterpart to wxGetTextFromUser(). There was previously no convenience wrapper for multiline entry at all - wxTextEntryDialog itself has supported wxTE_MULTILINE for a long time, but using it meant constructing the dialog directly. This new function is a thin wrapper that sets wxTE_MULTILINE and, since both dimensions are meaningful for a multiline control, takes a full wxSize like the choice dialogs above.
Design notes, in case they're useful for review:
All new overloads keep parent and pos as required (non-defaulted) parameters. This is what avoids ambiguity with the legacy overloads: a call passing a wxPoint in that position can only ever resolve to the new overload, since wxPoint doesn't implicitly convert from the separate x/y wxCoord parameters the old ones use. Existing code calling these functions is completely unaffected.
The old overloads are untouched (still take x/y/width/height and ignore them) for backwards compatibility; nothing about their behaviour changes.
To avoid four near-identical copies of the same "grow the dialog to at least this size, re-centre if it actually grew" logic, I factored it out into two small private helpers in src/generic/choicdgg.cpp (GetChoiceDialogStyle() and SizeChoiceDialog()), and the existing wxGetSingleChoiceIndex() overload was refactored to use them too, for consistency.
samples/dialogs/dialogs.cpp has been updated to exercise the new overloads (SingleChoice, MultiChoice, LineEntry and TextEntry all now also call the free function with an explicit offset position and an oversized wxSize, alongside the existing class-based demos), so the fix is visible when running the sample, not just documented.
docs/changes.txt and the Doxygen docs in interface/wx/choicdlg.h and interface/wx/textdlg.h have been updated for all of the above.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26688
Commit Summary
- fc792e7 wxGet*Choice(s) overloads using position and size
- 7d56997 wxGet*TextFromUser with dimensions
File Changes
(8 files)
- M docs/changes.txt (11)
- M include/wx/generic/choicdgg.h (94)
- M include/wx/generic/textdlgg.h (28)
- M interface/wx/choicdlg.h (103)
- M interface/wx/textdlg.h (41)
- M samples/dialogs/dialogs.cpp (59)
- M src/common/utilscmn.cpp (54)
- M src/generic/choicdgg.cpp (178)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26688.patch
- https://github.com/wxWidgets/wxWidgets/pull/26688.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
LukasBuricin
@LukasBuricin pushed 1 commit.
- 214fdf4 Merge branch 'master' into choicedlg-pos-size
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26688)Thanks for the PR but I don't understand at all why do we need the new overloads — shouldn't we just make the existing ones, already taking x, y, width and height, work? Using wxPoint and wxSize is better, but doesn't justify adding a whole new slew of overloads, these functions already have too many of them.
Could you please redo the PR to avoid adding them? TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
LukasBuricin
LukasBuricin left a comment (wxWidgets/wxWidgets#26688)Thank you for your feedback @vadz !
Fixing the existing ones would have been my first choice, but given the WXUNUSED deliberately put in them, to me it seemed like intentional, perhaps for backwards compatibility, so I didn't want to touch them.
If you are one of the decision makers and are confident that changing the existing ones isn't going to get refused, then I am happy to proceed with the existing ones.
Please, can you confirm you are happy with getting rid of the WXUNUSED etc. ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26688)I don't think these parameters were ignored intentionally, this was simply never implemented and the original version from c801d85 (Initial revision, 1998-05-20) seems to have survived until now. So it should be fine to take these parameters into account.
However, looking at this closer I think we have a problem with wxCHOICE_{WIDTH,HEIGHT}: we shouldn't suddenly start using them because these values are completely inappropriate. Instead, you should replace them with wxDefaultCoord in all
int width = wxCHOICE_WIDTH, int height = wxCHOICE_HEIGHT,
blocks and also use wxDefaultCoord instead of using them explicitly in src/generic/choicdgg.cpp.
Finally, I also wonder why the dialog doesn't adapt to its contents automatically. I.e. when you write "My choice items were not fitting", do you mean horizontally or vertically? And, in either case, wouldn't it be better to make the dialog fit them (maybe in addition to your changes or maybe instead of them)?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
LukasBuricin
LukasBuricin left a comment (wxWidgets/wxWidgets#26688)They didn't fit horizontally - the text was cut on the right side. Happy to use wxDefaultCoord instead. The parent class wxAnyChoiceDialog::Create seems to be using only wxBoxSizer( wxVERTICAL ), which might be why the dialog doesn't resize horizontally itself.
Again, I am happy to change the existing implementations, once you are happy with the plan. Either the dialog should be using sizer for both directions or the WXUNUSED width should be used.
Please, let me know what your preferences are? Thanks a lot!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26688)They didn't fit horizontally - the text was cut on the right side. Happy to use wxDefaultCoord instead. The parent class wxAnyChoiceDialog::Create seems to be using only wxBoxSizer( wxVERTICAL ), which might be why the dialog doesn't resize horizontally itself.
Even vertical box sizer should be allocating enough space for its elements. It looks like there might be something wrong with the listbox best size calculation in the port you're using (which one is it BTW?).
Again, I am happy to change the existing implementations, once you are happy with the plan. Either the dialog should be using sizer for both directions or the WXUNUSED width should be used.
Using vertical box sizer is perfectly fine, this shouldn't and doesn't need to be changed.
The width should be used if it's explicitly specified, but not by default: by default, the dialog should have the minimal size required to show its contents.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.