widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

Relying on the filenames is a no go - we have no control over these. Instead, we need to save the metadata that is encoded in the filenames into the game somehow and use it for parsing.

Diff comments:

> === modified file 'src/base/time_string.cc'
> --- src/base/time_string.cc	2014-07-05 14:22:44 +0000
> +++ src/base/time_string.cc	2014-08-02 11:13:21 +0000
> @@ -19,11 +19,16 @@
>  
>  #include "base/time_string.h"
>  
> +#include <algorithm>
>  #include <cassert>
>  #include <ctime>
> +#include <string>
>  
> +#include <boost/format.hpp>
>  #include <stdint.h>
>  
> +#include "base/i18n.h"
> +
>  namespace  {
>  char timestring_buffer[] = "YYYY-MM-DDThh.mm.ss"; //  ':' is not a valid file name character for FAT FS
>  char gamestringbuffer[] = "000:00:00";
> @@ -82,7 +87,103 @@
>  	return timestring_buffer;
>  }
>  
> -char * gametimestring(uint32_t gametime)
> +namespace  {
> +std::string localize_month(int8_t month) {
> +	std::string fallback = std::to_string(month);

only do this in the default block (i.e. return std::to_string(month))

> +	switch (month) {
> +		case 1:
> +			/** TRANSLATORS: January */
> +			return _("Jan");
> +		case 2:
> +			/** TRANSLATORS: February */
> +			return _("Feb");
> +		case 3:
> +			/** TRANSLATORS: March */
> +			return _("Mar");
> +		case 4:
> +			/** TRANSLATORS: April */
> +			return _("Apr");
> +		case 5:
> +			/** TRANSLATORS: May */
> +			return _("May");
> +		case 6:
> +			/** TRANSLATORS: June */
> +			return _("Jun");
> +		case 7:
> +			/** TRANSLATORS: July */
> +			return _("Jul");
> +		case 8:
> +			/** TRANSLATORS: August */
> +			return _("Aug");
> +		case 9:
> +			/** TRANSLATORS: September */
> +			return _("Sep");
> +		case 10:
> +			/** TRANSLATORS: October */
> +			return _("Oct");
> +		case 11:
> +			/** TRANSLATORS: November */
> +			return _("Nov");
> +		case 12:
> +			/** TRANSLATORS: December */
> +			return _("Dec");
> +		default:
> +			return fallback;
> +	}
> +}
> +}
> +
> +
> +// Locale-dependent formatting for datetime-based filenames.
> +std::string localize_timestring(std::string timestring) {

should take a const std::string&

I am not against this method, but would it not be easier to instead take all of the integers are arguments and return a correctly localized string instead? Or are you parsing filenames here (which would be fragile.)

> +
> +	std::string result = "";

no need to initialize. It will be done for you.

> +
> +	// Do some formatting if this is a string of the type "YYYY-MM-DDThh.mm.ss"
> +	// check separators
> +	if (timestring.length() >= sizeof(timestring_buffer) - 1 &&

we use .size() everywhere else.

> +		 timestring.compare(4, 1, "-") == 0 &&
> +		 timestring.compare(7, 1, "-") == 0 &&
> +		 timestring.compare(10, 1, "T") == 0 &&
> +		 timestring.compare(13, 1, ".") == 0 &&
> +		 timestring.compare(16, 1, ".") == 0) {
> +
> +		std::string year = timestring.substr(0, 4);

all of these can be const I think

> +		std::string month = timestring.substr(5, 2);
> +		std::string day = timestring.substr(8, 2);
> +		std::string hour = timestring.substr(11, 2);
> +		std::string minute = timestring.substr(14, 2);
> +		std::string second = timestring.substr(17, 2);
> +
> +		// check digits
> +		if (std::all_of(year.begin(), year.end(), ::isdigit) &&

this is easier to do with a boost regular expression. We have examples for using it in the codebase.

> +			 std::all_of(month.begin(), month.end(), ::isdigit) &&
> +			 std::all_of(day.begin(), day.end(), ::isdigit) &&
> +			 std::all_of(hour.begin(), hour.end(), ::isdigit) &&
> +			 std::all_of(minute.begin(), minute.end(), ::isdigit) &&
> +			 std::all_of(second.begin(), second.end(), ::isdigit)) {
> +
> +			month = localize_month(stoi(month));
> +
> +			/** TRANSLATORS: Date format for filenames on load game screens. YYYY Mon DD hh:mm:ss */
> +			result = (boost::format(_("%1% %2% %3% %4%:%5%:%6%"))
> +						 % day % month % year
> +						 % hour % minute % second).str();
> +
> +			if (timestring.length() > sizeof(timestring_buffer) - 1) {
> +				result.append(timestring.substr(19));
> +			}
> +		} else {
> +			result = timestring;
> +		}
> +	} else {
> +		result = timestring;
> +	}
> +	return result;
> +}
> +
> +
> +char * gametimestring_leading_zeros(uint32_t gametime)
>  {
>  	uint32_t time = gametime / 1000;
>  	gamestringbuffer[8] = '0' +  time        % 10;
> @@ -94,3 +195,25 @@
>  	gamestringbuffer[0] = '0' + (time /= 10);
>  	return gamestringbuffer;
>  }
> +
> +char * gametimestring(uint32_t gametime)

