widelands-dev team mailing list archive

[Mark Scott <mark@xxxxxxxxx>]

Mark Scott has proposed merging lp:~hono/widelands/984197 into lp:widelands.

Commit message:
Bug 984197 - Player needs to confirm Enhance/Destroy/Dismantle actions (or Ctrl-click to live dangerously) and adjust spacing of buttons on building windows.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #984197 in widelands: "Suggestion: Confirmation window for dismantling production building"
  https://bugs.launchpad.net/widelands/+bug/984197

For more details, see:
https://code.launchpad.net/~hono/widelands/984197/+merge/141475

Bug 984197.

Separated enhance/destroy/dismantle from the rest.

As a result:
   * the Stop/Go button is on the left.
   * Enhance/Destroy/Dismantle are next, separated from any Stop/Go button by a fixed width separator.
   * Workarea/Debug/Center/Help are on the right.

Exception for Warehouses (and anything else that has no Stop/Go/Enhance/Destroy/Dismantle):
   * Workarea/Debug/Center are on the left.
   * Help is on the right.
   * Why: it looks odd having the 4 resource management controls on one line left justified, then the 3/4 other buttons on the next line right justified.


Enhance/Destroy/Dismantle now all require confirmation (mouse cursor hops to the cancel button when window is opened). Ctrl-key can be used to bypass confirmation at your peril. I have only tested left-Ctrl as I have no right-Ctrl.


Implementation details:
   * Spacing: a fixed width space was used between Stop/Go and the Enhance/Destroy/Dismantle block to keep their X position constant across buildings.
   * Confirmation windows - bulldozeconfirm.cc/h has been renamed buildingconfirm.cc/h and a base building action confirmation class created. The Bulldoze/Enhance/Dismantle windows derive from this to customize the displayed text and the think()/ok() behaviour. Note that there are new translations required for the enhance and dismantle confirmation dialogs.

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

=== renamed file 'src/wui/bulldozeconfirm.cc' => 'src/wui/buildingconfirm.cc'
--- src/wui/bulldozeconfirm.cc	2012-12-13 10:41:22 +0000
+++ src/wui/buildingconfirm.cc	2012-12-30 03:41:24 +0000
@@ -17,7 +17,7 @@
  *
  */
 
-#include "bulldozeconfirm.h"
+#include "buildingconfirm.h"
 
 #include "interactive_player.h"
 #include "logic/building.h"
@@ -30,51 +30,84 @@
 
 using boost::format;
 
+struct BuildingActionConfirm : public UI::Window {
+	BuildingActionConfirm
+		(Interactive_Player        & parent,
+		 const std::string         & windowtitle,
+		 const char *                message,
+		 Widelands::Building       & building);
+
+	Interactive_Player & iaplayer() const {
+		return ref_cast<Interactive_Player, UI::Panel>(*get_parent());
+	}
+
+	virtual void think() = 0;
+	virtual void ok() = 0;
+
+protected:
+	Widelands::Object_Ptr m_building;
+};
 
 /**
  * Confirmation dialog box for the bulldoze request for a building.
  */
-struct BulldozeConfirm : public UI::Window {
+struct BulldozeConfirm : public BuildingActionConfirm {
 	BulldozeConfirm
 		(Interactive_Player         & parent,
 		 Widelands::Building        & building,
 		 Widelands::PlayerImmovable * todestroy = 0);
 
-	Interactive_Player & iaplayer() const {
-		return ref_cast<Interactive_Player, UI::Panel>(*get_parent());
-	}
-
 	virtual void think();
+	virtual void ok();
 
 private:
-	void ok();
-
-	Widelands::Object_Ptr m_building;
 	Widelands::Object_Ptr m_todestroy;
 };
 
-/*
-===============
-Create the panels.
-If todestroy is 0, the building will be destroyed when the user confirms it.
-Otherwise, todestroy is destroyed when the user confirms it. This is useful to
-confirm building destruction when the building's base flag is removed.
-===============
-*/
-BulldozeConfirm::BulldozeConfirm
+/**
+ * Confirmation dialog box for the dismantle request for a building.
+ */
+struct DismantleConfirm : public BuildingActionConfirm {
+	DismantleConfirm
+		(Interactive_Player         & parent,
+		 Widelands::Building        & building);
+
+	virtual void think();
+	virtual void ok();
+};
+
+/**
+ * Confirmation dialog box for the enhance request for a building.
+ */
+struct EnhanceConfirm : public BuildingActionConfirm {
+	EnhanceConfirm
+		(Interactive_Player              & parent,
+		 Widelands::Building             & building,
+		 Widelands::Building_Index const & id);
+
+	virtual void think();
+	virtual void ok();
+
+private:
+    // Do not make this a reference - it is a stack variable in the caller
+	Widelands::Building_Index const m_id;
+};
+
+
+BuildingActionConfirm::BuildingActionConfirm
 	(Interactive_Player         & parent,
-	 Widelands::Building        & building,
-	 Widelands::PlayerImmovable * todestroy)
+	 const std::string          & windowtitle,
+	 const char *                 message,
+	 Widelands::Building        & building)
 	:
 	UI::Window
