widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Code looks OK (as far as I can grasp it).
One nit inline.

I will however compile this and do some stress tesing on OSX with a dual Monitor layout.

Will/Hsould the 'f' (or some) shortcut be supported inside the menu?

Diff comments:

> === modified file 'src/ui_basic/checkbox.cc'
> --- src/ui_basic/checkbox.cc	2016-10-25 08:11:31 +0000
> +++ src/ui_basic/checkbox.cc	2016-12-10 10:30:36 +0000
> @@ -45,33 +47,47 @@
>  	set_size(w, h);
>  	set_can_focus(true);
>  	set_flags(Has_Custom_Picture, true);
> -	pic_graphics_ = pic;
>  }
>  
>  Statebox::Statebox(Panel* const parent,
>                     Vector2i const p,
>                     const std::string& label_text,
>                     const std::string& tooltip_text,
> -                   uint32_t width)
> -   : Panel(parent, p.x, p.y, kStateboxSize, kStateboxSize, tooltip_text),
> +                   int width)
> +   : Panel(parent, p.x, p.y, std::max(width, kStateboxSize), kStateboxSize, tooltip_text),
>       flags_(Is_Enabled),
> -     rendered_text_(label_text.empty() ? nullptr :
> -                                         UI::g_fh1->render(as_uifont(label_text),
> -                                                           width > (kStateboxSize + kPadding) ?
> -                                                              width - kStateboxSize - kPadding :
> -                                                              0)) {
> -	pic_graphics_ = g_gr->images().get("images/ui_basic/checkbox_light.png");
> -	if (rendered_text_) {
> -		int w = rendered_text_->width() + kPadding + pic_graphics_->width() / 2;
> -		int h = std::max(rendered_text_->height(), pic_graphics_->height());

this is a rather complex ? expression, cans this be refactores into some helper fucntions?

> +     pic_graphics_(g_gr->images().get("images/ui_basic/checkbox_light.png")),
> +     label_text_(label_text),
> +     rendered_text_(nullptr) {
> +	set_flags(Has_Text, !label_text_.empty());
> +	layout();
> +}
> +
> +void Statebox::layout() {
> +	// We only need to relayout if we have text
> +	if (flags_ & Has_Text) {
> +		int w = get_w();
> +		int h = kStateboxSize;
> +		int pic_width = kStateboxSize;
> +		if (pic_graphics_) {
> +			w = std::max(pic_graphics_->width(), w);
> +			h = pic_graphics_->height();
> +			pic_width = pic_graphics_->width();
> +		}
> +		rendered_text_ = label_text_.empty() ?
> +		                    nullptr :
> +		                    UI::g_fh1->render(
> +		                       as_uifont(label_text_),
> +		                       get_w() > (pic_width + kPadding) ? get_w() - pic_width - kPadding : 0);
> +		if (rendered_text_) {
> +			w = std::max(rendered_text_->width() + kPadding + pic_width, w);
> +			h = std::max(rendered_text_->height(), h);
> +		}
>  		set_desired_size(w, h);
>  		set_size(w, h);
>  	}
>  }
>  
> -Statebox::~Statebox() {
> -}
> -
>  /**
>   * Set the enabled state of the checkbox. A disabled checkbox cannot be clicked
>   * and is somewhat darker to tell it apart from enabled ones.


-- 
https://code.launchpad.net/~widelands-dev/widelands/fsmenu_fullscreen_4_options/+merge/312966
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/fsmenu_fullscreen_4_options into lp:widelands.