return a std::string. Otherwise you always return the same buffer and if somebody holds on to the pointer, the content might change under her.

> +{
> +	// update buffer
> +	gametimestring_leading_zeros(gametime);
> +
> +	// remove leading 0s

int i = 0;
while (gamestringbuffer[0] == '0') ++i;
return std::string(gamestringbuffer).substr(i) or something along these lines.

> +	int8_t returnindex = 0;
> +	if (gamestringbuffer[0] == '0')
> +	{
> +		returnindex++;
> +		if (gamestringbuffer[1] == '0')
> +		{
> +			returnindex++;
> +			if (gamestringbuffer[2] == '0')
> +			{
> +				returnindex = returnindex + 2;
> +			}
> +		}
> +	}
> +	return &gamestringbuffer[returnindex];
> +}
> 
> === modified file 'src/base/time_string.h'
> --- src/base/time_string.h	2014-07-05 16:41:51 +0000
> +++ src/base/time_string.h	2014-08-02 11:13:21 +0000
> @@ -20,6 +20,8 @@
>  #ifndef WL_BASE_TIME_STRING_H
>  #define WL_BASE_TIME_STRING_H
>  
> +#include <string>
> +
>  #include <stdint.h>
>  
>  /// Get a string representation conforming to ISO 8601 of the current time (in
> @@ -27,9 +29,22 @@
>  /// string which might be overwritten by subsequent calls.
>  char * timestring();
>  
> +/// Format a localized timestring for display on screen for the user,
> +/// so it is more easily read.
> +/// If the string starts with the datetime format "YYYY-MM-DDThh.mm.ss",
> +/// that part of the string is transformed to a localized datetime string.
> +/// Any other parts of the string remain as is.
> +std::string localize_timestring(std::string timestring);
> +
>  /// Get a string representation of the game time
>  /// as hhh:mm:ss. If Time represents more than
>  /// 999 hours, it wraps around
> +/// Use this in table columns for easy sorting
> +char * gametimestring_leading_zeros(uint32_t gametime);
> +
> +/// Get a string representation of the game time
> +/// as [hhh:]mm:ss. If Time represents more than
> +/// 999 hours, it wraps around
>  char * gametimestring(uint32_t gametime);
>  
>  #endif  // end of include guard: WL_BASE_TIME_STRING_H
> 
> === modified file 'src/ui_fsmenu/loadgame.cc'
> --- src/ui_fsmenu/loadgame.cc	2014-07-14 10:45:44 +0000
> +++ src/ui_fsmenu/loadgame.cc	2014-08-02 11:13:21 +0000
> @@ -311,7 +311,7 @@
>  				Widelands::Game_Loader gl(name, m_game);
>  				gl.preload_game(gpdp);
>  
> -				m_list.add(FileSystem::FS_FilenameWoExt(name).c_str(), name);
> +				m_list.add(localize_timestring(FileSystem::FS_FilenameWoExt(name)).c_str(), name);

that is fragile... what if somebody renamed the replay? Instead the replay should contain this information somehow (I think it does already, if not, it should be easy to add this to the preload_game) and you can extract it in preload_game() somehow. Otherwise your localize timestring must at least deal with this information and return the original string.

>  			} catch (const _wexception &) {
>  				//  we simply skip illegal entries
>  			}
> 
> === modified file 'src/ui_fsmenu/loadreplay.cc'
> --- src/ui_fsmenu/loadreplay.cc	2014-07-05 14:22:44 +0000
> +++ src/ui_fsmenu/loadreplay.cc	2014-08-02 11:13:21 +0000
> @@ -236,8 +236,42 @@
>  			Widelands::Game_Loader gl(savename, m_game);
>  			gl.preload_game(gpdp);
>  
> -			m_list.add
> -				(FileSystem::FS_FilenameWoExt(pname->c_str()).c_str(), *pname);
> +			// localize the filename
> +			std::string displayname = FileSystem::FS_FilenameWoExt(pname->c_str());

const

> +			std::string datetime = localize_timestring(displayname.substr(0, 19));

const.

