[Commits] [wesnoth/wesnoth] e3970e: gui: Fix theme UI buttons missing icons or toggle ...

[GitHub]

  Branch: refs/heads/1.12
  Home:   https://github.com/wesnoth/wesnoth
  Commit: e3970e8cb4c634f2dba676dce1865feabd2582c9
      https://github.com/wesnoth/wesnoth/commit/e3970e8cb4c634f2dba676dce1865feabd2582c9
  Author: Ignacio R. Morelle <shadowm at wesnoth.org>
  Date:   2016-05-16 (Mon, 16 May 2016)

  Changed paths:
    M changelog
    M players_changelog
    M src/display.cpp
    M src/widgets/button.cpp
    M src/widgets/button.hpp

  Log Message:
  -----------
  gui: Fix theme UI buttons missing icons or toggle state upon recreation

This is a blanket fix for the issue I previously tried to fix for
specific cases in the following two commits:

 * 1df73b9e4c563b3a700692b5db1021e23109b1c0 Call display::draw() before setting theme UI button states
 * f93b439e13731a49c09658a894a12a364af0b608 Call set_button_state() from playsingle_controller::init_gui()

This final version of the fix is far superior as it doesn't involve
sloppy guesswork regarding the various display class redraw methods and
their call sites. Instead, we just copy the previous state for each
button during reconstruction regardless of the situation at hand. We
know they are the right buttons because they have the same id strings.

It is a little more involved than I would've liked because it has to
deal with a quirk in gui::button's implementation that becomes more
evident with this fix. The following primarily applies to
gui::button::load_images():

gui::button generates its own built-in image size filename suffix for
the button overlays and frames when the button dimensions are already
known, usually after a method call that sets the button's geometry with
a valid size (i.e. not -1234 x -1234). button::set_overlay() happens to
be such a method, but not all buttons use it to set their initial
overlay, and that's where trouble begins...

 * Some theme UI buttons (mostly in the editor, and the Draw Terrains
   and Draw Units toggles on the minimap area in-game) have their
   overlay icon paths set in the WML, including their _25 size suffix.
   These have their overlays (and frames) set initially set by the
   gui::button ctor, which loads the images and figures out the button
   dimensions in the process (via button::load_images()). The ctor
   always does this unconditionally.

 * At that time, button::load_images() will not add an engine-defined
   suffix because the button dimensions are still unknown until it
   finishes. The images are loaded successfully.

 * Subsequent calls to button::set_overlay() *with the same path* will
   have button::load_images() append its own suffix determined from the
   previously determined button dimensions. Hilarity ensues.

The hotkeys engine (via hotkey::command_executor::set_button_state())
and the theme WML have different notions of which button icons have a
suffix defined externally to gui::button. This results in situations
where restoring the previous icons in display::create_buttons() causes
gui::button to look for inexistent images with duplicate size suffixes
("_25_25"). Although modifying the mainline WML to avoid using those
suffixes in the first place would be the most elegant solution, it also
has the potential to cause issues with UMC themes (esp. forks of the
mainine themes), even if those are relatively rare. Moreover, it counts
as a violation of the stable branch ban on API changes.

In other words, even if this part of the fix is very ugly, it's
ultimately necessary if we are to fix the original bug in a stable
branch without breaking the theme WML API contract. This is also why
everything is in a single commit instead of split in two.

Also note that the aforementioned commits aren't fully obsolete; the
most recent one is actually still required so that the hotkeys engine
gets to set the button icon overlays at least once (for those buttons
which don't have this defined by WML) before running WML events. Any
redundancy arising from keeping them wouldn't hurt anyway.

Finally, all this has been rendered irrelevant in master by Aginor's
refactoring and bug fixes, so this code is a technological dead end
anyway.