wxScrolled<>: reduce public API (PR #26319)
Bill Su
Reduce the number of functions declared public by giving internal classes friend access
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26319
Commit Summary
- 47aab7d wxScrolled<>: reduce public API
File Changes
(1 file)
- M include/wx/scrolwin.h (28)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26319.patch
- https://github.com/wxWidgets/wxWidgets/pull/26319.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 left a comment (wxWidgets/wxWidgets#26319)Should this be merged or do you still plan more changes here?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
wsu-cb left a comment (wxWidgets/wxWidgets#26319)Should this be merged or do you still plan more changes here?
I am waiting for a review before declaring it ready.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26319)Ah, this is a catch-22 because I typically wait for a PR to exit the draft status before reviewing it (which is how it's supposed to work, I think, as implied by the "Ready for review" button label). Anyhow, I'll look at it in a moment, thanks.
—
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, this mostly looks good, but I think a couple of places may need to be changed.
> @@ -161,13 +167,16 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
// Forbid autoscrolling when the mouse is outside the window
void DisableAutoScrollOutside();
+private:
Minor, but I'd rather move this part to the end of the class declaration instead of having public/private/public scopes.
> @@ -196,9 +204,11 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
DoShowScrollbars(horz, vert);
}
+private:
I think IsScrollbarShown() should remain public, at least I don't see why would we want to hide it, it's a possibly useful function and it could be already used by the applications code.
> @@ -254,15 +264,19 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
return p2;
}
+private:
Again, minor, but I'd rather move them to a private section in the end.
> @@ -430,7 +448,8 @@ public: \
virtual void DoSetVirtualSize(int x, int y) override \
{ ScrollDoSetVirtualSize(x, y); } \
virtual wxSize GetBestVirtualSize() const override \
- { return ScrollGetBestVirtualSize(); }
+ { return ScrollGetBestVirtualSize(); } \
+private:
I'd rather not do this, hiding access change inside a macro is not very developer-friendly, it's better to put private: explicitly after calling it if necessary.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- 7372ba1 wxScrolled<>: rearrange public/protected/private sections
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
wsu-cb left a comment (wxWidgets/wxWidgets#26319)@vadz This is ready for a re-review.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26319)Thanks, will push soon.
—
Reply to this email directly, 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.