-		(&parent, "bulldoze_confirm", 0, 0, 200, 120, _("Destroy building?")),
-	m_building (&building),
-	m_todestroy(todestroy ? todestroy : &building)
+		(&parent, "building_action_confirm", 0, 0, 200, 120, windowtitle),
+	m_building (&building)
 {
 	new UI::Multiline_Textarea
 		(this,
 		 0, 0, 200, 74,
-		 (format(_("Do you really want to destroy this %s?")) % building.descname()).str(),
+		 (format(message) % building.descname()).str(),
 		 UI::Align_Center);
 
 	UI::Button * okbtn =
@@ -83,7 +116,7 @@
 			 6, 80, 80, 34,
 			 g_gr->imgcache().load(PicMod_UI,   "pics/but4.png"),
 			 g_gr->imgcache().load(PicMod_Game, "pics/menu_okay.png"));
-	okbtn->sigclicked.connect(boost::bind(&BulldozeConfirm::ok, this));
+	okbtn->sigclicked.connect(boost::bind(&BuildingActionConfirm::ok, this));
 
 	UI::Button * cancelbtn =
 		new UI::Button
@@ -91,7 +124,7 @@
 			 114, 80, 80, 34,
 			 g_gr->imgcache().load(PicMod_UI,   "pics/but4.png"),
 			 g_gr->imgcache().load(PicMod_Game, "pics/menu_abort.png"));
-	cancelbtn->sigclicked.connect(boost::bind(&BulldozeConfirm::die, this));
+	cancelbtn->sigclicked.connect(boost::bind(&BuildingActionConfirm::die, this));
 
 	center_to_parent();
 	cancelbtn->center_mouse();
@@ -100,6 +133,30 @@
 
 /*
 ===============
+Create the panels.
+If todestroy is 0, the building will be destroyed when the user confirms it.
+Otherwise, todestroy is destroyed when the user confirms it. This is useful to
+confirm building destruction when the building's base flag is removed.
+===============
+*/
+BulldozeConfirm::BulldozeConfirm
+	(Interactive_Player         & parent,
+	 Widelands::Building        & building,
+	 Widelands::PlayerImmovable * todestroy)
+	:
+	BuildingActionConfirm
+		(parent,
+		 _("Destroy building?"),
+		 _("Do you really want to destroy this %s?"),
+		 building),
+	m_todestroy(todestroy ? todestroy : &building)
+{
+	// Nothing special to do
+}
+
+
+/*
+===============
 Make sure the building still exists and can in fact be bulldozed.
 ===============
 */
@@ -143,6 +200,126 @@
 }
 
 
