Merge lp:~widelands-dev/widelands/bugfix-1801208 into lp:widelands

Proposed by Arty
Status: Merged
Merged at revision: 8923
Proposed branch: lp:~widelands-dev/widelands/bugfix-1801208
Merge into: lp:widelands
Diff against target: 48 lines (+14/-4)
2 files modified
src/ui_basic/multilinetextarea.cc (+12/-3)
src/ui_basic/multilinetextarea.h (+2/-1)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bugfix-1801208
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+358594@code.launchpad.net

Commit message

Changed some types from uint to int to avoid overflows in calculations.

Forced the text anchor for drawing multiline text areas to be non-negative, so that text alignment wouldn't be based on overlong lines. (Otherwise short lines wouldn't be aligned properly, possibly even be outside the visible area.)

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 4206. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/453338126.
Appveyor build 4002. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bugfix_1801208-4002.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Code LGTM :)

@bunnybot merge

review: Approve
Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4206. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/453338126.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Transient failure on Travis

@bunnybot merge force

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/ui_basic/multilinetextarea.cc'
--- src/ui_basic/multilinetextarea.cc 2018-08-12 16:35:30 +0000
+++ src/ui_basic/multilinetextarea.cc 2018-11-10 17:41:58 +0000
@@ -30,7 +30,8 @@
3030
31namespace UI {31namespace UI {
3232
33static const uint32_t RICHTEXT_MARGIN = 2;33// int instead of uint because of overflow situations
34static const int32_t RICHTEXT_MARGIN = 2;
3435
35MultilineTextarea::MultilineTextarea(Panel* const parent,36MultilineTextarea::MultilineTextarea(Panel* const parent,
36 const int32_t x,37 const int32_t x,
@@ -141,11 +142,19 @@
141 int anchor = 0;142 int anchor = 0;
142 Align alignment = mirror_alignment(align_, text_);143 Align alignment = mirror_alignment(align_, text_);
143 switch (alignment) {144 switch (alignment) {
145 // TODO(Arty): We might want to revisit this after the font renderer can handle long strings
146 // without whitespaces differently.
147 // Currently, such long unbreakable strings are silently assumed to fit the line exactly,
148 // which means that rendered_text_->width() might actually be larger than the effective width
149 // of the textarea. If we'd allow the anchor here to become negative in this case, it would
150 // properly position the longest line (just truncated), BUT the positioning of shorter lines
151 // would be off (possibly even outside the textarea, thus invisible) because their positioning
152 // is calculated without regard for overlong lines.
144 case UI::Align::kCenter:153 case UI::Align::kCenter:
145 anchor = (get_eff_w() - rendered_text_->width()) / 2;154 anchor = std::max(0, (get_eff_w() - rendered_text_->width()) / 2);
146 break;155 break;
147 case UI::Align::kRight:156 case UI::Align::kRight:
148 anchor = get_eff_w() - rendered_text_->width() - RICHTEXT_MARGIN;157 anchor = std::max(0, get_eff_w() - rendered_text_->width() - RICHTEXT_MARGIN);
149 break;158 break;
150 case UI::Align::kLeft:159 case UI::Align::kLeft:
151 anchor = RICHTEXT_MARGIN;160 anchor = RICHTEXT_MARGIN;
152161
=== modified file 'src/ui_basic/multilinetextarea.h'
--- src/ui_basic/multilinetextarea.h 2018-07-08 09:18:33 +0000
+++ src/ui_basic/multilinetextarea.h 2018-11-10 17:41:58 +0000
@@ -60,7 +60,8 @@
60 }60 }
6161
62 void set_text(const std::string&);62 void set_text(const std::string&);
63 uint32_t get_eff_w() const {63 // int instead of uint because of overflow situations
64 int32_t get_eff_w() const {
64 return scrollbar_.is_enabled() ? get_w() - Scrollbar::kSize : get_w();65 return scrollbar_.is_enabled() ? get_w() - Scrollbar::kSize : get_w();
65 }66 }
6667

Subscribers

People subscribed via source and target branches

to status/vote changes: