widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

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/text/rt_render.cc:266 where different values for debug and release are used. When using the release version your restrictions work fine. The problem in debug mode is that the exception uses the smaller (hardcoded) value while your checks are using the real (hardware) value which is seemingly bigger on my system. Maybe we should move the restriction from rt_render.cc:266 into graphic/graphic.cc:91 ? (If you do: render_text.cc:183 can then be cleaned up, too.) Except for the text rendering no-one seems to care about the maximum texture size so moving this should be safe.

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.

Diff comments:

> 
> === modified file 'src/ui_basic/editbox.cc'
> --- src/ui_basic/editbox.cc	2018-04-27 06:11:05 +0000
> +++ src/ui_basic/editbox.cc	2018-05-23 07:31:09 +0000
> @@ -97,8 +97,7 @@
>  	m_->caret = 0;
>  	m_->scrolloffset = 0;
>  	// yes, use *signed* max as maximum length; just a small safe-guard.

Does this safe-guard make sense? set_max_length() takes an uint. Is the idea that we end up with (Uint::max() / 2) ?

> -	m_->maxLength =
> -	   std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, std::numeric_limits<int32_t>::max());
> +	set_max_length(std::numeric_limits<int32_t>::max());
>  
>  	set_handle_mouse(true);
>  	set_can_focus(true);
> @@ -146,7 +145,7 @@
>   * its end is cut off to fit into the maximum length.
>   */
>  void EditBox::set_max_length(uint32_t const n) {
> -	m_->maxLength = std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, static_cast<int>(n));
> +	m_->maxLength = std::min(g_gr->max_texture_size() / text_height(), static_cast<int>(n));

Why casting here? n could theoretically be too big for int.
Maybe cast the first part to uint? (And possibly add an assertion that the first part is >= 0?)

>  
>  	if (m_->text.size() > m_->maxLength) {
>  		m_->text.erase(m_->text.begin() + m_->maxLength, m_->text.end());
> 
> === modified file 'src/ui_basic/multilineeditbox.cc'
> --- src/ui_basic/multilineeditbox.cc	2018-05-13 07:15:39 +0000
> +++ src/ui_basic/multilineeditbox.cc	2018-05-23 07:31:09 +0000
> @@ -97,7 +97,7 @@
>       background_style(style),
>       cursor_pos(0),
>       lineheight(text_height()),
> -     maxbytes(std::min(g_gr->max_texture_size() / UI_FONT_SIZE_SMALL, 0xffff)),
> +     maxbytes(std::min(g_gr->max_texture_size() * g_gr->max_texture_size() / (text_height() * text_height()), std::numeric_limits<int32_t>::max())),

Same here.

>       ww_valid(false),
>       owner(o) {
>  	scrollbar.moved.connect(boost::bind(&MultilineEditbox::scrollpos_changed, &o, _1));


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1741779-map-description-edit-cutoff/+merge/345495
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1741779-map-description-edit-cutoff into lp:widelands.