Merge lp:~widelands-dev/widelands/bug-1610507-disallowed-filename-characters into lp:widelands

[GunChleoc]

Added tooltip to file save and make directory edit boxes if illegal filename is being entered. Also, some refactoring:

- New function as_listitem in text layout
- Fixed tooltip drawing for modal panels
- Some code style tweaks to Panel
- Filename checks now live directly in Filesystem

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3534. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/382014340.
Appveyor build 3338. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1610507_disallowed_filename_characters-3338.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3540. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/382500797.
Appveyor build 3344. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1610507_disallowed_filename_characters-3344.

[Notabilis (notabilis27)]

Nice change, I like the tooltip! Works as intended and code is looking good.

Some remarks:
- Maybe add the tooltip to the OK button as well? At least for me that would be the more likely place where I would notice it.
- Is the smaller font for the list in the tooltip intentional?
- Maybe calculate the tooltip text only once and store it as static string in the file system class?

Not really related: I noticed that in the editor->make directory dialog the OK button is initially disabled, even though it should (?) be enabled. This is done intentionally by calling set_enabled(false) when creating the dialog, even though the directory name is valid and can be used. Is there a reason for disabling it?

review: Approve

[GunChleoc (gunchleoc)]

It is disabled because the text is always empty at the start, and empty names aren't allowed.

I like your suggestions and we should implement them before merging this.

The smaller text in the list is intentional - I have another branch for the editor info tool that uses the new listitem function too, and I wanted a consistent look.

[Notabilis (notabilis27)]

In the case of the "make directory" dialog the default value of the edit box is "unnamed". This is a valid name (changing and changing it back is accepted) but the OK button is initially disabled anyway.

[GunChleoc (gunchleoc)]

OK, that's a bug then.

[GunChleoc (gunchleoc)]

OK button is fixed. Thanks for the review and testing :)

@bunnybot merge

=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc	2018-05-22 06:54:12 +0000
+++ src/editor/ui_menus/main_menu_save_map.cc	2018-05-31 17:12:19 +0000
@@ -68,7 +68,8 @@
                    UI::ButtonStyle::kWuiPrimary,
                    _("Map Options")),
      editbox_label_(
-        this, padding_, tabley_ + tableh_ + 3 * padding_, butw_, buth_, _("Filename:")) {
+        this, padding_, tabley_ + tableh_ + 3 * padding_, butw_, buth_, _("Filename:")),
+	 illegal_filename_tooltip_(FileSystem::illegal_filename_tooltip()) {
 	set_current_directory(curdir_);
 
 	// Make room for edit_options_ button
@@ -220,7 +221,9 @@
  */
 void MainMenuSaveMap::edit_box_changed() {
 	// Prevent the user from creating nonsense file names, like e.g. ".." or "...".
-	ok_.set_enabled(LayeredFileSystem::is_legal_filename(editbox_->text()));
+	const bool is_legal_filename = FileSystem::is_legal_filename(editbox_->text());
+	ok_.set_enabled(is_legal_filename);
+	editbox_->set_tooltip(is_legal_filename ? "" : illegal_filename_tooltip_);
 }
 
 void MainMenuSaveMap::reset_editbox_or_die(const std::string& current_filename) {

=== modified file 'src/editor/ui_menus/main_menu_save_map.h'
--- src/editor/ui_menus/main_menu_save_map.h	2018-05-10 15:57:43 +0000
+++ src/editor/ui_menus/main_menu_save_map.h	2018-05-31 17:12:19 +0000
@@ -55,6 +55,7 @@
 
 	UI::Textarea editbox_label_;
 	UI::EditBox* editbox_;
+	const std::string illegal_filename_tooltip_;
 };
 
 #endif  // end of include guard: WL_EDITOR_UI_MENUS_MAIN_MENU_SAVE_MAP_H

=== modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.cc'
--- src/editor/ui_menus/main_menu_save_map_make_directory.cc	2018-04-27 06:11:05 +0000
+++ src/editor/ui_menus/main_menu_save_map_make_directory.cc	2018-05-31 17:12:19 +0000
@@ -54,7 +54,8 @@
                     butw_,
                     buth_,
                     UI::ButtonStyle::kWuiSecondary,
-                    _("Cancel")) {
+                    _("Cancel")),
+	 illegal_filename_tooltip_(FileSystem::illegal_filename_tooltip()) {
 
 	vbox_.add(&label_);
 	vbox_.add_space(padding_);
@@ -66,7 +67,7 @@
 	ok_button_.sigclicked.connect(
 	   boost::bind(&MainMenuSaveMapMakeDirectory::end_modal<UI::Panel::Returncodes>,
 	               boost::ref(*this), UI::Panel::Returncodes::kOk));
-	ok_button_.set_enabled(false);
+	ok_button_.set_enabled(!dirname_.empty());
 	cancel_button_.sigclicked.connect(
 	   boost::bind(&MainMenuSaveMapMakeDirectory::end_modal<UI::Panel::Returncodes>,
 	               boost::ref(*this), UI::Panel::Returncodes::kBack));
@@ -80,6 +81,8 @@
 void MainMenuSaveMapMakeDirectory::edit_changed() {
 	const std::string& text = edit_.text();
 	// Prevent the user from creating nonsense directory names, like e.g. ".." or "...".
-	ok_button_.set_enabled(LayeredFileSystem::is_legal_filename(text));
+	const bool is_legal_filename = FileSystem::is_legal_filename(text);
+	ok_button_.set_enabled(is_legal_filename);
+	edit_.set_tooltip(is_legal_filename ? "" : illegal_filename_tooltip_);
 	dirname_ = text;
 }

=== modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.h'
--- src/editor/ui_menus/main_menu_save_map_make_directory.h	2018-04-07 16:59:00 +0000
+++ src/editor/ui_menus/main_menu_save_map_make_directory.h	2018-05-31 17:12:19 +0000
@@ -50,6 +50,7 @@
 	UI::EditBox edit_;
 	UI::Button ok_button_;
 	UI::Button cancel_button_;
+	const std::string illegal_filename_tooltip_;
 	void edit_changed();
 };
 

