Merge lp:~mxsscott/widelands/windowed-graphics into lp:widelands

[Mark Scott]

Fix resizing of the game window after the user chooses a new resolution bug 1096651.

- Also menus use the current graphics dimensions, not the dimensions in the options (not really a problem now!)

- Some graphics resolution tidyups.

- Support Enter/Escape for triggering the Apply or Cancel buttons within the Options menu. This allows the user to reselect a smaller window size if they get stuck due to bug 900784.

The screen resolution issue fix could possibly be improved if we display a window for 5-10 seconds asking the user to confirm they can see it. If they don't click OK, we revert the graphics mode back to the previous one. However, I believe the Enter/Escape solution is 'good enough' for now.

[SirVer (sirver)]

Nice change in total. The only issue I have is const SDL_Rect * const *. Maybe it's because i am a bad programmer, but i do not even know what type this is... a const pointer to pointer to constant SDL_Rects? does const SDL_Rect** compile without casting?

I also agree with the struct -> class changes and the simplifications concerning pulling the resolutions from the conf file periodically vs taking the real resolution.

I think here

 for (/* init above */; modes && *modes; ++modes)

you could just pull the initialization into the for loop as you do not do anything with the variable anyways.

otherwise looks good to me. Btw, I really enjoy reviewing your code :-). In the past we have always granted push to trunk rights to developers who have made good contributions. Are you interested in this? Then, we also no longer did code reviews and devs just push whatever they think is appropriate - however i am interested in changing this because I'd love to have code review for my own stuff as well in the future. .... this kinda went offtopic of this merge - I'd like to hear the comments of everybody though!

review: Approve

[Mark Scott (mxsscott)]

Reading right to left, it's a pointer to a const pointer to a const SDL_Rect.

const SDL_Rect** should also work because that's what SDL_ListModes() returns (we declare our variable to be more consty to indicate we don't mess with it.

The reason I left the init out of the for loop was due to line-length, but this is a pretty poor reason to do so.

I'll make these changes and re-push the branch.

I'm happy having trunk rights - but I do see the benefit of having code reviewed, especially where it is an area people are touching significantly or haven't touched before. Even if the coding is fine, there may be opportunity to say "Why don't you do this too?", or point out something that was missed "You left a todo in for function X's documentation" - we all forget these in the haste to commit before going away to do something else (sleep?)

[Mark Scott (mxsscott)]

Wasn't able to change the type
   const SDL_Rect * const *
to
   const SDL_Rect * *.

SDL_ListModes() returns a "SDL_Rect **" so we need to say we'll keep all but the final pointer variable constant (which is fine because that variable is ours), or nothing constant.

[SirVer (sirver)]

Okay, Ack. Code LGTM, so feel free to push it.

I gave you push rights to trunk (by adding you to widelands dev). here comes the very important usual disclaimer: bzr is awefull in one regard that you can push your branch history to trunk without confirmation. that is, if you work in a branch and someone adds new revisions to trunk in the meantime, when you push your branch to trunk you will overwrite those changes!!!

so, the proper thing is to always keep a version of trunk around that you do not modify. I personally use this workflow: http://www.taoeffect.com/blog/2009/06/using-bazaar-like-git-repoalias-plugin/

you then always work in branches:

$ cd trunk && bzr switch -b bug123
hack hack, commit, commit
$ bzr switch trunk && bzr pull && bzr merge .bzr-repo/bug123
$ bzr push lp:widelands

especially never push to lp:widelands from a feature branch or before you pulled. bzr uncommit helps sometimes to undo errors.

feel free to ask for anything when you need help. this is really important to get right - though basically everybody gets it wrong once. this will result in mailbombs from bzr and "XXX revisions removed." mails which are confusing at best.

[Hans Joachim Desserud (hjd)]

>I'd love to have code review for my own stuff as well in the future. .... this kinda went offtopic of this merge - I'd like to hear the comments of everybody though!

+1
Code reviews are really nice as they can often reveal and resolve issues before they hit trunk. And if something breaks in the future, both the author and reviewer should be familiar with the code as opposed to only a single person.

One potential issue I see is of shortage of reviewers. We currently have nine branches waiting for review, some of which are quite old. On the other hand, some of those have raised issues which authors haven't addressed yet.

I think this would be worth a try. We can always re-assess later if we end up with lots of unmerged reviews and a trunk gathering dust.

And since this is completely off-topic, we should find some better place to discuss it; a forum thread, meta bug?

[SirVer (sirver)]

right, maybe we should take this to widelands-public which is still the goto place to discuss dev topics.

=== modified file 'src/sound/sound_handler.cc'
--- src/sound/sound_handler.cc	2013-01-04 16:39:48 +0000
+++ src/sound/sound_handler.cc	2013-01-07 20:42:25 +0000
@@ -330,8 +330,8 @@
 	Interactive_Base const & ibase = *m_egbase->get_ibase();
 	Point const vp = ibase.get_viewpoint();
 
