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

Fix advanced inventory manager crash on small terminals #61166

Merged

Conversation

ZeroInternalReflection
Copy link
Contributor

Summary

Bugfixes "Fix advanced inventory manager crash on small terminals"

Purpose of change

The advanced inventory manager crashes when opened with a terminal width of <82. This has been noted several times on Android, but is not Android-exclusive. The crash looks like this (text in Portuguese for a better view of the string trimming problem discussed below):

Before--Improper_trimming_and_crash.mp4

Fixes #59629
Fixes #60047
#59821 has similar symptoms but no crash/debug log, so it might be a duplicate of above

Describe the solution

The crash is due to two separate bugs interacting:

  1. Fix a bug where trim_by_length would end up one character too long #59476, in an attempt to streamline string trimming, reserves a string based on the available width. This works as expected for normal widths, but will crash when the program tries to trim a string to a length less than 0.
  2. The pane header for the advanced inventory manager improperly calculates the width available for terrain description/flags. If you look carefully at the video above, you'll note that the tile description starts getting truncated well before anything overlaps, and the game crashes as soon as the available area is reduced below 0.

So, to solve this:

  1. If trim_by_length is given a width <=0, then something has already gone wrong in the UI design, so add a check and provide a warning debugmsg. Thus, if this problem occurs anywhere else, we'll at least be told what's happening so it can be fixed.
    Trim_by_length_warning
  2. Calculate the space actually used by the right side of the advanced inventory management pane header, which results in a minimum width > 0

Describe alternatives you've considered

Testing

Test cases were added for trim_by_length given width <= 0, confirming that it provides a debugmsg warning.

The AIM was tested at a variety of terminal widths. It did not crash at any size tested, and appeared to trim text correctly. (The page listing on the next line down does not trim correctly, but that's a separate part of the code)

After--Proper_trimming.mp4

Additional context

  • crash #60942 is, based on the log, a different crash on opening the AIM that I was unable to replicate
  • When deliberately given a width < 0 in testing, the debugmsg takes well over a minute to generate a backtrace (compared to ~5s for the other debugmsgs I triggered). I'm unsure why.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Sep 20, 2022
@Fris0uman Fris0uman merged commit f848351 into CleverRaven:master Sep 21, 2022
@WreckingAbandon
Copy link

Wooh yeah baby! I've been waiting for this for a month!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
3 participants