=== modified file 'src/graphic/text_layout.cc'
--- src/graphic/text_layout.cc	2018-05-05 17:10:37 +0000
+++ src/graphic/text_layout.cc	2018-05-31 17:12:19 +0000
@@ -141,6 +141,13 @@
 	return f.str();
 }
 
+/// Bullet list item
+std::string as_listitem(const std::string& txt, int ptsize, const RGBColor& clr) {
+	static boost::format f("<div width=100%%><div><p><font size=%d color=%s>•</font></p></div><div><p><space gap=6></p></div><div width=*><p><font size=%d color=%s>%s<vspace gap=6></font></p></div></div>");
+	f % ptsize % clr.hex_value() % ptsize % clr.hex_value() % txt;
+	return f.str();
+}
+
 std::string as_richtext(const std::string& txt) {
 	static boost::format f("<rt>%s</rt>");
 	f % txt;

=== modified file 'src/graphic/text_layout.h'
--- src/graphic/text_layout.h	2018-04-07 16:59:00 +0000
+++ src/graphic/text_layout.h	2018-05-31 17:12:19 +0000
@@ -84,6 +84,10 @@
                        const RGBColor& clr = UI_FONT_CLR_FG,
                        UI::FontSet::Face face = UI::FontSet::Face::kSans);
 
+std::string as_listitem(const std::string&,
+                      int ptsize = UI_FONT_SIZE_SMALL,
+                      const RGBColor& clr = UI_FONT_CLR_FG);
+
 std::string as_richtext(const std::string&);
 std::string as_tooltip(const std::string&);
 std::string as_waresinfo(const std::string&);

=== modified file 'src/io/CMakeLists.txt'
--- src/io/CMakeLists.txt	2016-02-06 11:11:24 +0000
+++ src/io/CMakeLists.txt	2018-05-31 17:12:19 +0000
@@ -23,24 +23,3 @@
     io_filesystem
     io_stream
 )
-
-wl_library(io_filesystem
-  SRCS
-    filesystem/disk_filesystem.cc
-    filesystem/disk_filesystem.h
-    filesystem/filesystem.cc
-    filesystem/filesystem.h
-    filesystem/filesystem_exceptions.h
-    filesystem/layered_filesystem.cc
-    filesystem/layered_filesystem.h
-    filesystem/zip_exceptions.h
-    filesystem/zip_filesystem.cc
-    filesystem/zip_filesystem.h
-  DEPENDS
-    base_exceptions
-    base_log
-    base_macros
-    io_fileread
-    io_stream
-    third_party_minizip
-)

