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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Codechange: Increase scrollbar length limit to 32bits #8006

wants to merge 1 commit into from
+37 −39

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

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 February 16, 2020 23:06
@LordAro
Copy link
Member Author

LordAro commented Feb 16, 2020

good catch, seems I'd missed quite a few

@nielsmh nielsmh mentioned this pull request Feb 16, 2020
Merged
6 tasks
@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Feb 22, 2020
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch 2 times, most recently from 462d4f2 to 77b52dc Compare February 26, 2020 22:46
@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 This PR should be backport to current release (RC / stable) label Feb 26, 2020
glx22
glx22 reviewed Feb 26, 2020
View reviewed changes
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed labels Dec 14, 2020
@TrueBrain TrueBrain added size: large This Pull Request is large in size; review will take a while work: minor details This Pull Request has some minor details left to do and removed size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed labels Dec 21, 2020
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from 77b52dc to 50e6c94 Compare January 3, 2021 17:11
@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Jan 3, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 January 3, 2021 17:13 Inactive
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
TrueBrain
TrueBrain reviewed Jan 6, 2021
View reviewed changes
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from 50e6c94 to 9767467 Compare January 10, 2021 12:28
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 January 10, 2021 12:28 Inactive
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from 9767467 to f8d0365 Compare January 11, 2021 19:02
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 January 11, 2021 19:02 Inactive
TrueBrain
TrueBrain reviewed Jan 11, 2021
View reviewed changes
stormcone
stormcone reviewed Jan 11, 2021
View reviewed changes
@LordAro LordAro force-pushed the unlimited-scrollbars-but-within-reason branch from f8d0365 to 800b469 Compare January 11, 2021 23:34
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 January 11, 2021 23:34 Inactive
TrueBrain
TrueBrain previously approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

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 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 March 25, 2021 23:15
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 March 25, 2021 23:15 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 This PR should be backport to current release (RC / stable) and removed size: large This Pull Request is large in size; review will take a while work: minor details This Pull Request has some minor details left to do labels Mar 25, 2021
@LordAro
Copy link
Member Author

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 March 26, 2021 12:29
@DorpsGek DorpsGek temporarily deployed to preview-pr-8006 March 26, 2021 12:30 Inactive
nielsmh
nielsmh requested changes Apr 10, 2021
View reviewed changes
Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

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

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?

Sorry, something went wrong.

@LordAro LordAro added work: needs more work This Pull Request needs more work before it can be accepted and removed backport requested This PR should be backport to current release (RC / stable) labels May 1, 2021
@TrueBrain TrueBrain removed the preview This PR is receiving preview builds label Jul 8, 2023
@PeterN PeterN mentioned this pull request Feb 25, 2024
Closed
@rubidium42 rubidium42 mentioned this pull request Feb 25, 2024
Merged
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

Assignees
No one assigned
Labels
candidate: yes This Pull Request is a candidate for being merged work: needs more work This Pull Request needs more work before it can be accepted
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