Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Codechange: Increase scrollbar length limit to 32bits #8006

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Codechange: Increase scrollbar length limit to 32bits #8006

wants to merge 1 commit into from

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Feb 16, 2020

Also makes scrollbar internals properly use unsigned numbers. Bumped scrollbar limit to uint(32) as well, because why not

Fixes #7613

@stormcone
Copy link
Contributor

@stormcone stormcone commented Feb 16, 2020

There are 3 occurrences of this->group_sb->SetCount((uint)this->groups.size()); in group_gui.cpp, where I think the (uint) could be removed.

@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from f37d429 to e3022c0 Compare Feb 16, 2020
@LordAro
Copy link
Member Author

@LordAro LordAro commented Feb 16, 2020

good catch, seems I'd missed quite a few

@nielsmh nielsmh mentioned this pull request Feb 16, 2020
Open
6 tasks
@LordAro LordAro added the backport requested label Feb 22, 2020
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch 2 times, most recently from 462d4f2 to 77b52dc Compare Feb 26, 2020
@LordAro LordAro changed the title Only store 1024 news messages Codechange: Increase scrollbar length limit to UINT_MAX and make their length properly unsigned Feb 26, 2020
@LordAro LordAro removed the backport requested label Feb 26, 2020
glx22
glx22 reviewed Feb 26, 2020
View changes
src/newgrf_debug_gui.cpp Show resolved Hide resolved
src/newgrf_gui.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added candidate: yes size: extra large labels Dec 14, 2020
@TrueBrain TrueBrain added size: large work: minor details and removed size: extra large labels Dec 21, 2020
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from 77b52dc to 50e6c94 Compare Jan 3, 2021
@TrueBrain TrueBrain added the preview label Jan 3, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 Jan 3, 2021 Inactive
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
TrueBrain
TrueBrain reviewed Jan 6, 2021
View changes
src/group_gui.cpp Outdated Show resolved Hide resolved
src/newgrf_debug_gui.cpp Outdated Show resolved Hide resolved
src/widget.cpp Outdated Show resolved Hide resolved
src/network/network_content_gui.cpp Outdated Show resolved Hide resolved
src/ai/ai_gui.cpp Outdated Show resolved Hide resolved
src/widget_type.h Outdated Show resolved Hide resolved
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from 50e6c94 to 9767467 Compare Jan 10, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 Jan 10, 2021 Inactive
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from 9767467 to f8d0365 Compare Jan 11, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 Jan 11, 2021 Inactive
TrueBrain
TrueBrain reviewed Jan 11, 2021
View changes
src/company_gui.cpp Outdated Show resolved Hide resolved
stormcone
stormcone reviewed Jan 11, 2021
View changes
src/core/math_func.hpp Outdated Show resolved Hide resolved
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from f8d0365 to 800b469 Compare Jan 11, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 Jan 11, 2021 Inactive
TrueBrain
TrueBrain previously approved these changes Jan 12, 2021
View changes
Copy link
Member

@TrueBrain TrueBrain left a comment

No longer crashes the game, tried a few windows, scrolling seems to work fine.

I am just there will be some window that doesn't work as we want or what-ever, but we will fix that when people report it :D

@LordAro
Copy link
Member Author

@LordAro LordAro commented Jan 12, 2021

Shouldn't be merged yet, lots of size_t -> uint warnings on Windows. Going to have to readd a load of casts :(

@TrueBrain TrueBrain removed this from the 1.11.0 milestone Mar 8, 2021
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from 800b469 to c879ecb Compare Mar 25, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 Mar 25, 2021 Inactive
@LordAro LordAro changed the title Codechange: Increase scrollbar length limit to UINT_MAX and make their length properly unsigned Codechange: Increase scrollbar length limit to 32bits Mar 25, 2021
@LordAro LordAro added backport requested and removed size: large work: minor details labels Mar 25, 2021
@LordAro
Copy link
Member Author

@LordAro LordAro commented Mar 25, 2021

I've updated it to just increase the size, no fiddling around with type-signedness.

...Except for some reason changing it from unsigned short to unsigned int has caused the compiler to no longer be able to make as many coersions as it was able to previously. I've fixed them in a "minimal" way. A better solution would be to go through the whole system to check everything for over/underflows, as I've no doubt there are some lurking somewhere.

@LordAro LordAro mentioned this pull request Mar 25, 2021
Merged
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from c879ecb to b91758d Compare Mar 26, 2021
nielsmh
nielsmh requested changes Apr 10, 2021
View changes
Copy link
Contributor

@nielsmh nielsmh left a comment

This code has a lot of typecasting to stop the compiler complaining about signed/unsigned type ranges, but it's not actually range safe as far as I can tell. If the scrollbar isn't going to work properly with more than 2^31 elements in the list anyway, just make it use a plain signed int type and do some sanity checks against negative numbers in a few strategic locations.

src/company_gui.cpp
@@ -731,7 +731,7 @@ struct SelectCompanyLiveryWindow : public Window {
/* Position scrollbar to selected group */
for (uint i = 0; i < this->rows; i++) {
if (this->groups[i]->index == sel) {
this->vscroll->SetPosition(Clamp(i - this->vscroll->GetCapacity() / 2, 0, std::max(this->vscroll->GetCount() - this->vscroll->GetCapacity(), 0)));
this->vscroll->SetPosition(Clamp(i - this->vscroll->GetCapacity() / 2, 0, std::max<int>(this->vscroll->GetCount() - this->vscroll->GetCapacity(), 0)));
Copy link
Contributor

@nielsmh nielsmh Apr 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is really confusing. Actually I'm not sure why Scrollbar::SetPosition assert-checks that the parameter is in range, instead of just clamping it into range itself. Letting the scrollbar handle that would make all the using code simpler, wouldn't it?

@LordAro LordAro added work: needs more work and removed backport requested labels May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glx22 glx22

@stormcone stormcone

@nielsmh nielsmh

@TrueBrain TrueBrain

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
candidate: yes preview work: needs more work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Generating too many news messages at once crashes the game.
6 participants
@LordAro @stormcone @nielsmh @TrueBrain @glx22 @DorpsGek