> +
> +			// if filename contains "network player".
> +			// Format of the source string: YYYY-MM-DDThh.mm.ss network player 1 (host)
> +			// using str.at rather than str.find for efficiency
> +			if (displayname.at(23) == 'w') {
> +				// chop off "network player" to get at the player number
> +				std::string temp = displayname.substr(35);
> +				uint8_t playernumber = stoi(temp.substr(0, temp.find(' ', 0))) + 1;
> +
> +				// if filename contains "(host)"
> +				if (temp.find('h', 0) != std::string::npos) {
> +					/** TRANSLATORS: Used in filenames for "Watch Replay" */
> +					/** TRANSLATORS: %1% is a formatted date/time string */
> +					/** TRANSLATORS: %2% is the number of the player */
> +					displayname = (boost::format(_("%1% Multiplayer (Player %2%, Host)"))
> +										% datetime % static_cast<int>(playernumber)).str();
> +				} else {
> +					/** TRANSLATORS: Used in filenames for "Watch Replay" */
> +					/** TRANSLATORS: %1% is a formatted date/time string */
> +					/** TRANSLATORS: %2% is the number of the player */
> +					displayname = (boost::format(_("%1% Multiplayer (Player %2%)"))
> +										% datetime % static_cast<int>(playernumber)).str();
> +				}
> +			}
> +			// assume that filename contains "singleplayer"
> +			else {
> +				/** TRANSLATORS: Used in filenames for "Watch Replay" */
> +				/** TRANSLATORS: %1% is a formatted date/time string */
> +				displayname = (boost::format(_("%1% Single Player")) % datetime).str();
> +			}
> +
> +			// add the localized filename
> +			m_list.add(displayname.c_str(), *pname);
>  		} catch (const _wexception &) {} //  we simply skip illegal entries
>  	}
>  
> 
> === modified file 'src/wui/game_main_menu_save_game.cc'
> --- src/wui/game_main_menu_save_game.cc	2014-07-22 09:54:49 +0000
> +++ src/wui/game_main_menu_save_game.cc	2014-08-02 11:13:21 +0000
> @@ -134,8 +134,6 @@
>  		m_gametime.set_text(gametimestring(gametime));
>  
>  		int player_nr = parent.game().player_manager()->get_number_of_players();
> -		// TODO(GunChleoc): This should be ngettext(" %i player" etc. with boost::format, but it refuses to work
> -		/** TRANSLATORS: This is preceded by a number */
>  		m_players_label.set_text(
>  		   (boost::format(ngettext("%i player", "%i players", player_nr)) % player_nr).str());
>  		m_win_condition.set_text(parent.game().get_win_condition_displayname());
> @@ -157,7 +155,7 @@
>  	gl.preload_game(gpdp); //  This has worked before, no problem
>  
>  	{
> -		m_editbox->setText(FileSystem::FS_FilenameWoExt(name.c_str()));
> +		m_editbox->setText(localize_timestring(FileSystem::FS_FilenameWoExt(name.c_str())));
>  	}
>  	m_button_ok->set_enabled(true);
>  
> @@ -215,7 +213,7 @@
>  		try {
>  			Widelands::Game_Loader gl(name, igbase().game());
>  			gl.preload_game(gpdp);
> -			m_ls.add(FileSystem::FS_FilenameWoExt(name).c_str(), name);
> +			m_ls.add(localize_timestring(FileSystem::FS_FilenameWoExt(name).c_str()).c_str(), name);
>  		} catch (const _wexception &) {} //  we simply skip illegal entries
>  	}
>  }
> 
> === modified file 'src/wui/game_message_menu.cc'
> --- src/wui/game_message_menu.cc	2014-07-25 22:17:48 +0000
> +++ src/wui/game_message_menu.cc	2014-08-02 11:13:21 +0000
> @@ -205,7 +205,7 @@
>  	er.set_string(ColTitle, message.title());
>  
>  	const uint32_t time = message.sent();
> -	er.set_string(ColTimeSent, gametimestring(time));
> +	er.set_string(ColTimeSent, gametimestring_leading_zeros(time));
>  }
>  
>  /*
> 
> === modified file 'src/wui/interactive_base.cc'
> --- src/wui/interactive_base.cc	2014-07-28 14:23:03 +0000
> +++ src/wui/interactive_base.cc	2014-08-02 11:13:21 +0000
> @@ -26,6 +26,7 @@
>  #include <boost/format.hpp>
>  
>  #include "base/macros.h"
> +#include "base/time_string.h"
>  #include "economy/flag.h"
>  #include "economy/road.h"
>  #include "graphic/default_resolution.h"
> @@ -404,6 +405,17 @@
>  
>  	// Blit node information when in debug mode.
>  	if (get_display_flag(dfDebug) || !dynamic_cast<const Game*>(&egbase())) {

this will also blit in the Editor, or not? I wonder what it does there.

> +
> +		std::string gametime(gametimestring(static_cast<uint32_t>(egbase().get_gametime())));
> +
> +		const std::string gametime_text = as_uifont
> +			(gametime.c_str(), UI_FONT_SIZE_SMALL);
> +		dst.blit(
> +			Point(5, 5),
> +			UI::g_fh1->render(gametime_text),
> +			CM_Normal,
> +			UI::Align_TopLeft
> +		);
>  		static format node_format("(%i, %i)");
>  
>  		const std::string node_text = as_uifont
> @@ -423,7 +435,7 @@
>  			((fps_format %
>  			  (1000.0 / m_frametime) % (1000.0 / (m_avg_usframetime / 1000)))
>  			 .str(), UI_FONT_SIZE_SMALL);
> -		dst.blit(Point(5, 5), UI::g_fh1->render(fps_text), CM_Normal, UI::Align_Left);
> +		dst.blit(Point(5, 25), UI::g_fh1->render(fps_text), CM_Normal, UI::Align_Left);
>  	}
>  }
>  
> 


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-566729/+merge/229302
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-566729.