+/*
+===============
+Create the panels.
+===============
+*/
+DismantleConfirm::DismantleConfirm
+	(Interactive_Player         & parent,
+	 Widelands::Building        & building)
+	:
+	BuildingActionConfirm
+		(parent,
+		 _("Dismantle building?"),
+		 _("Do you really want to dismantle this %s?"),
+		 building)
+{
+	// Nothing special to do
+}
+
+
+/*
+===============
+Make sure the building still exists and can in fact be dismantled.
+===============
+*/
+void DismantleConfirm::think()
+{
+	Widelands::Editor_Game_Base const & egbase = iaplayer().egbase();
+	upcast(Widelands::Building,        building,  m_building .get(egbase));
+
+	if
+		(not building  ||
+		 not iaplayer().can_act(building->owner().player_number())
+		 or not
+		 (building->get_playercaps() & Widelands::Building::PCap_Dismantle))
+		die();
+}
+
+
+/**
+ * The "Ok" button was clicked, so issue the CMD_DISMANTLEBUILDING command for this building.
+ */
+void DismantleConfirm::ok()
+{
+	Widelands::Game & game   = iaplayer().game();
+	upcast(Widelands::Building,        building,    m_building.get(game));
+	upcast(Widelands::PlayerImmovable, todismantle, m_building.get(game));
+
+	if
+		(building &&
+		 iaplayer().can_act(building->owner().player_number()) and
+		 (building->get_playercaps() & Widelands::Building::PCap_Dismantle))
+	{
+		game.send_player_dismantle(*todismantle);
+		iaplayer().need_complete_redraw();
+	}
+
+	die();
+}
+
+
+/*
+===============
+Create the panels.
+===============
+*/
+EnhanceConfirm::EnhanceConfirm
+	(Interactive_Player              & parent,
+	 Widelands::Building             & building,
+	 Widelands::Building_Index const & id)
+	:
+	BuildingActionConfirm
+		(parent,
+		 _("Enhance building?"),
+		 _("Do you really want to enhance this %s?"),
+		 building),
+	m_id(id)
+{
+	// Nothing special to do
+}
+
+
+/*
+===============
+Make sure the building still exists and can in fact be enhanced.
+===============
+*/
+void EnhanceConfirm::think()
+{
+	Widelands::Editor_Game_Base const & egbase = iaplayer().egbase();
+	upcast(Widelands::Building, building, m_building .get(egbase));
+
+	if
+		(not building  ||
+		 not iaplayer().can_act(building->owner().player_number())
+		 or not
+		 (building->get_playercaps() & Widelands::Building::PCap_Enhancable))
+		die();
+}
+
+
+/**
+ * The "Ok" button was clicked, so issue the CMD_ENHANCEBUILDING command for this building.
+ */
+void EnhanceConfirm::ok()
+{
+	Widelands::Game & game   = iaplayer().game();
+	upcast(Widelands::Building,        building,  m_building.get(game));
+
+	if
+		(building &&
+		 iaplayer().can_act(building->owner().player_number()) and
+		 (building->get_playercaps() & Widelands::Building::PCap_Enhancable))
+	{
+		game.send_player_enhance_building(*building, m_id);
+		iaplayer().need_complete_redraw();
+	}
+
+	die();
+}
+
 /**
  * Create a BulldozeConfirm window.
  * No further action is required by the caller: any action necessary to actually
@@ -161,3 +338,33 @@
 {
 	new BulldozeConfirm(player, building, todestroy);
 }
+
+/**
+ * Create a DismantleConfirm window.
+ * No further action is required by the caller: any action necessary to actually
+ * dismantle the building if the user confirms is taken automatically.
+ *
+ * \param building this is the building that the confirmation dialog displays.
+ */
+void show_dismantle_confirm
+	(Interactive_Player         &       player,
+	 Widelands::Building        &       building)
+{
+	new DismantleConfirm(player, building);
+}
+
+/**
+ * Create a EnhanceConfirm window.
+ * No further action is required by the caller: any action necessary to actually
+ * enhance the building if the user confirms is taken automatically.
+ *
+ * \param building this is the building that the confirmation dialog displays.
+ * \param id building ID
+ */
+void show_enhance_confirm
+	(Interactive_Player              & player,
+	 Widelands::Building             & building,
+	 Widelands::Building_Index const & id)
+{
+	new EnhanceConfirm(player, building, id);
+}

=== renamed file 'src/wui/bulldozeconfirm.h' => 'src/wui/buildingconfirm.h'
--- src/wui/bulldozeconfirm.h	2012-02-15 21:25:34 +0000
+++ src/wui/buildingconfirm.h	2012-12-30 03:41:24 +0000
@@ -17,13 +17,14 @@
  *
  */
 
-#ifndef _BULLDOZECONFIRM_H_
-#define _BULLDOZECONFIRM_H_
+#ifndef _BUILDINGCONFIRM_H_
+#define _BUILDINGCONFIRM_H_
 
 struct Interactive_Player;
 
 namespace Widelands {
 class Building;
+struct Building_Index;
 struct PlayerImmovable;
 }
 
@@ -34,8 +35,11 @@
 
 void show_dismantle_confirm
 	(Interactive_Player & player,
+	 Widelands::Building & building);
+
+void show_enhance_confirm
+	(Interactive_Player & player,
 	 Widelands::Building & building,
-	 Widelands::PlayerImmovable * const todestroy = 0);
-
-
-#endif // _BULLDOZECONFIRM_H_
+	 Widelands::Building_Index const & id);
+
+#endif // _BUILDINGCONFIRM_H_

=== modified file 'src/wui/buildingwindow.cc'
--- src/wui/buildingwindow.cc	2012-12-17 20:01:04 +0000
+++ src/wui/buildingwindow.cc	2012-12-30 03:41:24 +0000
@@ -17,7 +17,7 @@
  *
  */
 
-#include "bulldozeconfirm.h"
+#include "buildingconfirm.h"
 #include "game_debug_ui.h"
 #include "graphic/picture.h"
 #include "graphic/rendertarget.h"
@@ -172,6 +172,7 @@
 	bool const can_see = igbase().can_see(owner_number);
 	bool const can_act = igbase().can_act(owner_number);
 