=== modified file 'src/io/filesystem/CMakeLists.txt'
--- src/io/filesystem/CMakeLists.txt	2012-02-15 21:25:34 +0000
+++ src/io/filesystem/CMakeLists.txt	2018-05-31 17:12:19 +0000
@@ -1,1 +1,24 @@
 add_subdirectory(test)
+
+wl_library(io_filesystem
+  SRCS
+    disk_filesystem.cc
+    disk_filesystem.h
+    filesystem.cc
+    filesystem.h
+    filesystem_exceptions.h
+    layered_filesystem.cc
+    layered_filesystem.h
+    zip_exceptions.h
+    zip_filesystem.cc
+    zip_filesystem.h
+  DEPENDS
+    base_exceptions
+    base_i18n
+    base_log
+    base_macros
+    graphic_text_layout
+    io_fileread
+    io_stream
+    third_party_minizip
+)

=== modified file 'src/io/filesystem/filesystem.cc'
--- src/io/filesystem/filesystem.cc	2018-05-12 04:17:53 +0000
+++ src/io/filesystem/filesystem.cc	2018-05-31 17:12:19 +0000
@@ -32,6 +32,9 @@
 #include <string>
 #include <vector>
 
+// We have to add Boost to this block to make codecheck happy
+#include <boost/algorithm/string/predicate.hpp>
+#include <boost/format.hpp>
 #ifdef _WIN32
 #include <direct.h>
 #include <io.h>
@@ -43,8 +46,10 @@
 #include <sys/stat.h>
 #include <unistd.h>
 
+#include "base/i18n.h"
 #include "base/log.h"
 #include "config.h"
+#include "graphic/text_layout.h"
 #include "io/filesystem/disk_filesystem.h"
 #include "io/filesystem/layered_filesystem.h"
 #include "io/filesystem/zip_exceptions.h"
@@ -60,6 +65,34 @@
 #define PATH_MAX MAX_PATH
 #endif
 
