Details

Reviewers

Commits

rP20500: Fix scrollbare edgy-case and off by 1's in dropdown

Summary

Currently the selected item of the biome dropdown on f.e. ambush map looks ugly:

This is due to there are 9 elements in the list and the size of the elements is 25*9=225 and the space saved for the dropdown list is just 224 so we try to add a scrollbar, but a scrollbar for 1 pixel won't work, thus we get something ugly.
Also notice the cpp code is not trivial since it will set a scrollbar when the sizes are equal, sowhen something just fits, we try to add a scrollbar?

Fixing both:

Test Plan

Test dropdowns with 9 elements on different resolutions with different sizes.
Make sure scrollbars of different sizes work (as in 1 pixel too)

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bb created this revision.Nov 12 2017, 6:27 PM

Owners added a subscriber: Restricted Owners Package.Nov 12 2017, 6:27 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B3674: Diff 4154.Nov 12 2017, 7:06 PM

Why does Vulcan yell about those stables? seems unrelated

In D1030#40477, @bb wrote:

Why does Vulcan yell about those stables? seems unrelated

(rP20427)

Looks good, but I wonder if we shouldn't fix the behaviour, when the list is 1 px too long (either ignore that it is too long or show the scrollbar...)

Edit: To cut away this one px L401 could be changed to m_HideScrollBar = !m_ItemsYPositions.empty() && m_ItemsYPositions.back() <= size + 1.f;

For information: The wrong size was introduced in rP15006.

Seeing the proof it still can be bugged: fix it properly by adding a +1 to the check (so no scrollbar when we can only scroll 1 pix, just add that one pix to the frame), not to the set size since that would change all sizes and not only the one we want.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B3690: Diff 4177.Nov 14 2017, 1:50 PM

Yes.
(With that the list box can get 1px bigger than it is defined in its xml, but that's the only sane way to fix this bug)

source/gui/CDropDown.cpp
396 ↗	(On Diff #4177)	What about some comment stating why we add 1 px?

This revision is now accepted and ready to land.Nov 14 2017, 3:10 PM

add a comment

(In general an engine ought to add a scrollbar when needed even if only by one pixel if that one pixel can be fixed in the XML. An engine never ought to reserve space for a scrollbar if it doesn't render a scrollbar.
I don't know in which places one pixel is off or what the patch exactly does, so my current me has no concern with any proposed solution. Thanks for taking care of it!)

Actually fix the scrollbar: allow scrollbars for 1 pixel

refs rP17149, rP13773, rP13771

@elexis why have you choosen 1px in rP17149?

source/gui/IGUIScrollBar.h
215 ↗	(On Diff #4183)	Shouldn't that be a bool?

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B3695: Diff 4181.Nov 14 2017, 4:28 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B3697: Diff 4183.Nov 14 2017, 5:28 PM

Imarok added inline comments.Nov 14 2017, 11:29 PM

source/gui/IGUIScrollBar.h
210 ↗	(On Diff #4183)	Sure this is needed? Also you could divide by 0 with that in https://github.com/0ad/0ad/blob/3bf098b1bf47e313c0d2bd952812cc365fd9ec93/source/gui/CGUIScrollBarVertical.cpp#L155

bb added inline comments.Nov 15 2017, 11:10 AM

source/gui/IGUIScrollBar.h
210 ↗	(On Diff #4183)	Dunno, but what would the check below has for use if we do not change this... Also we make it invisible if it is 0, so there is no need for calling that function, and that isn't happening either. See the call from getBarRect either have such a check of are mouse actions which are only executed AFAIK when the scrollbar is visible
215 ↗	(On Diff #4183)	Should perhaps

Make bool

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B3708: Diff 4197.Nov 15 2017, 12:08 PM

In D1030#40695, @Imarok wrote:

@elexis why have you choosen 1px in rP17149?

That function looks improvable given that it's a float and not a bool and that the comment was copied without modification from the function above.
I suspect it's due to that function comparing with GetMaxPos() and 1.f is the smallest number returned.
So I believe the question is equivalent to asking why that other function returns 1.f.

In D1030#40889, @elexis wrote:

In D1030#40695, @Imarok wrote:

@elexis why have you choosen 1px in rP17149?

That function looks improvable given that it's a float and not a bool and that the comment was copied without modification from the function above.
I suspect it's due to that function comparing with GetMaxPos() and 1.f is the smallest number returned.
So I believe the question is equivalent to asking why that other function returns 1.f.

What bug should your patch fix originally?
The ticket didn't help me much...
As far as I tested it everything works also when isVisible always returns true.

In D1030#40906, @Imarok wrote:

What bug should your patch fix originally?
The ticket didn't help me much...

comment:2 is more precise than the ticket description.

change the duplicate wrong comment

I couldn't reproduce the issue written above

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B3734: Diff 4237.Nov 17 2017, 12:52 PM

Imarok added inline comments.Nov 17 2017, 5:28 PM

source/gui/IGUIScrollBar.h
210 ↗	(On Diff #4183)	When we make it invisible for 0 then why not just let this stay 1?

bb added inline comments.Nov 17 2017, 5:32 PM

source/gui/IGUIScrollBar.h
210 ↗	(On Diff #4183)	since bars of 1 height should be visible, since you can scroll there (y ok 1 pixel, but still)

Imarok added inline comments.Nov 17 2017, 5:40 PM

source/gui/IGUIScrollBar.h
210 ↗	(On Diff #4183)	Hmm, it seems like this change is needed...

Seems like it is the right way

Closed by commit rP20500: Fix scrollbare edgy-case and off by 1's in dropdown (authored by bb). · Explain WhyNov 21 2017, 9:07 PM

This revision was automatically updated to reflect the committed changes.

Fix off by two error in dropdowns and of by one in scrollbar
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 4312

ps/trunk/binaries/data/mods/mod/gui/common/modern/styles.xml

ps/trunk/source/gui/CDropDown.cpp

ps/trunk/source/gui/IGUIScrollBar.h

Fix off by two error in dropdowns and of by one in scrollbarClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 4312

ps/trunk/binaries/data/mods/mod/gui/common/modern/styles.xml

ps/trunk/source/gui/CDropDown.cpp

ps/trunk/source/gui/IGUIScrollBar.h

Fix off by two error in dropdowns and of by one in scrollbar
ClosedPublic
Actions

Revision Contents
Changeset List