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

Don't insist that windows request nonzero height or width #5000

Merged
merged 1 commit into from Jul 20, 2020

Conversation

gwhitney
Copy link
Contributor

Reverts the assertion concerning nonzero window dimensions in
gui/widgets/window.cpp; it turns out that some windows do request zero
dimension, and it doesn't hurt anything for them to.

Fixes #4960.

@Wedge009
Copy link
Member

Wedge009 commented Jul 19, 2020

I'm just wondering if it's worth still keeping the lower-bound check, but assert that it's >= 0 rather than just > 0? I shudder to think when negative values might be used, but I suppose it could happen so could well be worthwhile to check for them.

@Wedge009
Copy link
Member

Maybe throw in a comment about why 0-value dimensions are permitted as well.

@gwhitney
Copy link
Contributor Author

Thanks for the suggestions, done on both counts.

@Wedge009
Copy link
Member

Wedge009 commented Jul 20, 2020

Maybe I'm missing something but it looks the same as what I saw earlier. :S

Edit: oh, it's an unsigned comparison - so only an upper-bound check is necessary?

@Wedge009
Copy link
Member

I just tested a local change to make assertion >= 0 and it seems to resolve the issue.

  Eases the assertion concerning nonzero window dimensions in
  gui/widgets/window.cpp; it turns out that some windows do request zero
  dimension, and it doesn't hurt anything for them to.

  Fixes wesnoth#4960.
@gwhitney
Copy link
Contributor Author

Dang, forgot git commit --amend does not make a fuss if you (cheeks red) happened to forget to git add your changes. (I will level with you, I am sort of sad git won out over mercurial etc. I find it fairly convoluted and error-prone, but maybe it's just lack of sustained use...)
Anyhow, all better now.

@Wedge009
Copy link
Member

Wedge009 commented Jul 20, 2020

No worries at all, I'm not that comfortable with the git command-line myself although I am learning (I did start my PC days in MS-DOS, so it's not that I dislike command-line, I'm just not used to it).

It looks good to me and I basically tested it just now, so I'm happy with it. If no-one else objects or takes action on this sooner I'll merge this around this time tomorrow.

@Pentarctagon
Copy link
Member

Vultraz does need to approve UI-related changes, since he's the UI maintainer now.

@Vultraz Vultraz merged commit ff0eecc into wesnoth:master Jul 20, 2020
@Wedge009
Copy link
Member

Right, didn't know that was the new process. All done now, it seems.

@Pentarctagon
Copy link
Member

Yep, going forward the maintainer for a specific area needs to approve PRs related to their area, or for areas without a specific maintainer it falls back to gfgtdf (for code).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abort at Outro (the "The End" screen before the credits)
4 participants