+
+namespace {
+// Characters that are allowed in filenames, but not at the beginning
+const std::vector<std::string> illegal_filename_starting_characters{
+	".",
+	"-",
+	" ", // Keep the blank last
+};
+
+// Characters that are disallowed anywhere in a filename
+// No potential file separators or other potentially illegal characters
+// https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
+// http://www.linfo.org/file_name.html
+// https://support.apple.com/en-us/HT202808
+// We can't just regex for word & digit characters here because of non-Latin scripts.
+const std::vector<std::string> illegal_filename_characters{
+	"<",
+	">",
+	":",
+	"\"",
+	"|",
+	"?",
+	"*",
+	"/",
+	"\\",
+};
+} // namespace
+
 /**
  * \param path A file or directory name
  * \return True if ref path is absolute and within this FileSystem, false otherwise
@@ -148,6 +181,43 @@
 #endif
 }
 
+bool FileSystem::is_legal_filename(const std::string& filename) {
+	if (filename.empty()) {
+		return false;
+	}
+	for (const std::string& illegal_start : illegal_filename_starting_characters) {
+		if (boost::starts_with(filename, illegal_start)) {
+			return false;
+		}
+	}
+	for (const std::string& illegal_char : illegal_filename_characters) {
+		if (boost::contains(filename, illegal_char)) {
+			return false;
+		}
+	}
+	return true;
+}
+
+std::string FileSystem::illegal_filename_tooltip() {
+	std::vector<std::string> starting_characters;
+	for (const std::string& character : illegal_filename_starting_characters) {
+		if (character == " ") {
+			/** TRANSLATORS: Part of tooltip entry for characters in illegal filenames. replaces tha blank space in a list of illegal characters */
+			starting_characters.push_back(pgettext("illegal_filename_characters", "blank space"));
+		} else {
+			starting_characters.push_back(character);
+		}
+	}
+	/** TRANSLATORS: Tooltip entry for characters in illegal filenames. %s is a list of illegal characters */
+	const std::string illegal_start(as_listitem((boost::format(pgettext("illegal_filename_characters", "%s at the start of the filename")) % richtext_escape(i18n::localize_list(starting_characters, i18n::ConcatenateWith::OR))).str(), UI_FONT_SIZE_MESSAGE));
+
+	/** TRANSLATORS: Tooltip entry for characters in illegal filenames. %s is a list of illegal characters */
+	const std::string illegal(as_listitem((boost::format(pgettext("illegal_filename_characters", "%s anywhere in the filename")) % richtext_escape(i18n::localize_list(illegal_filename_characters, i18n::ConcatenateWith::OR))).str(), UI_FONT_SIZE_MESSAGE));
+
+	/** TRANSLATORS: Tooltip header for characters in illegal filenames. This is followed by a list of bullet points */
+	return (boost::format("%s%s%s") % pgettext("illegal_filename_characters", "The following characters are not allowed:") % illegal_start % illegal).str();
+}
+
 // TODO(unknown): Write homedir detection for non-getenv-systems
 std::string FileSystem::get_homedir() {
 	std::string homedir;

=== modified file 'src/io/filesystem/filesystem.h'
--- src/io/filesystem/filesystem.h	2018-04-07 16:59:00 +0000
+++ src/io/filesystem/filesystem.h	2018-05-31 17:12:19 +0000
@@ -108,6 +108,10 @@
 	std::string canonicalize_name(std::string path) const;
 	bool is_path_absolute(const std::string& path) const;
 
+	/// Returns true if the filename is legal in all operating systems
+	static bool is_legal_filename(const std::string& filename);
+	static std::string illegal_filename_tooltip();
+
 	// Returns the path separator, i.e. \ on windows and / everywhere else.
 	static char file_separator();
 

=== modified file 'src/io/filesystem/layered_filesystem.cc'
--- src/io/filesystem/layered_filesystem.cc	2018-04-07 16:59:00 +0000
+++ src/io/filesystem/layered_filesystem.cc	2018-05-31 17:12:19 +0000
@@ -22,16 +22,12 @@
 #include <cstdio>
 #include <memory>
 
-#include <boost/algorithm/string/predicate.hpp>
-#include <boost/regex.hpp>
-
 #include "base/log.h"
 #include "base/wexception.h"
 #include "io/fileread.h"
 #include "io/streamread.h"
 
 LayeredFileSystem* g_fs;
-
 LayeredFileSystem::LayeredFileSystem() : home_(nullptr) {
 }
 
@@ -41,18 +37,6 @@
 LayeredFileSystem::~LayeredFileSystem() {
 }
 
-bool LayeredFileSystem::is_legal_filename(const std::string& filename) {
-	// No potential file separators or other potentially illegal characters
-	// https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
-	// http://www.linfo.org/file_name.html
-	// https://support.apple.com/en-us/HT202808
-	// We can't just regex for word & digit characters here because of non-Latin scripts.
-	boost::regex re(".*[<>:\"|?*/\\\\].*");
-	return !filename.empty() && !boost::starts_with(filename, ".") &&
-	       !boost::starts_with(filename, " ") && !boost::starts_with(filename, "-") &&
-	       !boost::regex_match(filename, re);
-}
-
 /**
  * Just assume that at least one of our child FSs is writable
  */

=== modified file 'src/io/filesystem/layered_filesystem.h'
--- src/io/filesystem/layered_filesystem.h	2018-04-07 16:59:00 +0000
+++ src/io/filesystem/layered_filesystem.h	2018-05-31 17:12:19 +0000
@@ -58,8 +58,7 @@
 
 	std::set<std::string> list_directory(const std::string& path) override;
 
-	/// Returns true if the filename is legal in all operating systems
-	static bool is_legal_filename(const std::string& filename);
+
 	bool is_writable() const override;
 	bool file_exists(const std::string& path) override;
 	bool is_directory(const std::string& path) override;

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc	2018-05-14 06:59:39 +0000
+++ src/ui_basic/panel.cc	2018-05-31 17:12:19 +0000
@@ -143,8 +143,9 @@
 	app->set_mouse_lock(false);  // more paranoia :-)
 
 	Panel* forefather = this;
-	while (Panel* const p = forefather->parent_)
-		forefather = p;
+	while (forefather->parent_ != nullptr) {
+		forefather = forefather->parent_;
+	}
 
 	default_cursor_ = g_gr->images().get("images/ui_basic/cursor.png");
 	default_cursor_click_ = g_gr->images().get("images/ui_basic/cursor_click.png");
@@ -175,13 +176,16 @@
 		app->handle_input(&input_callback);
 
 		if (start_time >= next_think_time) {
-			if (app->should_die())
+			if (app->should_die()) {
 				end_modal<Returncodes>(Returncodes::kBack);
+			}
 
 			do_think();
 
-			if (flags_ & pf_child_die)
+			if (flags_ & pf_child_die) {
 				check_child_death();
+			}
+
 			next_think_time = start_time + kGameLogicDelay;
 		}
 
@@ -190,7 +194,13 @@
 			forefather->do_draw(rt);
 			rt.blit((app->get_mouse_position() - Vector2i(3, 7)),
 			        app->is_mouse_pressed() ? default_cursor_click_ : default_cursor_);
-			forefather->do_tooltip();
+
+			if (is_modal()) {
+				do_tooltip();
+			} else {
+				forefather->do_tooltip();
+			}
+
 			g_gr->refresh();
 			next_draw_time = start_time + draw_delay;
 		}
