Add wxStatusBar::AddFieldControl() API (PR #24092)
AliKet
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/24092
Commit Summary
- 549e951 Added wxStatusBar::AddFieldControl() member function
- 3896649 Update statbar sample to show AddFieldControl() in action
File Changes
(3 files)
- M include/wx/statusbr.h (18)
- M samples/statbar/statbar.cpp (18)
- M src/common/statbar.cpp (48)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/24092.patch
- https://github.com/wxWidgets/wxWidgets/pull/24092.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks! The API is exactly what I had in mind, so it would just need to be documented (in interface/wx/statusbr.h).
If the implementation can be simplified by storing wxWindow* (or maybe wxWindowRef?), I'd be for doing it, but if we have to use IDs for some reason that I'm missing, this is fine too.
Thanks again!
> @@ -97,6 +102,9 @@ class WXDLLIMPEXP_CORE wxStatusBarPane
// is the currently shown value shown with ellipsis in the status bar?
bool m_bEllipsized;
+
+ // remember the window that will be shown in this pane. Updated by SetFieldControl().
+ wxWindowID m_controlId = wxID_NONE;
Why not just store wxWindow* itself? Is there some benefit in storing the ID instead?
> @@ -173,6 +181,13 @@ class WXDLLIMPEXP_CORE wxStatusBarBase : public wxControl
wxSize GetBorders() const
{ return wxSize(GetBorderX(), GetBorderY()); }
+ // controls
+ // --------
+
+ // Add a control (child of the wxStatusBar) to be shown at the specified
+ // field position #n
I'd add that the control shouldn't be null -- unless it can be, to remove it?
In samples/statbar/statbar.cpp:
> wxRect rect;
- if (!GetFieldRect(Field_Checkbox, rect))
- {
- event.Skip();
- return;
- }
-
-#if wxUSE_CHECKBOX
- wxRect rectCheck = rect;
- rectCheck.Deflate(2);
This Deflate(2) seems to have disappeared, are you sure it's really not needed? It might be wxMSW-specific...
> +{
+ wxCHECK_MSG( (unsigned)n < m_panes.size(), false,
+ "invalid status bar field index" );
+ wxCHECK_MSG( !m_panes[n].IsFieldControl(), false,
+ "another control is already added in this field" );
+
+ m_panes[n].SetFieldControl(win);
+
+ return true;
+}
+
+void wxStatusBarBase::OnSize(wxSizeEvent& event)
+{
+ event.Skip();
+
+ auto GetFieldNumber = [this](wxWindow* win)
If we stored windows instead of IDs we could simplify this by just iterating over the panes, checking if each pane has a window and repositioning the window if so.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 3 commits.
- 1a1ce73 Added wxStatusBar::AddFieldControl() member function
- bbec687 Update statbar sample to show AddFieldControl() in action
- 4c93d12 Add documentation
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet commented on this pull request.
In samples/statbar/statbar.cpp:
> wxRect rect;
- if (!GetFieldRect(Field_Checkbox, rect))
- {
- event.Skip();
- return;
- }
-
-#if wxUSE_CHECKBOX
- wxRect rectCheck = rect;
- rectCheck.Deflate(2);
It's unfortunate that there is no comment telling us why this call is needed in the first place. but testing (with and without) this Deflate(2) I didn't see any difference at all under Windows 10 or Linux, except under Gnome I got a warning which can be silenced by just removing this call which I did here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@vadz As another remark on control positioning, this code:
https://github.com/wxWidgets/wxWidgets/blob/8cd33455febb5146437912cfe2d356684824a6db/samples/statbar/statbar.cpp#L987-L992
is supposed to keep m_statbmp centered in the pane when the status bar is resized. But the pane is set to be fixed-width here: https://github.com/wxWidgets/wxWidgets/blob/8cd33455febb5146437912cfe2d356684824a6db/samples/statbar/statbar.cpp#L928
which simply turn off what the code above is trying to demonstrate.
So we should either use a variable-width field for the m_statbmp or just remove the OnSize() handler entirely and rely on the default positioning which works correctly ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Great, thanks, I can merge it already but please let me know if you want to make more changes to the sample, e.g. make the bitmap pane growable.
> @@ -248,6 +248,22 @@ class wxStatusBar : public wxControl
*/
virtual void SetFieldsCount(int number = 1, const int* widths = nullptr);
+ /**
+ Add a control (child of the wxStatusBar) to be shown at the specified
+ field position in the status bar.
+
+ @param i
+ The field index where the control will be shown.
+ @param win
+ The control in question. Normally a child of the wxStatusBar itself.
+
+ @note You must delete the control to remove it from the status bar, as
+ simply passing @NULL will not do that.
+
+ @since 3.2.5
This can't be easily backported (it adds new fields), so I'd use
⬇️ Suggested change- @since 3.2.5 + @since 3.3.0
In samples/statbar/statbar.cpp:
> wxRect rect;
- if (!GetFieldRect(Field_Checkbox, rect))
- {
- event.Skip();
- return;
- }
-
-#if wxUSE_CHECKBOX
- wxRect rectCheck = rect;
- rectCheck.Deflate(2);
I'm pretty sure it was added because "it looked better" with it, but let's drop it if it's not needed any longer (in fact, I'd avoid changing the size of checkbox at all and just center it in the available space instead).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 834f1cb Update interface/wx/statusbr.h
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 86209cd Update statbar sample
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Looks like the unused variable in the sample is breaking some builds, but otherwise it's good, thanks!
> @@ -248,6 +248,22 @@ class wxStatusBar : public wxControl
*/
virtual void SetFieldsCount(int number = 1, const int* widths = nullptr);
+ /**
+ Add a control (child of the wxStatusBar) to be shown at the specified
+ field position in the status bar.
+
+ @param i
+ The field index where the control will be shown.
+ @param win
+ The control in question. Normally a child of the wxStatusBar itself.
I think it should be
⬇️ Suggested change- The control in question. Normally a child of the wxStatusBar itself. + The control in question. Must be a child of the wxStatusBar itself.
because the positioning loop doesn't do anything if it's not a child.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- 9c02c13 Remove unused variable after the last commit
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
AliKet
@AliKet pushed 1 commit.
- aaed2a5 Update interface/wx/statusbr.h
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.