make wxScrolled<> autoscroll region configurable (PR #25978)
Bill Su
This is an initial attempt at configuring wxScrolled<>'s autoscroll region. The default configuration emulates the existing behavior, but can be overridden to change the autoscroll region. In particular, my personal requirement is for the region to be inside the window, but near the edge, but the configuration functions allow for some other possibilities.
Would anyone prefer a different API? Or have any other comments?
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25978
Commit Summary
- 19b7337 make autoscroll region configurable
File Changes
(3 files)
- M include/wx/scrolwin.h (22)
- M samples/drawing/drawing.cpp (19)
- M src/generic/scrlwing.cpp (122)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25978.patch
- https://github.com/wxWidgets/wxWidgets/pull/25978.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.
Sorry, I'm really confused by the API, so I'd appreciate some clarification/examples. TIA!
> @@ -171,6 +171,17 @@ bool wxScrollHelperEvtHandler::ProcessEvent(wxEvent& event)
{
wxEventType evType = event.GetEventType();
+ // always process these mouse events ourselves, even if the user code handles
+ // them as well, as we need to autoscroll
+ if ( evType == wxEVT_LEFT_DOWN )
+ {
+ m_scrollHelper->HandleOnLButtonUp((wxMouseEvent&)event);
It seems counterintuitive to process wxEVT_LEFT_DOWN events in a function called OnLButtonUp. I think wx convention would be to use just OnLeftDown (with "Mouse" and "Button" being implicit).
> @@ -152,6 +152,20 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
// returns 0 if no scrollbars are there.
int GetScrollLines( int orient ) const;
+ // wxWidgets <= 3.3.1 autoscrolled exactly when the (captured) mouse cursor
I admit I have a lot of trouble parsing this. Maybe I could manage to do it, but it's easier to just ask questions, so here I go:
- Why do we have 2 functions if they are not independent (or are they?)? It would be better to have a single function with 2 parameters instead, I think. It's annoying that they're of the same type and so it would be simple to give them in wrong order, but it would be also simpler to check for invalid combinations (e.g. I think that valid inner and invalid outer are invalid?).
- Perhaps we should have 2 functions but based on whether they operate on window or client coordinates/rectangle basis? I think this would also make it simpler to use this.
- Can this be used to disable auto-scroll entirely? It seems like it should be possible to, but it's not obvious how.
- What is your actual use case, i.e. how do you use it?
—
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 2 commits.
- 77389a3 fixup! make autoscroll region configurable
- 16ea5d5 fixup! make autoscroll region configurable
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -171,6 +171,17 @@ bool wxScrollHelperEvtHandler::ProcessEvent(wxEvent& event)
{
wxEventType evType = event.GetEventType();
+ // always process these mouse events ourselves, even if the user code handles
+ // them as well, as we need to autoscroll
+ if ( evType == wxEVT_LEFT_DOWN )
+ {
+ m_scrollHelper->HandleOnLButtonUp((wxMouseEvent&)event);
Fixed in 77389a3
—
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 commented on this pull request.
> @@ -152,6 +152,20 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
// returns 0 if no scrollbars are there.
int GetScrollLines( int orient ) const;
+ // wxWidgets <= 3.3.1 autoscrolled exactly when the (captured) mouse cursor
It is entirely possible that this interface is overly complicated by attempting to be both (unnecessarily?) general and default to backward compatible behavior. Personally, I don't like the original behavior, but I assumed it had to remain the default. The only behavior that I actually care about is having the window autoscroll when the mouse is in a picture-frame-shaped region (16 pixels wide in the example in the drawing sample) that covers the outermost portion of the client rectangle. However, for generality, I chose an interface that allows using a picture-frame-shaped region outside the (window) rectangle rather than the original behavior's infinitely large region. Also for generality, the interface allows for the autoscroll region to extend both inside and outside the client rectangle. If it would make things easier to understand, I would be happy to restrict the options to the original behavior and the picture-frame-in-client-space region. (I would be even happier to remove the old behavior!)
To answer the specific questions about the proposed interface:
1: The two functions in this interface are mostly, but not entirely, independent. The dependency is that I want the inside-the-window autoscroll region to be based on the CLIENT rect so that no one has to worry about how to deal with making the autoscroll region account for the scrollbars. However, the original code has the autoscroll region based on the WINDOW rect. My attempt to resolve this inconsistency is to have the outside-the-window autoscroll region be relative to the WINDOW rect by default, but relative to the CLIENT rect if there is an inside-window autoscroll region.
1: I have no objection to a single function, but since I considered the zones mostly independent, I thought it was plausible that some API users would only want to change one of them.
1: I don't know what you mean by invalid combinations. The invalid values are negative sizes other than wxDefaultCoord, and that is invalid no matter what the other parameter is. In the case of a strictly positive value K inner region and a wxDefaultCoord outer region, then the inner region is the expected picture-frame with width K, and the outer region is the entire plane outside the client rect, so the total autoscroll region is the entire plane minus a rectangular hole in the middle of the client rect.
2: We could have both client and window rect based versions of the zones. I don't want to define an inside-the-window zone that extends into the scroll bar, but, now that you suggest it, I see that Windows Explorer's autoscroll zone includes the scrollbar, I had not previously noticed that.
3: Yes, this can disable autoscroll. I have added a demonstration of that in 16ea5d5.
4: My use case is dragging objects both within a window and from one window to another. Therefore, like Windows Explorer (see https://groups.google.com/g/wx-dev/c/9tJ3mpkJ4NQ/m/ysvIr7llAQAJ), I want the autoscroll region to be INSIDE the window, not outside it. While dragging inside the original window, the original window should autoscroll, but when dragging out of the window, the original window should stop autoscrolling, and when the dragged object is over a destination window, the destination window should autoscroll. This PR is only taking care of changing the autscrolling of the original window; once this is done, I will have to implement autoscrolling the destination window.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
trogmaniac
trogmaniac left a comment (wxWidgets/wxWidgets#25978)If the autoscroll code is touched at all, i think it should also fix the (IMHO) only flaw it currently has: It can't scroll in both directions at the same time. E.g. if you drag and move the mouse outside at the bottom, it starts scrolling down. But if you then also move the mouse outside to the right side, it won't start also scrolling in that directions.
—
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.
- a5b6684 wxScrolled<>: for interior scroll, autoscroll window under mouse
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 1 commit.
- a5f5095 wxScrolled<>: for interior scroll, autoscroll window under mouse
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb pushed 2 commits.
- 81103a5 wxScrolled<>: support auto-scrolling both orientations simultaneously
- 6533645 wxScrolled<>: make autoscroll region configurable
—
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#25978)If the autoscroll code is touched at all, i think it should also fix the (IMHO) only flaw it currently has: It can't scroll in both directions at the same time. E.g. if you drag and move the mouse outside at the bottom, it starts scrolling down. But if you then also move the mouse outside to the right side, it won't start also scrolling in that directions.
I have made this change in 81103a5, and then re-built the autoscroll zone changes on top of that.
—
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#25978)Sorry for the delay, I'll try to get to this before 3.3.2.
—
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.
Sorry for the long delay, I've finally looked at this again and I think I understand the logic better now, so I'm ready to merge this, but I wonder if this could be improved a bit first. Notably I'd really like to avoid using std::pair and could make the proposed change myself if you agree.
I think trying to make things more clear is worth it as this is not the most obvious part of code already...
> + std::pair<wxEventType /*horz*/, + wxEventType /*vert*/> AutoscrollTest(wxPoint clientPt) const;
This is relatively minor, but I really dislike using std::pair as first and second are horrible field names. I thought about replacing it with an ad hoc struct ScrollEvents { wxEventType horz, vert; } but after looking at how this function is used, I think it would make even more sense to give it this signature:
- std::pair<wxEventType /*horz*/, - wxEventType /*vert*/> AutoscrollTest(wxPoint clientPt) const; + bool AutoscrollTest(wxPoint clientPt, wxEventType& evtHorzScroll, wxEventType& evtVertScroll) const;
to avoid tests for both of the events being wxEVT_NULL as the function would just return false in this case.
What do you think?
> @@ -345,6 +364,10 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
}
+ wxCoord m_innerScrollZone = wxDefaultCoord;
+ wxCoord m_outerScrollZone = wxDefaultCoord;
+ bool m_inNonScrollRegion = true;
Why use negation here? Wouldn't
⬇️ Suggested change- bool m_inNonScrollRegion = true; + bool m_inAutoScrollRegion = false;
be clearer?
> @@ -122,20 +112,46 @@ void wxAutoScrollTimer::Notify()
}
else // we still capture the mouse, continue generating events
{
+ // where is the mouse?
+ // client coords
+ wxPoint pt = m_win->ScreenToClient(wxGetMousePosition());
Very minor, but could be made more clear that it's not going to change:
⬇️ Suggested change- wxPoint pt = m_win->ScreenToClient(wxGetMousePosition()); + const wxPoint pt = m_win->ScreenToClient(wxGetMousePosition());
> // first scroll the window if we are allowed to do it
- wxScrollWinEvent event1(m_eventType, m_pos, m_orient);
- event1.SetEventObject(m_win);
- event1.SetId(m_win->GetId());
- if ( m_scrollHelper->SendAutoScrollEvents(event1) &&
- m_win->GetEventHandler()->ProcessEvent(event1) )
+ bool needMotion = false;
+ auto orientations = {
As above
⬇️ Suggested change- auto orientations = {
+ const auto orientations = {
> // first scroll the window if we are allowed to do it
- wxScrollWinEvent event1(m_eventType, m_pos, m_orient);
- event1.SetEventObject(m_win);
- event1.SetId(m_win->GetId());
- if ( m_scrollHelper->SendAutoScrollEvents(event1) &&
- m_win->GetEventHandler()->ProcessEvent(event1) )
+ bool needMotion = false;
+ auto orientations = {
+ std::make_pair(horizontalEvent, wxHORIZONTAL),
+ std::make_pair(verticalEvent, wxVERTICAL),
+ };
+ for (auto& orientation : orientations)
And here too
⬇️ Suggested change- for (auto& orientation : orientations) + for ( const auto& orientation : orientations )
> + needMotion = ( m_scrollHelper->SendAutoScrollEvents(event1) && + m_win->GetEventHandler()->ProcessEvent(event1) ) + || + needMotion;
I think this would be simpler to parse:
⬇️ Suggested change- needMotion = ( m_scrollHelper->SendAutoScrollEvents(event1) && - m_win->GetEventHandler()->ProcessEvent(event1) ) - || - needMotion; + if ( m_scrollHelper->SendAutoScrollEvents(event1) && +m_win->GetEventHandler()->ProcessEvent(event1) ) + needMotion = true;
> +// wxWidgets <= 3.3.1 autoscrolled exactly when the (captured) mouse cursor +// was outside the entire window. Starting with wxWidgets 3.3.2, the +// region where the mouse cursor triggers autoscrolling is configurable. +// If the inner scroll zone is default, then the autoscroll region is +// relative to the WINDOW rect, and has width of size outer scroll zone. +// If the outer scroll zone width is default, then it extends infinitely +// outward from the WINDOW rect, and the behavior is the same as +// wxWidgets 3.3.1. If the inner scroll zone is non-default, then the +// autoscroll region is relative to the CLIENT rect since that avoids +// requiring the user to figure out how the width of scroll region +// interacts with the scrollbar.
I'd rather not duplicate exactly the same comment both in the header and here as they will inevitably get out of sync later resulting in confusion. Let's have it in one place only (not sure where it's more appropriate) and just refer to it in the other one.
> @@ -263,8 +280,10 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
// the methods to be called from the window event handlers
void HandleOnScroll(wxScrollWinEvent& event);
void HandleOnSize(wxSizeEvent& event);
- void HandleOnMouseEnter(wxMouseEvent& event);
- void HandleOnMouseLeave(wxMouseEvent& event);
+ void OnMotion(wxMouseEvent& event);
+ void OnLeftDown(wxMouseEvent& event);
+ void OnEnterNonScrollRegion();
+ void OnLeaveNonScrollRegion();
I think these function should be private.
Also, perhaps it would be more clear to avoid using "Non" in their names, e.g.
⬇️ Suggested change- void OnLeaveNonScrollRegion(); + void OnLeaveAutoScrollRegion(); + void OnEnterAutoScrollRegion();
?
—
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 commented on this pull request.
> @@ -263,8 +280,10 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
// the methods to be called from the window event handlers
void HandleOnScroll(wxScrollWinEvent& event);
void HandleOnSize(wxSizeEvent& event);
- void HandleOnMouseEnter(wxMouseEvent& event);
- void HandleOnMouseLeave(wxMouseEvent& event);
+ void OnMotion(wxMouseEvent& event);
+ void OnLeftDown(wxMouseEvent& event);
+ void OnEnterNonScrollRegion();
+ void OnLeaveNonScrollRegion();
wxScrollHelperEvtHandler calls OnMotion(), etc., but these functions are in wxScrollHelperBase, so they can't be private unless wxScrollHelperEvtHandler becomes a friend. Is that your intention?
—
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.
> @@ -263,8 +280,10 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
// the methods to be called from the window event handlers
void HandleOnScroll(wxScrollWinEvent& event);
void HandleOnSize(wxSizeEvent& event);
- void HandleOnMouseEnter(wxMouseEvent& event);
- void HandleOnMouseLeave(wxMouseEvent& event);
+ void OnMotion(wxMouseEvent& event);
+ void OnLeftDown(wxMouseEvent& event);
+ void OnEnterNonScrollRegion();
+ void OnLeaveNonScrollRegion();
Sorry, I had overlooked that they were called from wxScrollHelperEvtHandler to be honest, so please disregard the first part of this comment (the second one would still be worth considering IMO).
This being said, I rather like the idea of making wxScrollHelperEvtHandler friend of this class instead of making all these functions public. But this is not really related to this PR and probably would be better done in a separate one.
—
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 5 commits.
- e6fbbaf wxScrolled<>: avoid std::pair<>
- 563f042 wxScroll<>: rename *NonScroll* to *AutoScroll*
- e3c116d wxScrolled<>: add some const markers for clarity
- 36f0c5b wxScrolled<>: rewrite boolean arithmetic as if for clarity
- c04d8bf wxScrolled<>: avoid duplicated comments
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
@wsu-cb commented on this pull request.
> @@ -345,6 +364,10 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
}
+ wxCoord m_innerScrollZone = wxDefaultCoord;
+ wxCoord m_outerScrollZone = wxDefaultCoord;
+ bool m_inNonScrollRegion = true;
I actually initially wrote this in positive form, and then switched it to negative because the original code used OnEnterWindow(), and to keep the OnEnter equivalent, I used OnEnterNonScrollRegion(). However, I agree that OnLeaveAutoScrollRegion() is clearer if one isn't trying to match the original code.
Fixed in 563f042
—
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 commented on this pull request.
> @@ -122,20 +112,46 @@ void wxAutoScrollTimer::Notify()
}
else // we still capture the mouse, continue generating events
{
+ // where is the mouse?
+ // client coords
+ wxPoint pt = m_win->ScreenToClient(wxGetMousePosition());
Do you want local variables marked const (when possible) in general?
Fixed in e3c116d
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> // first scroll the window if we are allowed to do it
- wxScrollWinEvent event1(m_eventType, m_pos, m_orient);
- event1.SetEventObject(m_win);
- event1.SetId(m_win->GetId());
- if ( m_scrollHelper->SendAutoScrollEvents(event1) &&
- m_win->GetEventHandler()->ProcessEvent(event1) )
+ bool needMotion = false;
+ auto orientations = {
Fixed in e3c116d
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> // first scroll the window if we are allowed to do it
- wxScrollWinEvent event1(m_eventType, m_pos, m_orient);
- event1.SetEventObject(m_win);
- event1.SetId(m_win->GetId());
- if ( m_scrollHelper->SendAutoScrollEvents(event1) &&
- m_win->GetEventHandler()->ProcessEvent(event1) )
+ bool needMotion = false;
+ auto orientations = {
+ std::make_pair(horizontalEvent, wxHORIZONTAL),
+ std::make_pair(verticalEvent, wxVERTICAL),
+ };
+ for (auto& orientation : orientations)
Fixed in e3c116d
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> + needMotion = ( m_scrollHelper->SendAutoScrollEvents(event1) && + m_win->GetEventHandler()->ProcessEvent(event1) ) + || + needMotion;
Fixed in 36f0c5b
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Bill Su
> +// wxWidgets <= 3.3.1 autoscrolled exactly when the (captured) mouse cursor +// was outside the entire window. Starting with wxWidgets 3.3.2, the +// region where the mouse cursor triggers autoscrolling is configurable. +// If the inner scroll zone is default, then the autoscroll region is +// relative to the WINDOW rect, and has width of size outer scroll zone. +// If the outer scroll zone width is default, then it extends infinitely +// outward from the WINDOW rect, and the behavior is the same as +// wxWidgets 3.3.1. If the inner scroll zone is non-default, then the +// autoscroll region is relative to the CLIENT rect since that avoids +// requiring the user to figure out how the width of scroll region +// interacts with the scrollbar.
Fixed in c04d8bf
—
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 commented on this pull request.
> @@ -263,8 +280,10 @@ class WXDLLIMPEXP_CORE wxScrollHelperBase : public wxAnyScrollHelperBase
// the methods to be called from the window event handlers
void HandleOnScroll(wxScrollWinEvent& event);
void HandleOnSize(wxSizeEvent& event);
- void HandleOnMouseEnter(wxMouseEvent& event);
- void HandleOnMouseLeave(wxMouseEvent& event);
+ void OnMotion(wxMouseEvent& event);
+ void OnLeftDown(wxMouseEvent& event);
+ void OnEnterNonScrollRegion();
+ void OnLeaveNonScrollRegion();
Second part of comment fixed in 563f042
—
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.
Looks good, thanks! Will merge soon.
—
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#25978)Looks good, thanks! Will merge soon.
I'm certainly not going to complain if you merge now, but, now that you have approved the implementation,I was planning to document SetInnerScrollZone() and SetInnerScrollZone() in interface/wx/scrolwin.h before merging.
—
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 2 commits.
- ad63a2f wxScrolled<>: document new autoscroll region configuration
- d313c5d wxTipWindow: fix some documentation typos
—
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#25978)Commit d313c5d fixes some typos unrelated to this pull request.
—
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#25978)Thanks, I've indeed forgotten that this still needed to be documented. But while applying the docs commit (I'm afraid I've started rewriting it trying to make it more clear...) I've realized that I can't explain the difference between inner zone being set to 0 or -1: both values behave the same, don't they?
OTOH setting outer zone to 0 is very different from setting it to -1 and this doesn't seem good.
—
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#25978)Maybe we should handle -1 as INT_MAX, i.e. enable autoscrolling anywhere inside the window when the inner scroll zone is set to this value? This doesn't seem very useful, but it would be consistent with the outer zone.
Of course, if we do this, its default value would need to be changed to 0.
—
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#25978)My attempt at documenting the existing behaviour:
diff --git a/interface/wx/scrolwin.h b/interface/wx/scrolwin.h index 7eb4774d1b..67da5ea2e8 100644 --- a/interface/wx/scrolwin.h +++ b/interface/wx/scrolwin.h @@ -95,6 +95,28 @@ enum wxScrollbarVisibility window out of the visible area), the child window will report a position of (10,-90). + @section scrolled_autoscroll Automatic scrolling + + Scrolled windows implement automatic scrolling behaviour: when the mouse is + captured by the application and the user holds the mouse button down, the + window may start and keep scrolling even if the mouse doesn't move, + provided that it is in the "autoscroll zone". This is convenient to allow + extending selection to the parts of the window currently outside of the + visible area, for example. + + By default, autoscrolling is triggered when the mouse is anywhere outside + of the window, i.e. the window starts scrolling when the (captured) mouse + pointer leaves the window and stops when it re-enters the window. However, + this behaviour can be customized using SetInnerScrollZone() and + SetOuterScrollZone() functions. The first of them allows to enable + autoscrolling even when mouse is inside the window, but close to its + border, while the second one allows to disable autoscrolling when mouse is + outside the window. + + To disable autoscrolling completely, call SetOuterScrollZone() with 0 + argument. + + @beginStyleTable @style{wxHSCROLL} If this style is specified and ::wxVSCROLL isn't, the window will be @@ -603,6 +625,47 @@ public: void SetScrollPageSize(int orient, int pageSize); int GetScrollLines( int orient ) const; + /** + Set the width of the autoscroll zone inside the client rectangle. + + Setting inner scroll zone to a non-zero value enables autoscrolling if + the distance between the mouse and the closest edge of the client + rectangle is less than the given value. + + By default, autoscrolling is not triggered when the mouse is inside the + window. + + @see @ref scrolled_autoscroll, SetOuterScrollZone() + + @param innerZone + The width of the inner scroll zone in pixels. If this parameter is + set to either 0 or ::wxDefaultCoord, there is no inner scroll zone + and autoscrolling may be triggered only when the mouse is outside + of the window (unless it is disabled too). + + @since 3.3.2 + */ + void SetInnerScrollZone(wxCoord innerZone); + + /** + Set the width of the auto-scroll zone outside the window. + + By default, autoscrolling is triggered when the mouse is anywhere + outside of the window. + + @see @ref scrolled_autoscroll, SetInnerScrollZone() + + @param outerZone + The width of the outer scroll zone in pixels. If this parameter is + set to ::wxDefaultCoord, autoscrolling is triggered when the mouse + is anywhere outside of the window. May be set to 0 to disable + autoscrolling when the mouse is outside of the window entirely. + It is typically not useful to set it at any other value. + + @since 3.3.2 + */ + void SetOuterScrollZone(wxCoord outerZone); + /** Set the scaling factor for the window.
I think this is correct (but please let me know if it isn't), but this shows at least 2 problems with the current API:
- Previously mentioned inconsistency between the value of
wxDefaultCoordfor inner and outer zone: in one case, it disables autoscroll, in the other one it allows it anywhere. - Lack of any real use for the
SetOuterScrollZone()parameter: the only useful value for it is 0, it just doesn't make sense to call it with anything else.
I think that perhaps we should have the following functions instead:
EnableInnerAutoScroll(int width)wherewidth > 0.DisableOuterAutoScroll().
This is still not consistent, but at least it's pretty clear what the functions do and they don't allow doing anything useless, such as calling SetOuterScrollZone(5).
What do you think?
—
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#25978)Thanks, I've indeed forgotten that this still needed to be documented. But while applying the docs commit (I'm afraid I've started rewriting it trying to make it more clear...) I've realized that I can't explain the difference between inner zone being set to
0or-1: both values behave the same, don't they?
Setting the inner zone to -1 (aka wxDefaultCoord) is not the same as 0, but I admit you might not like the difference: the default wxDefaultCoord defines the OUTER scroll zone relative to the WINDOW rect, but setting the inner scroll zone to 0 defines the OUTER scroll zone relative to the CLIENT rect.
OTOH setting outer zone to
0is very different from setting it to-1and this doesn't seem good.
I don't understand why 0 and -1 being different is objectionable.
—
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#25978)Setting the inner zone to
-1(akawxDefaultCoord) is not the same as0, but I admit you might not like the difference: the defaultwxDefaultCoorddefines the OUTER scroll zone relative to the WINDOW rect, but setting the inner scroll zone to0defines the OUTER scroll zone relative to the CLIENT rect.
Oh, I see (after looking at the code again). This is still really confusing and not at all consistent with how these values are used for the outer zone, however.
Shouldn't outer zone always cover everything outside of the client area?
OTOH setting outer zone to
0is very different from setting it to-1and this doesn't seem good.I don't understand why
0and-1being different is objectionable.
This is not objectionable, of course. It's the difference between interpretations of -1 between zones which is: for the inner zone it is (roughly) the same as 0, but for the outer zone it's (roughly) exactly the opposite of 0.
—
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#25978)Maybe we should handle
-1asINT_MAX, i.e. enable autoscrolling anywhere inside the window when the inner scroll zone is set to this value? This doesn't seem very useful, but it would be consistent with the outer zone.
Of course, if we do this, its default value would need to be changed to 0.
I prefer that the default value be wxDefaultCoord, but, as maintainer, you have the final decision.
—
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#25978)I prefer that the default value be
wxDefaultCoord, but, as maintainer, you have the final decision.
I'd rather reach some consensus. I understand that it's better to use wxDefaultCoord as default value, but this is in conflict with inner autoscroll being disabled by default and outer being enabled. And IMO it's worse for the same value to mean 2 different things in related contexts.
What do you think about EnableInnerAutoScroll(width) and DisableOuterAutoScroll() idea? The names could be improved, e.g. EnableAutoScrollInside() and DisableAutoScrollOutside() might be better.
—
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#25978)Shouldn't outer zone always cover everything outside of the client area?
I think this is the crux of the issue. As I wrote in #25978 (comment), it is entirely possible that I have attempted too much generality. Because I don't like the outer scroll zone at all, I am unqualified to determine whether a partial outer scroll zone is ever useful.
This is not objectionable, of course. It's the difference between interpretations of
-1between zones which is: for the inner zone it is (roughly) the same as0, but for the outer zone it's (roughly) exactly the opposite of0.
I see your point now, and can't dispute it.
For my purposes, the suggested API in #25978 (comment) would work well. I chose a general API to avoid objections about it being inconsistent to permit a partial inner scroll zone but not a partial outer scroll zone, but if no one values that generality, I actually prefer the simpler API as well.
—
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#25978)If nobody else (if anybody else is reading this in the first place...) has any better ideas, I'll change the API like this — or, alternatively, please let me know if you'd prefer to do it yourself. I've pushed the commits I was about to merge in configurable-auto-scroll branch in my fork, if you make any changes, please base them either on master or on that branch because it has some changes compared to this PR.
—
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#25978)If nobody else (if anybody else is reading this in the first place...) has any better ideas, I'll change the API like this — or, alternatively, please let me know if you'd prefer to do it yourself. I've pushed the commits I was about to merge in configurable-auto-scroll branch in my fork, if you make any changes, please base them either on master or on that branch because it has some changes compared to this PR.
I will look at your branch and get back to you...
—
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#25978)I will look at your branch and get back to you...
After experimenting, I now think the additional complexity in my previous design of sometimes setting the autoscroll region relative to the client rectangle is also not worthwhile. I now think I will revert to the wxWidgets 3.2 behavior of always basing the autoscroll region on the window rectangle (although the implementation is unrelated to the 3.2 implementation using wxEVT_ENTER_WINDOW/wxEVT_LEAVE_WINDOW).
—
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 3 commits.
- af88a67 Improve auto-scrolling in wxScrolled
- 2fdad1d Document wxScrolled::Set{Inner,Outer}ScrollZone()
- 7694eaf wxScrolled<>: simplify autoscroll configuration API
—
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#25978)Commit 7694eaf modifies interface/wx/scrolwin.h to match the recent discussion. Does this look good to you? If so, I will modify the implementation to match.
—
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.
- 4ab8c81 wxScrolled<>: simplify autoscroll configuration API
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25978)Yes, thank you, this looks good, the only remaining question is whether we should allow passing wxDefaultCoord to EnableAutoScrollInside(). As long as it works in the same way as 0, I would say that we shouldn't and that the function should assert that its argument is >= 0. And setting it to 0 would disable inner auto scroll region, of course.
—
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#25978)Yes, thank you, this looks good, the only remaining question is whether we should allow passing
wxDefaultCoordtoEnableAutoScrollInside(). As long as it works in the same way as0, I would say that we shouldn't and that the function should assert that its argument is>= 0. And setting it to 0 would disable inner auto scroll region, of course.
As noted in #25978 (comment), my preference is that the default value be wxDefaultCoord, even if it acts the same as 0, and even if it means a little bit of extra overhead in the implementation.
—
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#25978)But now there is no more distinction between client and window, so -1 and 0 behave exactly the same, IIUC. Why have 2 ways to do the same thing?
Alternative would be to forbid passing 0 to this function, but this doesn't look logical.
—
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#25978)But now there is no more distinction between client and window, so
-1and0behave exactly the same, IIUC. Why have 2 ways to do the same thing?
It bothers me for wxDefaultCoord to be illegal, and it seems perfectly reasonable to me for wxDefaultCoord to mean something that can also be specified explicitly. In fact, I would EXPECT that to be true in most cases.
Alternative would be to forbid passing 0 to this function, but this doesn't look logical.
I agree that forbidding 0 here would be even worse than forbidding wxDefaultCoord.
—
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#25978)it seems perfectly reasonable to me for
wxDefaultCoordto mean something that can also be specified explicitly
I can't think of any occurrence of this in wxWidgets API. wxDefaultXXX always means "unspecified" or, well, "default". There is no reasonable default value for the inner auto scroll zone size, but I'd actually expect setting it to wxDefaultCoord to do exactly this, i.e. choose some predefined value like FromDIP(5), rather than disable it entirely.
In fact, you've helped me put my finger on it: this API looks wrong to me because it uses wxDefaultCoord as meaning "none" and this is just not how it's used anywhere else.
—
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#25978)I can't think of any occurrence of this in wxWidgets API.
wxDefaultXXXalways means "unspecified" or, well, "default". There is no reasonable default value for the inner auto scroll zone size, but I'd actually expect setting it towxDefaultCoordto do exactly this, i.e. choose some predefined value likeFromDIP(5), rather than disable it entirely.
This appears to be where we disagree: I think 0 is the default value. The difference between 1 and 0 is enormous in a logical sense, but for the user, a 1-pixel wide scroll zone is almost as unhittable as a 0-pixel wide scroll zone.
In fact, you've helped me put my finger on it: this API looks wrong to me because it uses
wxDefaultCoordas meaning "none" and this is just not how it's used anywhere else.
—
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#25978)I'd still keep wxDefaultCoord for the future use as meaning "use default width for the inner zone" and would let use 0 to disable it now (not that it will be often useful, as it's disabled by default and I don't see why would you ever enable it first and then disable it later). But I won't argue any more.
—
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#25978)One way or another, this needs to be finalized soon if it's to be included in 3.3.2.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.