Merge lp:~widelands-dev/widelands/bug-1741779-map-description-edit-cutoff into lp:widelands
- bug-1741779-map-description-edit-cutoff
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 8723 | ||||
Proposed branch: | lp:~widelands-dev/widelands/bug-1741779-map-description-edit-cutoff | ||||
Merge into: | lp:widelands | ||||
Diff against target: |
156 lines (+20/-22) 8 files modified
src/graphic/CMakeLists.txt (+1/-0) src/graphic/graphic.cc (+9/-0) src/graphic/graphic.h (+1/-3) src/graphic/text/rendered_text.cc (+1/-5) src/graphic/text/rt_render.cc (+1/-6) src/graphic/text_layout.cc (+2/-1) src/ui_basic/editbox.cc (+3/-4) src/ui_basic/multilineeditbox.cc (+2/-3) |
||||
To merge this branch: | bzr merge lp:~widelands-dev/widelands/bug-1741779-map-description-edit-cutoff | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Notabilis | Approve | ||
kaputtnik (community) | testing | Approve | |
Review via email: mp+345495@code.launchpad.net |
Commit message
Removed texture size restrictions from editboxes. We no longer need them thanks to the RenderedText class.
Description of the change
bunnybot (widelandsofficial) wrote : | # |
Notabilis (notabilis27) wrote : | # |
I am not quite sure what to make out of this branch. If I guess right, that means that text in editboxes should be able to become as long as I want. However, when typing long texts I get (both here and in trunk):
Texture (2051, 18) too big! Maximum size is 2048.
terminate called after throwing an instance of 'RT::TextureTooBig'
what(): Texture (2051, 18) too big! Maximum size is 2048.
Tested with the host-ip box in the LAN lobby, the edit field when saving a game and the map description when using the editor by entering one long string (i.e., keeping key pressed for 10 seconds). When entering multiple "words" in the editor it does not crash in either branch but starts lagging after some time.
Some remarks about the code:
- In Editbox the maximum length can also be set by set_max_length(). I guess the condition there has to be adapted, too?
m_->maxLength = std::min(
(Also a small documentation bug: the function is called set_max_length() but the in the documentation of set_text() it is called setMaxLength().)
- It seems like MultilineEditbo
GunChleoc (gunchleoc) wrote : | # |
YOu're right, somebody leaning endlessly on a key will eventually crash it, because there are no word breaks for splitting the text into separate textures.
I have brought back the limit and fixed it for the Multilineeditbox - the bug there was that we forgot to square the texture size.
I haven't tested my last commit yet, it's still compiling.
Notabilis (notabilis27) wrote : | # |
Hm, seems to behave as with the commit before. Only tested the map description edit box, but long inputs still crash and I seem to be able to input ... well, at least very long texts.
On a side note: Tab key has a strange order in that dialog.
GunChleoc (gunchleoc) wrote : | # |
I have tied the calculation to actual rendered font height now - this fixes the Editbox for me.
The problem in Multilineeditbox was the text_width function used by the WordWrap class - I have added a safeguard there too.
I have also opened a new bug for the tabbing order:
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 3533. State: failed. Details: https:/
Appveyor build 3337. State: success. Details: https:/
Notabilis (notabilis27) wrote : | # |
Thanks for the bug report.
Unfortunately, I am still unable to notice any changes. :(
Maybe I am understanding the purpose of this branch wrong or are testing something unrelated.
Can you describe a testcase that works different in this branch than in trunk?
Setting maxLength and maxBytes to 3 in the source does restrict the number of characters I am able to enter, so it seems to me as if the calculations show different results for you than for me. Sounds strange, though, since I am assuming that the font and its height is everywhere the same. Does it help if I try to figure out restrictions that do not lead to crashes for me?
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 3543. State: passed. Details: https:/
Appveyor build 3347. State: success. Details: https:/
GunChleoc (gunchleoc) wrote : | # |
Open the "Last bastion" map in the editor, open "Map Options" and scroll down to the bottom of the description. Some text is missing here in trunk.
Font and its height is not the same everywhere - we get bigger textures for Arabic.
By all means, change the restrictions so it won't crash for you. I would expect the results to be identical for the default fontset, so this is strange indeed.
I would like to rework all this stuff again once we get rid of the old renderer - at the moment, the WordWrap class is used by both renderers, so I don' want to touch it. Too many complications.
Notabilis (notabilis27) wrote : | # |
Okay, thanks. I think I finally understand what is happening on my system.
Regarding the primary bug, I am unfortunately unable to reproduce it on my system. But the code is looking okay and if it fixes the bug for you, that is enough testing for me.
Are you by chance testing in release mode? Or has your system only limited texture memory? My RT::TextureTooBig exception is not triggered in release mode since another restriction is used there. My exception is triggered due to graphic/
I added some questions regarding int-casting in the diff, but they haven't been introduced by your changes and someone probably had a reasons for the casts.
kaputtnik (franku) wrote : | # |
The main bug is solved, IMHO.
GunChleoc (gunchleoc) wrote : | # |
I introduced that condition in check_size to make sure that the texture size would be OK for everybody. That was especially important before we had the RenderedText class. I have now shifted the difference to Graphic, which will hopefully fix the bug in debug builds.
Notabilis (notabilis27) wrote : | # |
Thanks, it is no longer crashing for me now.
One nit: In rendered_
GunChleoc (gunchleoc) wrote : | # |
That line needs to be fixed
GunChleoc (gunchleoc) wrote : | # |
Done :)
@bunnybot merge
Preview Diff
1 | === modified file 'src/graphic/CMakeLists.txt' | |||
2 | --- src/graphic/CMakeLists.txt 2018-05-11 04:42:13 +0000 | |||
3 | +++ src/graphic/CMakeLists.txt 2018-05-31 16:50:31 +0000 | |||
4 | @@ -300,6 +300,7 @@ | |||
5 | 300 | text_layout.cc | 300 | text_layout.cc |
6 | 301 | text_layout.h | 301 | text_layout.h |
7 | 302 | DEPENDS | 302 | DEPENDS |
8 | 303 | graphic # TODO(Gunchleoc): For text_width safety only - rewrite that function | ||
9 | 303 | graphic_align | 304 | graphic_align |
10 | 304 | graphic_color | 305 | graphic_color |
11 | 305 | graphic_surface | 306 | graphic_surface |
12 | 306 | 307 | ||
13 | === modified file 'src/graphic/graphic.cc' | |||
14 | --- src/graphic/graphic.cc 2018-04-27 06:11:05 +0000 | |||
15 | +++ src/graphic/graphic.cc 2018-05-31 16:50:31 +0000 | |||
16 | @@ -171,6 +171,15 @@ | |||
17 | 171 | return render_target_.get(); | 171 | return render_target_.get(); |
18 | 172 | } | 172 | } |
19 | 173 | 173 | ||
20 | 174 | int Graphic::max_texture_size_for_font_rendering() const { | ||
21 | 175 | // Test with minimum supported size in debug builds. | ||
22 | 176 | #ifndef NDEBUG | ||
23 | 177 | return kMinimumSizeForTextures; | ||
24 | 178 | #else | ||
25 | 179 | return max_texture_size_; | ||
26 | 180 | #endif | ||
27 | 181 | } | ||
28 | 182 | |||
29 | 174 | bool Graphic::fullscreen() { | 183 | bool Graphic::fullscreen() { |
30 | 175 | uint32_t flags = SDL_GetWindowFlags(sdl_window_); | 184 | uint32_t flags = SDL_GetWindowFlags(sdl_window_); |
31 | 176 | return (flags & SDL_WINDOW_FULLSCREEN) || (flags & SDL_WINDOW_FULLSCREEN_DESKTOP); | 185 | return (flags & SDL_WINDOW_FULLSCREEN) || (flags & SDL_WINDOW_FULLSCREEN_DESKTOP); |
32 | 177 | 186 | ||
33 | === modified file 'src/graphic/graphic.h' | |||
34 | --- src/graphic/graphic.h 2018-04-27 06:11:05 +0000 | |||
35 | +++ src/graphic/graphic.h 2018-05-31 16:50:31 +0000 | |||
36 | @@ -81,9 +81,7 @@ | |||
37 | 81 | return sdl_window_; | 81 | return sdl_window_; |
38 | 82 | } | 82 | } |
39 | 83 | 83 | ||
43 | 84 | int max_texture_size() const { | 84 | int max_texture_size_for_font_rendering() const; |
41 | 85 | return max_texture_size_; | ||
42 | 86 | } | ||
44 | 87 | 85 | ||
45 | 88 | ImageCache& images() const { | 86 | ImageCache& images() const { |
46 | 89 | return *image_cache_.get(); | 87 | return *image_cache_.get(); |
47 | 90 | 88 | ||
48 | === modified file 'src/graphic/text/rendered_text.cc' | |||
49 | --- src/graphic/text/rendered_text.cc 2018-04-07 16:59:00 +0000 | |||
50 | +++ src/graphic/text/rendered_text.cc 2018-05-31 16:50:31 +0000 | |||
51 | @@ -180,11 +180,7 @@ | |||
52 | 180 | 180 | ||
53 | 181 | // Draw Solid background Color | 181 | // Draw Solid background Color |
54 | 182 | if (rect.has_background_color()) { | 182 | if (rect.has_background_color()) { |
60 | 183 | #ifndef NDEBUG | 183 | const int maximum_size = g_gr->max_texture_size_for_font_rendering(); |
56 | 184 | const int maximum_size = kMinimumSizeForTextures; | ||
57 | 185 | #else | ||
58 | 186 | const int maximum_size = g_gr->max_texture_size(); | ||
59 | 187 | #endif | ||
61 | 188 | const int tile_width = std::min(maximum_size, rect.width()); | 184 | const int tile_width = std::min(maximum_size, rect.width()); |
62 | 189 | const int tile_height = std::min(maximum_size, rect.height()); | 185 | const int tile_height = std::min(maximum_size, rect.height()); |
63 | 190 | for (int tile_x = blit_point.x; tile_x + tile_width <= blit_point.x + rect.width(); | 186 | for (int tile_x = blit_point.x; tile_x + tile_width <= blit_point.x + rect.width(); |
64 | 191 | 187 | ||
65 | === modified file 'src/graphic/text/rt_render.cc' | |||
66 | --- src/graphic/text/rt_render.cc 2018-04-07 16:59:00 +0000 | |||
67 | +++ src/graphic/text/rt_render.cc 2018-05-31 16:50:31 +0000 | |||
68 | @@ -264,12 +264,7 @@ | |||
69 | 264 | /// Throws a TextureTooBig exception if the given dimensions would be bigger than the graphics | 264 | /// Throws a TextureTooBig exception if the given dimensions would be bigger than the graphics |
70 | 265 | /// can handle | 265 | /// can handle |
71 | 266 | void check_size(int check_w, int check_h) { | 266 | void check_size(int check_w, int check_h) { |
78 | 267 | // Test for minimum supported size in debug builds. | 267 | const int maximum_size = g_gr->max_texture_size_for_font_rendering(); |
73 | 268 | #ifndef NDEBUG | ||
74 | 269 | const int maximum_size = kMinimumSizeForTextures; | ||
75 | 270 | #else | ||
76 | 271 | const int maximum_size = g_gr->max_texture_size(); | ||
77 | 272 | #endif | ||
79 | 273 | if (check_w > maximum_size || check_h > maximum_size) { | 268 | if (check_w > maximum_size || check_h > maximum_size) { |
80 | 274 | const std::string error_message = | 269 | const std::string error_message = |
81 | 275 | (boost::format("Texture (%d, %d) too big! Maximum size is %d.") % check_w % check_h % | 270 | (boost::format("Texture (%d, %d) too big! Maximum size is %d.") % check_w % check_h % |
82 | 276 | 271 | ||
83 | === modified file 'src/graphic/text_layout.cc' | |||
84 | --- src/graphic/text_layout.cc 2018-05-05 17:10:37 +0000 | |||
85 | +++ src/graphic/text_layout.cc 2018-05-31 16:50:31 +0000 | |||
86 | @@ -26,6 +26,7 @@ | |||
87 | 26 | #include <boost/format.hpp> | 26 | #include <boost/format.hpp> |
88 | 27 | 27 | ||
89 | 28 | #include "graphic/font_handler1.h" | 28 | #include "graphic/font_handler1.h" |
90 | 29 | #include "graphic/graphic.h" | ||
91 | 29 | #include "graphic/image.h" | 30 | #include "graphic/image.h" |
92 | 30 | #include "graphic/text/bidi.h" | 31 | #include "graphic/text/bidi.h" |
93 | 31 | #include "graphic/text/font_set.h" | 32 | #include "graphic/text/font_set.h" |
94 | @@ -49,7 +50,7 @@ | |||
95 | 49 | } | 50 | } |
96 | 50 | 51 | ||
97 | 51 | int text_width(const std::string& text, int ptsize) { | 52 | int text_width(const std::string& text, int ptsize) { |
99 | 52 | return UI::g_fh1->render(as_editorfont(text, ptsize - UI::g_fh1->fontset()->size_offset())) | 53 | return UI::g_fh1->render(as_editorfont(text.substr(0, g_gr->max_texture_size_for_font_rendering() / text_height() - 1), ptsize - UI::g_fh1->fontset()->size_offset())) |
100 | 53 | ->width(); | 54 | ->width(); |
101 | 54 | } | 55 | } |
102 | 55 | 56 | ||
103 | 56 | 57 | ||
104 | === modified file 'src/ui_basic/editbox.cc' | |||
105 | --- src/ui_basic/editbox.cc 2018-04-27 06:11:05 +0000 | |||
106 | +++ src/ui_basic/editbox.cc 2018-05-31 16:50:31 +0000 | |||
107 | @@ -97,8 +97,7 @@ | |||
108 | 97 | m_->caret = 0; | 97 | m_->caret = 0; |
109 | 98 | m_->scrolloffset = 0; | 98 | m_->scrolloffset = 0; |
110 | 99 | // yes, use *signed* max as maximum length; just a small safe-guard. | 99 | // yes, use *signed* max as maximum length; just a small safe-guard. |
113 | 100 | m_->maxLength = | 100 | set_max_length(std::numeric_limits<int32_t>::max()); |
112 | 101 | std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, std::numeric_limits<int32_t>::max()); | ||
114 | 102 | 101 | ||
115 | 103 | set_handle_mouse(true); | 102 | set_handle_mouse(true); |
116 | 104 | set_can_focus(true); | 103 | set_can_focus(true); |
117 | @@ -124,7 +123,7 @@ | |||
118 | 124 | * Set the current text in the edit box. | 123 | * Set the current text in the edit box. |
119 | 125 | * | 124 | * |
120 | 126 | * The text is truncated if it is longer than the maximum length set by | 125 | * The text is truncated if it is longer than the maximum length set by |
122 | 127 | * \ref setMaxLength(). | 126 | * \ref set_max_length(). |
123 | 128 | */ | 127 | */ |
124 | 129 | void EditBox::set_text(const std::string& t) { | 128 | void EditBox::set_text(const std::string& t) { |
125 | 130 | if (t == m_->text) | 129 | if (t == m_->text) |
126 | @@ -146,7 +145,7 @@ | |||
127 | 146 | * its end is cut off to fit into the maximum length. | 145 | * its end is cut off to fit into the maximum length. |
128 | 147 | */ | 146 | */ |
129 | 148 | void EditBox::set_max_length(uint32_t const n) { | 147 | void EditBox::set_max_length(uint32_t const n) { |
131 | 149 | m_->maxLength = std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, static_cast<int>(n)); | 148 | m_->maxLength = std::min(g_gr->max_texture_size_for_font_rendering() / text_height(), static_cast<int>(n)); |
132 | 150 | 149 | ||
133 | 151 | if (m_->text.size() > m_->maxLength) { | 150 | if (m_->text.size() > m_->maxLength) { |
134 | 152 | m_->text.erase(m_->text.begin() + m_->maxLength, m_->text.end()); | 151 | m_->text.erase(m_->text.begin() + m_->maxLength, m_->text.end()); |
135 | 153 | 152 | ||
136 | === modified file 'src/ui_basic/multilineeditbox.cc' | |||
137 | --- src/ui_basic/multilineeditbox.cc 2018-05-13 07:15:39 +0000 | |||
138 | +++ src/ui_basic/multilineeditbox.cc 2018-05-31 16:50:31 +0000 | |||
139 | @@ -97,7 +97,7 @@ | |||
140 | 97 | background_style(style), | 97 | background_style(style), |
141 | 98 | cursor_pos(0), | 98 | cursor_pos(0), |
142 | 99 | lineheight(text_height()), | 99 | lineheight(text_height()), |
144 | 100 | maxbytes(std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, 0xffff)), | 100 | maxbytes(std::min(g_gr->max_texture_size_for_font_rendering() * g_gr->max_texture_size_for_font_rendering() / (text_height() * text_height()), std::numeric_limits<int32_t>::max())), |
145 | 101 | ww_valid(false), | 101 | ww_valid(false), |
146 | 102 | owner(o) { | 102 | owner(o) { |
147 | 103 | scrollbar.moved.connect(boost::bind(&MultilineEditbox::scrollpos_changed, &o, _1)); | 103 | scrollbar.moved.connect(boost::bind(&MultilineEditbox::scrollpos_changed, &o, _1)); |
148 | @@ -133,8 +133,7 @@ | |||
149 | 133 | d_->erase_bytes(d_->prev_char(d_->text.size()), d_->text.size()); | 133 | d_->erase_bytes(d_->prev_char(d_->text.size()), d_->text.size()); |
150 | 134 | } | 134 | } |
151 | 135 | 135 | ||
154 | 136 | d_->set_cursor_pos(d_->text.size()); | 136 | d_->set_cursor_pos(0); |
153 | 137 | |||
155 | 138 | d_->update(); | 137 | d_->update(); |
156 | 139 | d_->scroll_cursor_into_view(); | 138 | d_->scroll_cursor_into_view(); |
157 | 140 | 139 |
Continuous integration builds have changed state:
Travis build 3514. State: passed. Details: https:/ /travis- ci.org/ widelands/ widelands/ builds/ 378610485. /ci.appveyor. com/project/ widelands- dev/widelands/ build/_ widelands_ dev_widelands_ bug_1741779_ map_description _edit_cutoff- 3319.
Appveyor build 3319. State: success. Details: https:/