-	int32_t const xres = ibase.get_xres();
-	int32_t const yres = ibase.get_yres();
+	int32_t const xres = g_gr->get_xres();
+	int32_t const yres = g_gr->get_yres();
 
 	MapviewPixelFunctions::get_pix(m_egbase->map(), position, sx, sy);
 	sx -= vp.x;
 
=== modified file 'src/ui_fsmenu/base.cc'
--- src/ui_fsmenu/base.cc	2013-01-05 21:03:10 +0000
+++ src/ui_fsmenu/base.cc	2013-01-07 20:42:25 +0000
@@ -86,11 +86,11 @@
 
 
 uint32_t Fullscreen_Menu_Base::gr_x() {
-	return g_options.pull_section("global").get_int("xres", XRES);
+	return g_gr->get_xres();
 }
 
 uint32_t Fullscreen_Menu_Base::gr_y() {
-	return g_options.pull_section("global").get_int("yres", YRES);
+	return g_gr->get_yres();
 }
 
 
 
=== modified file 'src/ui_fsmenu/options.cc'
--- src/ui_fsmenu/options.cc	2012-12-16 19:08:53 +0000
+++ src/ui_fsmenu/options.cc	2013-01-07 20:42:25 +0000
@@ -236,37 +236,47 @@
 
 	//  GRAPHIC_TODO: this shouldn't be here List all resolutions
 	// take a copy to not change real video info structure
-	SDL_PixelFormat  fmt = *SDL_GetVideoInfo()->vfmt;
+	SDL_PixelFormat fmt = *SDL_GetVideoInfo()->vfmt;
+
 	fmt.BitsPerPixel = 16;
-	if
-		(SDL_Rect const * const * const modes =
-		 	SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN))
-		for (uint32_t i = 0; modes[i]; ++i)
-			if (640 <= modes[i]->w and 480 <= modes[i]->h) {
-				res const this_res = {modes[i]->w, modes[i]->h, 16};
+	for
+		(const SDL_Rect * const * modes = SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN);
+		 modes && *modes;
+		 ++modes)
+	{
+		const SDL_Rect & mode = **modes;
+		if (640 <= mode.w and 480 <= mode.h)
+		{
+			const Resolution this_res = {mode.w, mode.h, fmt.BitsPerPixel};
 			if
 				(m_resolutions.empty()
-				 or
-				 this_res.xres != m_resolutions.rbegin()->xres
-				 or
-				 this_res.yres != m_resolutions.rbegin()->yres)
+				 || this_res.xres != m_resolutions.rbegin()->xres
+				 || this_res.yres != m_resolutions.rbegin()->yres)
+			{
 				m_resolutions.push_back(this_res);
 			}
+		}
+	}
+
 	fmt.BitsPerPixel = 32;
-	if
-		(SDL_Rect const * const * const modes =
-		 	SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN))
-		for (uint32_t i = 0; modes[i]; ++i)
-			if (640 <= modes[i]->w and 480 <= modes[i]->h) {
-				res const this_res = {modes[i]->w, modes[i]->h, 32};
-				if
-					(m_resolutions.empty()
-					 or
-					 this_res.xres != m_resolutions.rbegin()->xres
-					 or
-					 this_res.yres != m_resolutions.rbegin()->yres)
-					m_resolutions.push_back(this_res);
+	for
+		(const SDL_Rect * const * modes = SDL_ListModes(&fmt, SDL_SWSURFACE | SDL_FULLSCREEN);
+		 modes && *modes;
+		 ++modes)
+	{
+		const SDL_Rect & mode = **modes;
+		if (640 <= mode.w and 480 <= mode.h)
+		{
+			const Resolution this_res = {mode.w, mode.h, fmt.BitsPerPixel};
+			if
+				(m_resolutions.empty()
+				 || this_res.xres != m_resolutions.rbegin()->xres
+				 || this_res.yres != m_resolutions.rbegin()->yres)
+			{
+				m_resolutions.push_back(this_res);
 			}
+		}
+	}
 
 	bool did_select_a_res = false;
 	for (uint32_t i = 0; i < m_resolutions.size(); ++i) {
@@ -343,6 +353,26 @@
 	}
 }
 
+bool Fullscreen_Menu_Options::handle_key(bool down, SDL_keysym code)
+{
+	if (down)
+	{
+		switch (code.sym)
+		{
+			case SDLK_KP_ENTER:
+			case SDLK_RETURN:
+				end_modal(static_cast<int32_t>(om_ok));
+				return true;
+			case SDLK_ESCAPE:
+				end_modal(static_cast<int32_t>(om_cancel));
+				return true;
+			default:
+				break; // not handled
+		}
+	}
+
+	return Fullscreen_Menu_Base::handle_key(down, code);
+}
 
 Options_Ctrl::Options_Struct Fullscreen_Menu_Options::get_values() {
 	const uint32_t res_index = m_reslist.selection_index();
@@ -561,6 +591,28 @@
 	}
 }
 
