[ wxwindows-Patches-1708485 ] Pixel accounting for wxScrolledWindow
SourceForge.net
noreply at sourceforge.net
Sat Dec 1 19:20:06 PST 2007
Patches item #1708485, was opened at 2007-04-26 21:02
Message generated for change (Comment added) made by sf-robot
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=309863&aid=1708485&group_id=9863
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Generic
Group: bug fix
>Status: Closed
Resolution: None
Priority: 6
Private: No
Submitted By: d2walter (d2walter)
Assigned to: Nobody/Anonymous (nobody)
Summary: Pixel accounting for wxScrolledWindow
Initial Comment:
This is a change to allow wxScrolledWindow to track things in terms of pixels rather than scroll units. This also fixes the problem where the number of "dead pixels" could exceed the scroll unit size.
This was discussed on the mailing list back in December as [wx-dev] wxScrolledWindow problems
This is a first patch that only changes the scroll accounting to use pixels instead of scroll units. There will be another patch to allow the virtual size of the window to be an exact number rather than a multiple of the scroll increments.
I apologize for taking so long in making these changes.
----------------------------------------------------------------------
>Comment By: SourceForge Robot (sf-robot)
Date: 2007-12-01 19:20
Message:
Logged In: YES
user_id=1312539
Originator: NO
This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-11-17 16:33
Message:
Logged In: YES
user_id=71618
Originator: NO
Sorry for taking so long but I've finally returned to this patch. I think
we could apply it -- while I don't see any big changes compared to the
current behaviour (i.e. there is still a lot of "unused" space in the
scroll sample... was it supposed to disappear?) I don't see any breakage
neither and if this is a necessary first step to do more fixes later, I can
certainly understand it.
However there is one big problem: this patch currently breaks wxGTK
compilation. wxGTK uses m_[xy]Scroll{Position,Lines} in wx/gtk/scrolwin.h
and these variables don't exist any more. And I'm not sure if fixing this
is as simple as replacing them with the new GetScrollLines() accessors...
Could you please look at this? TIA!
I also attach the slightly corrected version of the patch for the current
svn trunk (no real changes there).
File Added: scroll-20071118.patch
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-06-10 02:12
Message:
Logged In: YES
user_id=71618
Originator: NO
No, it's just because it was left in the "Pending" state somehow. I'll
look at the new version a.s.a.p., thanks.
----------------------------------------------------------------------
Comment By: d2walter (d2walter)
Date: 2007-06-09 19:30
Message:
Logged In: YES
user_id=1095478
Originator: YES
I don't understand how this got automatically closed. Did I do something
wrong here?
----------------------------------------------------------------------
Comment By: SourceForge Robot (sf-robot)
Date: 2007-06-09 19:20
Message:
Logged In: YES
user_id=1312539
Originator: NO
This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).
----------------------------------------------------------------------
Comment By: d2walter (d2walter)
Date: 2007-05-26 11:37
Message:
Logged In: YES
user_id=1095478
Originator: YES
I am attaching a new patch. It adds some comments, makes a few fixes
which are now more visible because the scroll samples work again, and makes
minor changes to the scroll sample red box that make it more obvious where
the old way was failing.
File Added: tryagain.patch
----------------------------------------------------------------------
Comment By: d2walter (d2walter)
Date: 2007-05-26 11:36
Message:
Logged In: YES
user_id=1095478
Originator: YES
I am attaching a new patch. It adds some comments, makes a few fixes
which are now more visible because the scroll samples work again, and makes
minor changes to the scroll sample red box that make it more obvious where
the old way was failing.
File Added: tryagain.patch
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-05-24 16:41
Message:
Logged In: YES
user_id=71618
Originator: NO
0. Could you please explain how can I see this problem (number of unused
pixels > 1 scroll unit)? I can't really verify that the patch works without
being able to see the problem before applying it and not seeing it any more
after doing it so I absolutely need the test code and I guess you must
already have some, don't you?
1a. Could you please add a comment saying this to the patch? It's really
not obvious when just reading the code.
2. Ok.
3. How exactly is "maximum pixel view start position" defined? I'm sorry
to insist but this code is really not clear (many of the present problems
stem from lack of documentation in it) and it's thus even more crucial than
usual to have the new version clearly documented in the comments.
TIA!
----------------------------------------------------------------------
Comment By: d2walter (d2walter)
Date: 2007-05-24 07:58
Message:
Logged In: YES
user_id=1095478
Originator: YES
0. This patch makes some basic structural changes without really adding
the ability to functionally use pixel scrolling. The bug it fixes is the
extra counting of dead space which in the past could be greater than the
scroll increment. After this patch, the maximum dead space will be a
scroll unit -1.
1. a. Frequently the divisor is going to be a scroll unit position where 0
says no scrolling. If no scrolling, the result should always be 0.
1 b.c. ok.
2. The primary purpose of the comment and the functions is to ease
transition. The last time I submitted this patch, there were 5-10 changes
that used these variables inside the time that it took for the patch to be
reviewed.
3. scrollLines was the total number of scroll units, while the pixel
version I am changing things too keeps track of the maximum viewstart
position. For now GetScrollLinesX() and GetScrollLinesY() exist only too
ease transition. I think they should be completely eliminated, but that
makes my patch too big and too complicated and if anyone makes changes it
won't merge smoothly. The goal is to move to using m_x/y
MaxScrollPixelPosition which keeps track of the maximum pixel view start
position. This will allow easy verification of new view start locations
with FORCE_BETWEEN(value,0,maxScrollPixelPosition). This will also work
both when using strictly scroll unit scrolling and when the view start is
allowed to move off the scroll units.
----------------------------------------------------------------------
Comment By: Vadim Zeitlin (vadz)
Date: 2007-05-23 18:10
Message:
Logged In: YES
user_id=71618
Originator: NO
This patch looks simple enough to be accepted, thanks. But I do have
several questions about it:
0. How can I see which problems this patch fixes? I.e. do we have any test
code showing the problems which currently arise from using lines instead of
pixels? If not, it should be written (and added to the patch) as something
as fundamental as this really can't be applied without testing it first.
1. DivideAndRoundUp() looks strange to me:
a) It's not clear if the parameters are allowed to be <= 0 or not. If
they're not, there really should be an assert there (i.e. a wxCHECK). If
they're, it would be nice to know why dividing by 0 or negative number
should yield zero...
b) Why exclude the case "a == 0"? If the body was written as the (more
usual IMHO) "(a + divisor - 1)/divisor" it would work just as well for
a==0
c) This function should be static
2. I don't think the comment before GetScrollUnitPositionX() is
appropriate. The important thing is that this function returns the logical
unit coordinates, it doesn't matter if they're kept in variables or
computed on the fly. But in fact it's also not clear why do we need to add
them -- don't they do the same thing as GetViewStart()?
3. The naming of GetScrollLines[XY] methods is strange: it's not "lines"
in "X" direction but rather "columns". I realize that this was the name of
the old variables but when making changes in the code it's better to
correct the mistakes. I'd just call both functions GetScrollUnits[XY] as
you already used "unit" for the functions just above them.
More importantly, I must be missing something obvious but I don't
understand how are these functions implemented. Why do we add the max
scroll position in pixels to the width? Shouldn't we just divide the width
in pixels by the unit size in pixels t get the number of units? If I'm
wrong and the code is right, I'd really appreciate having a comment in it
explaining why is it so. And having comments explaining what
m_[xy]MaxScrollPixelPosition are, exactly, would be really helpful as
well.
Thanks in advance!
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=309863&aid=1708485&group_id=9863
More information about the wx-dev
mailing list