widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Changes look mostly good to me. A few comments in the diff, but nothing serious. In think there is a doubled function call to scroll_to_field() but that can be fixed.

I couldn't figure out where the new focus() of the panel or the scrolling for boxes are used. I wasn't able to find a box with scrollbars that is not already a list or textarea, maybe there simply is non in the current game. Also I didn't notice any changes due to the focus() call. Adding a printf() call shows that the code is reached but I found no behavior changes.

Diff comments:

> 
> === modified file 'src/ui_basic/box.cc'
> --- src/ui_basic/box.cc	2017-03-11 17:10:33 +0000
> +++ src/ui_basic/box.cc	2017-12-09 19:38:06 +0000
> @@ -135,6 +135,23 @@
>  	layout();
>  }
>  
> +bool Box::handle_mousewheel(uint32_t which, int32_t x, int32_t y) {

Is there some GUI where this is used? All scrollbars I found were with textareas or lists.
Not that I am against adding this here.

> +	if (scrollbar_) {
> +		assert(scrolling_);
> +		return scrollbar_->handle_mousewheel(which, x, y);
> +	}
> +	return Panel::handle_mousewheel(which, x, y);
> +
> +}
> +bool Box::handle_key(bool down, SDL_Keysym code) {
> +	if (scrollbar_) {
> +		assert(scrolling_);
> +		return scrollbar_->handle_key(down, code);
> +	}
> +	return Panel::handle_key(down, code);
> +}
> +
> +
>  /**
>   * Adjust all the children and the box's size.
>   */
> 
> === modified file 'src/ui_basic/panel.cc'
> --- src/ui_basic/panel.cc	2017-09-04 15:02:05 +0000
> +++ src/ui_basic/panel.cc	2017-12-09 19:38:06 +0000
> @@ -501,7 +501,10 @@
>   *
>   * \return true if the mouseclick was processed, flase otherwise

flase -> false.
Okay, not really a problem of this branch.