+bool Fullscreen_Menu_Advanced_Options::handle_key(bool down, SDL_keysym code)
+{
+	if (down)
+	{
+		switch (code.sym)
+		{
+			case SDLK_KP_ENTER:
+			case SDLK_RETURN:
+				end_modal(static_cast<int32_t>(om_ok));
+				return true;
+			case SDLK_ESCAPE:
+				end_modal(static_cast<int32_t>(om_cancel));
+				return true;
+			default:
+				break; // not handled
+		}
+	}
+
+	return Fullscreen_Menu_Base::handle_key(down, code);
+}
+
+
 Options_Ctrl::Options_Struct Fullscreen_Menu_Advanced_Options::get_values() {
 	// Write all remaining data from UI elements
 	os.message_sound        = m_message_sound.get_state();
 
=== modified file 'src/ui_fsmenu/options.h'
--- src/ui_fsmenu/options.h	2012-02-15 21:25:34 +0000
+++ src/ui_fsmenu/options.h	2013-01-07 20:42:25 +0000
@@ -32,10 +32,11 @@
 #include <cstring>
 #include <vector>
 
-struct Fullscreen_Menu_Options;
+class Fullscreen_Menu_Options;
 struct Section;
 
-struct Options_Ctrl {
+class Options_Ctrl {
+public:
 	struct Options_Struct {
 		int32_t xres;
 		int32_t yres;
@@ -80,7 +81,8 @@
  * Fullscreen Optionsmenu. A modal optionsmenu
  */
 
-struct Fullscreen_Menu_Options : public Fullscreen_Menu_Base {
+class Fullscreen_Menu_Options : public Fullscreen_Menu_Base {
+public:
 	Fullscreen_Menu_Options(Options_Ctrl::Options_Struct opt);
 	Options_Ctrl::Options_Struct get_values();
 	enum {
@@ -89,6 +91,9 @@
 		om_restart = 2
 	};
 
+	/// Handle keypresses
+	virtual bool handle_key(bool down, SDL_keysym code);
+
 private:
 	uint32_t                          m_vbutw;
 	uint32_t                          m_butw;
@@ -127,19 +132,24 @@
 
 	void advanced_options();
 
-	struct res {
+	/// A screen resolution in terms of width, height and pixel depth.
+	class Resolution {
+	public:
 		int32_t xres;
 		int32_t yres;
 		int32_t depth;
 	};
-	std::vector<res> m_resolutions;
+
+	/// All supported screen resolutions.
+	std::vector<Resolution> m_resolutions;
 };
 
 /**
  * Fullscreen Optionsmenu. A modal optionsmenu
  */
 
-struct Fullscreen_Menu_Advanced_Options : public Fullscreen_Menu_Base {
+class Fullscreen_Menu_Advanced_Options : public Fullscreen_Menu_Base {
+public:
 	Fullscreen_Menu_Advanced_Options(Options_Ctrl::Options_Struct opt);
 	Options_Ctrl::Options_Struct get_values();
 	enum {
@@ -147,6 +157,9 @@
 		om_ok     =   1
 	};
 
+	/// Handle keypresses
+	virtual bool handle_key(bool down, SDL_keysym code);
+
 private:
 	uint32_t                    m_vbutw;
 	uint32_t                    m_butw;
 
=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2013-01-04 15:24:56 +0000
+++ src/wlapplication.cc	2013-01-07 20:42:25 +0000
@@ -263,7 +263,7 @@
 m_mouse_locked         (0),
 m_mouse_compensate_warp(0, 0),
 m_should_die           (false),
-m_gfx_w(0), m_gfx_h(0),
+m_gfx_w(0), m_gfx_h(0), m_gfx_bpp(0),
 m_gfx_fullscreen       (false),
 m_gfx_opengl           (true),
 m_default_datadirs     (true),
@@ -790,11 +790,11 @@
  */
 
 void WLApplication::init_graphics
-	(int32_t const w, int32_t const h, int32_t const bpp,
-	 bool const fullscreen, bool const opengl)
+	(const int32_t w, const int32_t h, const int32_t bpp,
+	 const bool fullscreen, const bool opengl)
 {
 	if
-		(w == m_gfx_w && h == m_gfx_h &&
+		(w == m_gfx_w && h == m_gfx_h && bpp == m_gfx_bpp &&
 		 fullscreen == m_gfx_fullscreen &&
 		 opengl == m_gfx_opengl)
 		return;
@@ -804,6 +804,7 @@
 
 	m_gfx_w = w;
 	m_gfx_h = h;
+	m_gfx_bpp = bpp;
 	m_gfx_fullscreen = fullscreen;
 	m_gfx_opengl = opengl;
 
@@ -815,6 +816,23 @@
 	}
 }
 
+void WLApplication::refresh_graphics()
+{
+	Section & s = g_options.pull_section("global");
+
+	//  Switch to the new graphics system now, if necessary.
+	init_graphics
+		(s.get_int("xres", XRES),
+		 s.get_int("yres", YRES),
+		 s.get_int("depth", 16),
+		 s.get_bool("fullscreen", false),
+#if USE_OPENGL
+		 s.get_bool("opengl", true));
+#else
+		 false);
+#endif
+}
+
 /**
  * Read the config file, parse the commandline and give all other internal
  * parameters sensible default values
@@ -1515,6 +1533,9 @@
 	std::string message;
 
 	for (;;) {
+		// Refresh graphics system in case we just changed resolution.
+		refresh_graphics();
+
 		Fullscreen_Menu_Main mm;
 
 		if (message.size()) {
 
=== modified file 'src/wlapplication.h'
--- src/wlapplication.h	2012-02-15 21:25:34 +0000
+++ src/wlapplication.h	2013-01-07 20:42:25 +0000
@@ -179,8 +179,15 @@
 	//@}
 
 	void init_graphics
-		(int32_t w, int32_t h, int32_t bpp,
-		 bool fullscreen, bool opengl);
+		(const int32_t w, const int32_t h, const int32_t bpp,
+		 const bool fullscreen, const bool opengl);
+
+	/**
+	 * Refresh the graphics from the latest options.
+	 *
+	 * \note See caveats for \ref init_graphics()
+	 */
+	void refresh_graphics();
 
 	void handle_input(InputCallback const *);
 
@@ -282,6 +289,9 @@
 	///The Widelands window's height in pixels
 	int32_t    m_gfx_h;
 
+	///The Widelands window's bits per pixel
+	int32_t    m_gfx_bpp;
+
 	///If true Widelands is (should be, we never know ;-) running
 	///in a fullscreen window
 	bool   m_gfx_fullscreen;
 
=== modified file 'src/wui/interactive_base.cc'
--- src/wui/interactive_base.cc	2012-12-16 14:29:46 +0000
+++ src/wui/interactive_base.cc	2013-01-07 20:42:25 +0000
@@ -71,7 +71,7 @@
 Interactive_Base::Interactive_Base
 	(Editor_Game_Base & the_egbase, Section & global_s)
 	:
-	Map_View(0, 0, 0, get_xres(), get_yres(), *this),
+	Map_View(0, 0, 0, global_s.get_int("xres", XRES), global_s.get_int("yres", YRES), *this),
 	m_show_workarea_preview(global_s.get_bool("workareapreview", true)),
 	m
 		(new InteractiveBaseInternals
@@ -112,15 +112,7 @@
 		(global_s.get_bool("dock_windows_to_edges", false));
 
 	//  Switch to the new graphics system now, if necessary.
-	WLApplication::get()->init_graphics
-		(get_xres(), get_yres(),
-		 global_s.get_int("depth", 16),
-		 global_s.get_bool("fullscreen", false),
-#if USE_OPENGL
-		 global_s.get_bool("opengl", true));
-#else
-		 false);
-#endif
+	WLApplication::get()->refresh_graphics();
 
 	//  Having this in the initializer list (before Sys_InitGraphics) will give
 	//  funny results.
@@ -238,28 +230,6 @@
 
 
 /**
- * Retrieves the configured in-game resolution.
- *
- * \note For most purposes, you should use \ref Graphic::get_xres instead.
- */
-int32_t Interactive_Base::get_xres()
-{
-	return g_options.pull_section("global").get_int("xres", XRES);
-}
-
-
-/**
- * Retrieves the configured in-game resolution.
- *
- * \note For most purposes, you should use \ref Graphic::get_yres instead.
- */
-int32_t Interactive_Base::get_yres()
-{
-	return g_options.pull_section("global").get_int("yres", YRES);
-}
-
-
-/**
  * Called by \ref Game::postload at the end of the game loading
  * sequence.
  *
 
=== modified file 'src/wui/interactive_base.h'
--- src/wui/interactive_base.h	2012-12-13 10:41:22 +0000
+++ src/wui/interactive_base.h	2013-01-07 20:42:25 +0000
@@ -113,9 +113,6 @@
 	virtual void cleanup_for_load() {};
 
 private:
-	static int32_t get_xres();
-	static int32_t get_yres();
-
 	void roadb_add_overlay   ();
 	void roadb_remove_overlay();