+	bool requires_destruction_separator = false;
 	if (can_act) {
 		if (upcast(Widelands::ProductionSite const, productionsite, &m_building))
 			if (not dynamic_cast<Widelands::MilitarySite const *>(productionsite)) {
@@ -188,6 +189,13 @@
 				capsbuttons->add
 					(stopbtn,
 					 UI::Box::AlignCenter);
+
+				// Add a fixed width separator rather than infinite space so the
+				// enhance/destroy/dismantle buttons are fixed in their position
+				// and not subject to the number of buttons on the right of the
+				// panel.
+				UI::Panel * spacer = new UI::Panel(capsbuttons, 0, 0, 17, 34);
+				capsbuttons->add(spacer, UI::Box::AlignCenter);
 			}
 
 		if (m_capscache & Widelands::Building::PCap_Enhancable) {
@@ -225,6 +233,7 @@
 					capsbuttons->add
 						(enhancebtn,
 						 UI::Box::AlignCenter);
+					requires_destruction_separator = true;
 				}
 		}
 
@@ -240,6 +249,8 @@
 			capsbuttons->add
 				(destroybtn,
 				 UI::Box::AlignCenter);
+
+			requires_destruction_separator = true;
 		}
 
 		if (m_capscache & Widelands::Building::PCap_Dismantle) {
@@ -262,6 +273,16 @@
 			capsbuttons->add
 				(dismantlebtn,
 				 UI::Box::AlignCenter);
+
+			requires_destruction_separator = true;
+		}
+
+		if (requires_destruction_separator and can_see) {
+			// Need this as well as the infinite space from the can_see section
+			// to ensure there is a separation.
+			UI::Panel * spacer = new UI::Panel(capsbuttons, 0, 0, 17, 34);
+			capsbuttons->add(spacer, UI::Box::AlignCenter);
+			capsbuttons->add_inf_space();
 		}
 	}
 
@@ -305,7 +326,12 @@
 			 UI::Box::AlignCenter);
 
 		if (m_building.descr().has_help_text()) {
-			capsbuttons->add_inf_space();
+			if (not requires_destruction_separator) {
+				// When there was no separation of destruction buttons put
+				// the infinite space here (e.g. Warehouses)
+				capsbuttons->add_inf_space();
+			}
+
 			UI::Button * helpbtn =
 				new UI::Button
 					(capsbuttons, "help", 0, 0, 34, 34,
@@ -345,7 +371,13 @@
 */
 void Building_Window::act_bulldoze()
 {
-	show_bulldoze_confirm(ref_cast<Interactive_Player, Interactive_GameBase>(igbase()), m_building);
+	if (get_key_state(SDLK_LCTRL) or get_key_state(SDLK_RCTRL)) {
+		if (m_building.get_playercaps() & Widelands::Building::PCap_Bulldoze)
+			igbase().game().send_player_bulldoze(m_building);
+	}
+	else {
+		show_bulldoze_confirm(ref_cast<Interactive_Player, Interactive_GameBase>(igbase()), m_building);
+	}
 }
 
 /*
@@ -355,8 +387,13 @@
 */
 void Building_Window::act_dismantle()
 {
-	if (m_building.get_playercaps() & Widelands::Building::PCap_Dismantle)
-		igbase().game().send_player_dismantle(m_building);
+	if (get_key_state(SDLK_LCTRL) or get_key_state(SDLK_RCTRL)) {
+		if (m_building.get_playercaps() & Widelands::Building::PCap_Dismantle)
+			igbase().game().send_player_dismantle(m_building);
+	}
+	else {
+		show_dismantle_confirm(ref_cast<Interactive_Player, Interactive_GameBase>(igbase()), m_building);
+	}
 }
 
 void Building_Window::act_start_stop() {
@@ -368,15 +405,21 @@
 
 /*
 ===============
-Callback for bulldozing request
+Callback for enhancement request
 ===============
 */
 void Building_Window::act_enhance(Widelands::Building_Index const id)
 {
-	if (m_building.get_playercaps() & Widelands::Building::PCap_Enhancable)
-		igbase().game().send_player_enhance_building (m_building, id);
-
-	die();
+	if (get_key_state(SDLK_LCTRL) or get_key_state(SDLK_RCTRL)) {
+		if (m_building.get_playercaps() & Widelands::Building::PCap_Enhancable)
+			igbase().game().send_player_enhance_building(m_building, id);
+	}
+	else {
+		show_enhance_confirm
+			(ref_cast<Interactive_Player, Interactive_GameBase>(igbase()),
+			 m_building,
+			 id);
+	}
 }
 
 /*