>   */
> -bool Panel::handle_mousepress(const uint8_t, int32_t, int32_t) {
> +bool Panel::handle_mousepress(const uint8_t btn, int32_t, int32_t) {
> +	if (btn == SDL_BUTTON_LEFT) {
> +		focus();

Where is this used / what it is doing?
In trunk when I have two windows and click on the "lower" one, it is brought to the front and its (potential) button is clicked. This would have been the behavior I expected through this change but seemingly it is not required.

> +	}
>  	return false;
>  }
>  
> 
> === modified file 'src/ui_basic/scrollbar.cc'
> --- src/ui_basic/scrollbar.cc	2017-05-18 21:26:29 +0000
> +++ src/ui_basic/scrollbar.cc	2017-12-09 19:38:06 +0000
> @@ -438,6 +438,75 @@
>  	return true;
>  }
>  
> +bool Scrollbar::handle_key(bool down, SDL_Keysym code) {
> +	if (down) {
> +		if (horizontal_) {
> +			switch (code.sym) {
> +			case SDLK_KP_6:
> +				if (code.mod & KMOD_NUM) {
> +					break;
> +				}
> +				FALLS_THROUGH;
> +			case SDLK_RIGHT:
> +				action(Plus);

Maybe make Area an enum class?

> +				return true;
> +
> +			case SDLK_KP_4:
> +				if (code.mod & KMOD_NUM) {
> +					break;
> +				}
> +				FALLS_THROUGH;
> +			case SDLK_LEFT:
> +				action(Minus);
> +				return true;
> +			default:
> +				break;  // not handled
> +			}
> +		} else {
> +			switch (code.sym) {
> +			case SDLK_KP_2:
> +				if (code.mod & KMOD_NUM) {
> +					break;
> +				}
> +				FALLS_THROUGH;
> +			case SDLK_DOWN:
> +				action(Plus);
> +				return true;
> +
> +			case SDLK_KP_8:
> +				if (code.mod & KMOD_NUM) {
> +					break;
> +				}
> +				FALLS_THROUGH;
> +			case SDLK_UP:
> +				action(Minus);
> +				return true;
> +
> +			case SDLK_KP_3:
> +				if (code.mod & KMOD_NUM) {
> +					break;
> +				}
> +				FALLS_THROUGH;
> +			case SDLK_PAGEDOWN:
> +				action(PlusPage);
> +				return true;
> +
> +			case SDLK_KP_9:
> +				if (code.mod & KMOD_NUM) {
> +					break;
> +				}
> +				FALLS_THROUGH;
> +			case SDLK_PAGEUP:
> +				action(MinusPage);
> +				return true;
> +			default:
> +				break;  // not handled
> +			}
> +		}
> +	}
> +	return Panel::handle_key(down, code);
> +}
> +
>  void Scrollbar::layout() {
>  	if ((2 * kSize + get_knob_size()) > static_cast<uint32_t>((horizontal_ ? get_w() : get_h()))) {
>  		buttonsize_ = kSize / 2;
> 
> === modified file 'src/wui/story_message_box.cc'
> --- src/wui/story_message_box.cc	2017-01-25 18:55:59 +0000
> +++ src/wui/story_message_box.cc	2017-12-09 19:38:06 +0000
> @@ -19,68 +19,84 @@
>  
>  #include "wui/story_message_box.h"
>  
> +#include "base/i18n.h"
>  #include "graphic/graphic.h"
> +#include "logic/game_controller.h"
> +#include "logic/save_handler.h"
>  #include "ui_basic/button.h"
>  #include "ui_basic/multilinetextarea.h"
>  #include "ui_basic/textarea.h"
> -
> -/**
> - * The message box itself
> - */
> -StoryMessageBox::StoryMessageBox(UI::Panel* const parent,
> +#include "wui/interactive_player.h"
> +
> +namespace {
> +constexpr int kPadding = 4;
> +}
> +
> +StoryMessageBox::StoryMessageBox(Widelands::Game* game,
> +											const Widelands::Coords coords,
>                                   const std::string& title,
>                                   const std::string& body,
> -                                 const std::string& button_text,
> -                                 int32_t const gposx,
> -                                 int32_t const gposy,
> +                                 int32_t const x,
> +                                 int32_t const y,
>                                   uint32_t const w,
>                                   uint32_t const h)
> -   : UI::Window(parent, "story_message_box", 0, 0, 600, 400, title.c_str()) {
> -	UI::MultilineTextarea* message_text = nullptr;
> -	int32_t const spacing = 5;
> -	int32_t offsy = 5;
> -	int32_t offsx = spacing;
> -	int32_t posx = offsx;
> -	int32_t posy = offsy;
> -
> -	set_inner_size(w, h);
> -	message_text = new UI::MultilineTextarea(
> -	   this, posx, posy, get_inner_w() - posx - spacing, get_inner_h() - posy - 2 * spacing - 50);
> -
> -	if (message_text)
> -		message_text->set_text(body);
> -
> -	int32_t const but_width = 120;
> -	int32_t space = get_inner_w() - 2 * spacing;
> -	space -= but_width;
> -	space /= 2;  // center button
> -	posx = spacing;
> -	posy = get_inner_h() - 30;
> -	posx += space;
> -	UI::Button* okbtn = new UI::Button(this, "ok", posx, posy, but_width, 20,
> -	                                   g_gr->images().get("images/ui_basic/but5.png"), button_text);
> -	okbtn->sigclicked.connect(boost::bind(&StoryMessageBox::clicked_ok, boost::ref(*this)));
> -
> -	center_to_parent();
> -
> -	if (gposx != -1)
> -		set_pos(Vector2i(gposx, get_y()));
> -	if (gposy != -1)
> -		set_pos(Vector2i(get_x(), gposy));
> -
> +   : UI::Window(game->get_ipl(), "story_message_box", x, y, w, h, title.c_str()),
> +	  main_box_(this, kPadding, kPadding, UI::Box::Vertical, 0, 0, kPadding),
> +	  button_box_(&main_box_, kPadding, kPadding, UI::Box::Horizontal, 0, 0, kPadding),
> +	  textarea_(&main_box_, 0, 0, 100, 100, ""),
> +	  ok_(&button_box_, "ok", 0, 0, 120, 0, g_gr->images().get("images/ui_basic/but5.png"), _("OK")),
> +	  desired_speed_(game->game_controller()->desired_speed()),
> +	  game_(game) {
> +
> +	// Pause the game
> +	game_->game_controller()->set_desired_speed(0);
> +	game_->save_handler().set_allow_saving(false);
> +
> +	// Adjust map view
> +	if (coords != Widelands::Coords::null()) {
> +		game_->get_ipl()->map_view()->scroll_to_field(coords, MapView::Transition::Jump);

This call is also done in the lua function above. Remove it in the lua function?

> +	}
> +
> +	// TODO(GunChleoc): When all campaigns and scenarios have been converted, simply add the body text above.
> +	try {
> +		textarea_.force_new_renderer();
> +		textarea_.set_text(body);
> +		log("Story Message Box: using NEW font renderer.\n");
> +	} catch (const std::exception& e) {
> +		log("Story Message Box: falling back to OLD font renderer:\n%s\n%s\n", body.c_str(), e.what());
> +		textarea_.force_new_renderer(false);
> +		textarea_.set_text(body);
> +	}
> +
> +
> +	// Add and configure the panels
> +	main_box_.set_size(get_inner_w() - 3 * kPadding, get_inner_h() - 2 * kPadding);
> +
> +	main_box_.add(&textarea_, UI::Box::Resizing::kExpandBoth);
> +	main_box_.add(&button_box_, UI::Box::Resizing::kFullSize);
> +	button_box_.add_inf_space();
> +	button_box_.add(&ok_);
> +	button_box_.add_inf_space();
> +
> +	ok_.sigclicked.connect(boost::bind(&StoryMessageBox::clicked_ok, boost::ref(*this)));
> +
> +	if (x == -1 && y == -1) {
> +		center_to_parent();
> +	}
>  	move_inside_parent();
> +	textarea_.focus();
>  }
>  
> -/**
> - * Clicked
> - */
>  void StoryMessageBox::clicked_ok() {
> +	// Manually force the game to reevaluate its current state, especially time information.
> +	game_->game_controller()->think();
> +	// Now get the game running again.
> +	game_->game_controller()->set_desired_speed(desired_speed_);
> +	game_->save_handler().set_allow_saving(true);
> +
>  	end_modal<UI::Panel::Returncodes>(UI::Panel::Returncodes::kOk);
>  }
>  
> -/*
> - * Avoid being closed by right click
> - */
>  bool StoryMessageBox::handle_mousepress(const uint8_t btn, int32_t mx, int32_t my) {
>  	if (btn == SDL_BUTTON_RIGHT)
>  		return true;


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