@@ -594,8 +604,7 @@
  * false otherwise.
  */
 bool Panel::handle_tooltip() {
-	RenderTarget& rt = *g_gr->get_render_target();
-	return draw_tooltip(rt, tooltip());
+	return draw_tooltip(tooltip());
 }
 
 /**
@@ -1060,13 +1069,15 @@
 /**
  * Draw the tooltip. Return true on success
  */
-bool Panel::draw_tooltip(RenderTarget& dst, const std::string& text) {
+bool Panel::draw_tooltip(const std::string& text) {
 	if (text.empty()) {
 		return false;
 	}
+
+	RenderTarget& dst = *g_gr->get_render_target();
 	std::string text_to_render = text;
 	if (!is_richtext(text_to_render)) {
-		text_to_render = as_tooltip(text);
+		text_to_render = as_tooltip(text_to_render);
 	}
 
 	constexpr uint32_t kTipWidthMax = 360;

=== modified file 'src/ui_basic/panel.h'
--- src/ui_basic/panel.h	2018-04-27 06:11:05 +0000
+++ src/ui_basic/panel.h	2018-05-31 17:12:19 +0000
@@ -311,7 +311,7 @@
 	static void play_new_chat_member();
 	static void play_new_chat_message();
 
-	static bool draw_tooltip(RenderTarget&, const std::string& text);
+	static bool draw_tooltip(const std::string& text);
 	void draw_background(RenderTarget& dst, const UI::PanelStyleInfo&);
 	void draw_background(RenderTarget& dst, Recti rect, const UI::PanelStyleInfo&);
 

=== modified file 'src/wui/game_main_menu_save_game.cc'
--- src/wui/game_main_menu_save_game.cc	2018-05-22 06:54:12 +0000
+++ src/wui/game_main_menu_save_game.cc	2018-05-31 17:12:19 +0000
@@ -65,7 +65,8 @@
      cancel_(&buttons_box_, "cancel", 0, 0, 0, 0, UI::ButtonStyle::kWuiSecondary, _("Cancel")),
      ok_(&buttons_box_, "ok", 0, 0, 0, 0, UI::ButtonStyle::kWuiPrimary, _("OK")),
 
-     curdir_(kSaveDir) {
+     curdir_(kSaveDir),
+	 illegal_filename_tooltip_(FileSystem::illegal_filename_tooltip()) {
 
 	layout();
 
@@ -133,8 +134,9 @@
 
 void GameMainMenuSaveGame::edit_box_changed() {
 	// Prevent the user from creating nonsense directory names, like e.g. ".." or "...".
-	const bool is_legal_filename = LayeredFileSystem::is_legal_filename(filename_editbox_.text());
+	const bool is_legal_filename = FileSystem::is_legal_filename(filename_editbox_.text());
 	ok_.set_enabled(is_legal_filename);
+	filename_editbox_.set_tooltip(is_legal_filename ? "" : illegal_filename_tooltip_);
 	load_or_save_.delete_button()->set_enabled(false);
 	load_or_save_.clear_selections();
 }

=== modified file 'src/wui/game_main_menu_save_game.h'
--- src/wui/game_main_menu_save_game.h	2018-05-10 15:57:43 +0000
+++ src/wui/game_main_menu_save_game.h	2018-05-31 17:12:19 +0000
@@ -79,6 +79,7 @@
 	UI::Button cancel_, ok_;
 
 	std::string curdir_;
+	const std::string illegal_filename_tooltip_;
 };
 
 #endif  // end of include guard: WL_WUI_GAME_MAIN_MENU_SAVE_GAME_H