Merge lp:~widelands-dev/widelands/bug-1560454-mapdir into lp:widelands

[GunChleoc]

UI improvements for saving maps:
- Maps are now saved in a "My Maps" subdirectory to prevent overwriting of official maps
- Show full save path to user
- After creating a new directory, enter it
- Automatically focus directory name editbox when creating new directory
- Got rid of blank space in load map screen
- Made show map names/filenames button wide

[Miroslav Remák (miroslavr256)]

For me it shows a different save path than where the map is actually saved: /widelands_working_dir/maps/My_Maps/map.wmf instead of /home/user/.widelands/maps/My_Maps/map.wmf. I guess that's because canonicalize_name does not take file system layers into account.

[kaputtnik (franku)]

> - Show full save path to user
This shows the wrong path. Here it is shown: /home/kaputtnik/widelands-repo/bug-1560454-mapdir/maps/My_Maps
Instead the map is saved in: /home/kaputtnik/.widelands/maps/My_Maps

> - After creating a new directory, enter it
Good idea :-) But it confuses a bit, because one couldn't see which folder is currently shown.

> - Automatically focus map name editbox when creating new map
I couldn't find this feature :-S Either it is not working or i do not understand it. If i create a new map nothing is shown to edit the name of the map. And if i want to save the map, the focus is always on the map, not in the menu (f.e. hitting "f" toggles Fullscreen). If i first choose the map options menu, there is also no focus on the map name edit box.

It is not possible anymore to store a map in a folder which was created prior this changes, because i couldn't go to the "previous folder". So my maps stored in folder ~/.widelands/maps/test/ could be loaded, but not saved in this folder anymore. One has to move the previous folders he created in ~/.widelands/maps into ~/.widelands/maps/My_Maps/ to save his own maps in his own folders again. Same goes for maps which are stored in ~/.widelands/maps/

As mentioned in the bug report i would like to see the "Shipped maps" in a sub folder in menu "Load map" where the shipped maps could be found. In menu "Save map" this folder shouldn't be shown then. I believe this would be the better approach. Isn't that possible?

Needs fixing because of "Show full path to the user".

review: Needs Fixing

[GunChleoc (gunchleoc)]

> > - Show full save path to user
> This shows the wrong path. Here it is shown: /home/kaputtnik/widelands-
> repo/bug-1560454-mapdir/maps/My_Maps
> Instead the map is saved in: /home/kaputtnik/.widelands/maps/My_Maps

I'll look into this.

> > - After creating a new directory, enter it
> Good idea :-) But it confuses a bit, because one couldn't see which folder is
> currently shown.

I'll remove this feature then

> > - Automatically focus map name editbox when creating new map
> I couldn't find this feature :-S Either it is not working or i do not
> understand it.

Sorry, I meant create directory, not create map.

> It is not possible anymore to store a map in a folder which was created prior
> this changes, because i couldn't go to the "previous folder". So my maps
> stored in folder ~/.widelands/maps/test/ could be loaded, but not saved in
> this folder anymore. One has to move the previous folders he created in
> ~/.widelands/maps into ~/.widelands/maps/My_Maps/ to save his own maps in his
> own folders again. Same goes for maps which are stored in ~/.widelands/maps/

Yes, they will have to be moved. Which is why I came up with the idea of displaying the full directory, so the user can go find the maps on their computer and fix.

> As mentioned in the bug report i would like to see the "Shipped maps" in a sub
> folder in menu "Load map" where the shipped maps could be found. In menu "Save
> map" this folder shouldn't be shown then. I believe this would be the better
> approach. Isn't that possible?

And how would this look when creating a new game then? All users, even those who don't have any own maps, will have to enter a "Shipped Maps" subfolder every time in order to start a new game?

[kaputtnik (franku)]

> > As mentioned in the bug report i would like to see the "Shipped maps" in a
> sub
> > folder in menu "Load map" where the shipped maps could be found. In menu
> "Save
> > map" this folder shouldn't be shown then. I believe this would be the better
> > approach. Isn't that possible?
>
> And how would this look when creating a new game then? All users, even those
> who don't have any own maps, will have to enter a "Shipped Maps" subfolder
> every time in order to start a new game?

Sorry, i forget always that "New game" and "Load map" is the same menu :-S

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1127. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/130361026.
Appveyor build 964. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1560454_mapdir-964.

[kaputtnik (franku)]

> > > - Automatically focus map name editbox when creating new map
> > I couldn't find this feature :-S Either it is not working or i do not
> > understand it.
>
> Sorry, I meant create directory, not create map.

If i create a new directory the map name editbox isn't focused here ("f" button toggles fullscreen).

[GunChleoc (gunchleoc)]

Displaying the absolute path is less than trivial, because the LayeredFilesystem will iterate internally over all file systems until it finds a match. So, I decided to display the relative directory instead, both in the load and save screens.

> If i create a new directory the map name editbox isn't focused here ("f"
> button toggles fullscreen).

The shortcut for toggle fullscreen is CTRL+f, so I have no idea how you managed this.

What I am talking about is:

Old behaviour: Save Map -> Make Directory, click the edit box, start typing
New behaviour: Save Map -> Make Directory, start typing right away

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1132. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/130773045.
Appveyor build 969. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1560454_mapdir-969.

[kaputtnik (franku)]

Ahhh... now i got it. I've always tested the map name edit box (as the commit message says, even after you've changed it) but you meant the directory name edit box... This is a really good enhancement :-)

Showing the relative directory is IMHO ok. The position of this could be discussed... but i think we should discuss this if we have time to make a menu overhaul. I want to make some mockups with my suggestions if i am ready with the website code.

The "f" button toggles fullscreen without "CTRL" here. Just like it is explained in the help ;)

review: Approve (testing)

[GunChleoc (gunchleoc)]

Oops, I fixed the commit message. I don't get the problem with the fullscreen toggling while the text input is focused. You're right, it's without the Ctrl key.

[kaputtnik (franku)]

If the focus is in the directory name edit box, all works correct, no keyboard shortcuts works then.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

The read operation timed out

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1132. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/130773045.
Appveyor build 969. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1560454_mapdir-969.

[SirVer (sirver)]

look at me! I did a codereview!!!

code lgtm. only one nit.

review: Approve

[GunChleoc (gunchleoc)]

LOL thanks!

[GunChleoc (gunchleoc)]

@bunnybot merge

=== modified file 'src/editor/ui_menus/main_menu_load_map.cc'
--- src/editor/ui_menus/main_menu_load_map.cc	2016-04-06 09:23:04 +0000
+++ src/editor/ui_menus/main_menu_load_map.cc	2016-06-07 09:07:42 +0000
@@ -19,6 +19,9 @@
 
 #include "editor/ui_menus/main_menu_load_map.h"
 
+#include <boost/algorithm/string.hpp>
+#include <boost/format.hpp>
+
 #include "base/i18n.h"
 #include "editor/editorinteractive.h"
 #include "io/filesystem/layered_filesystem.h"
@@ -30,7 +33,8 @@
  * Create all the buttons etc...
 */
 MainMenuLoadMap::MainMenuLoadMap(EditorInteractive& parent)
-   : MainMenuLoadOrSaveMap(parent, "load_map_menu", _("Load Map")) {
+   : MainMenuLoadOrSaveMap(parent, 2, "load_map_menu", _("Load Map")) {
+	set_current_directory(curdir_);
 
 	table_.selected.connect(boost::bind(&MainMenuLoadMap::entry_selected, this));
 	table_.double_clicked.connect(boost::bind(&MainMenuLoadMap::clicked_ok, boost::ref(*this)));
@@ -45,7 +49,7 @@
 	const MapData& mapdata = maps_data_[table_.get_selected()];
 	if (g_fs->is_directory(mapdata.filename) &&
 	    !Widelands::WidelandsMapLoader::is_widelands_map(mapdata.filename)) {
-		curdir_ = mapdata.filename;
+		set_current_directory(mapdata.filename);
 		fill_table();
 	} else {
 		EditorInteractive& eia = dynamic_cast<EditorInteractive&>(*get_parent());
@@ -54,6 +58,22 @@
 	}
 }
 
+void MainMenuLoadMap::set_current_directory(const std::string& filename) {
+	curdir_ = filename;
+
+	std::string display_dir = curdir_.substr(basedir_.size());
+	if (boost::starts_with(display_dir, "/")) {
+		display_dir = display_dir.substr(1);
+	}
+	if (boost::starts_with(display_dir, "My_Maps")) {
+		boost::replace_first(display_dir, "My_Maps", _("My Maps"));
+	} else if (boost::starts_with(display_dir, "MP_Scenarios")) {
+		boost::replace_first(display_dir, "MP_Scenarios", _("Multiplayer Scenarios"));
+	}
+	/** TRANSLATORS: The folder that a file will be saved to. */
+	directory_info_.set_text((boost::format(_("Current Directory: %s")) % display_dir).str());
+}
+
 /**
  * Called when a entry is selected
  */

=== modified file 'src/editor/ui_menus/main_menu_load_map.h'
--- src/editor/ui_menus/main_menu_load_map.h	2016-04-06 09:23:04 +0000
+++ src/editor/ui_menus/main_menu_load_map.h	2016-06-07 09:07:42 +0000
@@ -33,6 +33,8 @@
 
 protected:
 	void clicked_ok() override;
+	// Sets the current dir and updates labels.
+	void set_current_directory(const std::string& filename) override;
 
 private:
 	void entry_selected();

=== modified file 'src/editor/ui_menus/main_menu_load_or_save_map.cc'
--- src/editor/ui_menus/main_menu_load_or_save_map.cc	2016-04-20 07:58:14 +0000
+++ src/editor/ui_menus/main_menu_load_or_save_map.cc	2016-06-07 09:07:42 +0000
@@ -32,8 +32,10 @@
 #include "map_io/widelands_map_loader.h"
 
 MainMenuLoadOrSaveMap::MainMenuLoadOrSaveMap(EditorInteractive& parent,
+                                             int no_of_bottom_rows,
                                              const std::string& name,
-                                             const std::string& title)
+                                             const std::string& title,
+                                             const std::string& basedir)
    : UI::Window(&parent, name, 0, 0, parent.get_inner_w() - 40, parent.get_inner_h() - 40, title),
 
      // Values for alignment and size
@@ -42,7 +44,7 @@
      tablex_(padding_),
      tabley_(buth_ + 2 * padding_),
      tablew_(get_inner_w() * 7 / 12),
-     tableh_(get_inner_h() - tabley_ - 3 * buth_ - 2 * padding_),
+     tableh_(get_inner_h() - tabley_ - (no_of_bottom_rows + 1) * buth_ - no_of_bottom_rows * padding_),
      right_column_x_(tablew_ + 2 * padding_),
      butw_((get_inner_w() - right_column_x_ - 2 * padding_) / 2),
 
@@ -50,6 +52,11 @@
 	  map_details_(
 		  this, right_column_x_, tabley_, get_inner_w() - right_column_x_ - padding_, tableh_,
 		  MapDetails::Style::kWui),
+     directory_info_(this,
+                     padding_,
+                     get_inner_h() - 2 * buth_ - 4 * padding_,
+                     "",
+                     UI::Align::kLeft),
      ok_(this,
          "ok",
 			UI::g_fh1->fontset()->is_rtl() ? get_inner_w() / 2 - butw_ - padding_ : get_inner_w() / 2 + padding_,
@@ -68,9 +75,10 @@
              buth_,
 				 g_gr->images().get("images/ui_basic/but1.png"),
              _("Cancel")),
-     basedir_("maps"),
+     basedir_(basedir),
      has_translated_mapname_(false),
      showing_mapames_(false) {
+	g_fs->ensure_directory_exists(basedir_);
 	curdir_ = basedir_;
 
 	UI::Box* vbox = new UI::Box(this, tablex_, padding_, UI::Box::Horizontal, padding_, get_w());
@@ -78,7 +86,7 @@
 	                                "show_mapnames",
 	                                0,
 	                                0,
-	                                butw_,
+	                                2 * butw_,
 	                                buth_,
 											  g_gr->images().get("images/ui_basic/but1.png"),
 	                                _("Show Map Names"));
@@ -155,6 +163,8 @@
 	// about the absolute filesystem top!) we manually add ".."
 	if (curdir_ != basedir_) {
 		maps_data_.push_back(MapData::create_parent_dir(curdir_));
+	} else if (files.empty()) {
+		maps_data_.push_back(MapData::create_empty_dir(curdir_));
 	}
 
 	MapData::DisplayType display_type;

=== modified file 'src/editor/ui_menus/main_menu_load_or_save_map.h'
--- src/editor/ui_menus/main_menu_load_or_save_map.h	2016-04-06 09:23:04 +0000
+++ src/editor/ui_menus/main_menu_load_or_save_map.h	2016-06-07 09:07:42 +0000
@@ -25,6 +25,7 @@
 #include "editor/editorinteractive.h"
 #include "ui_basic/button.h"
 #include "ui_basic/checkbox.h"
+#include "ui_basic/textarea.h"
 #include "ui_basic/window.h"
 #include "wui/mapdetails.h"
 #include "wui/maptable.h"
@@ -34,12 +35,16 @@
 */
 struct MainMenuLoadOrSaveMap : public UI::Window {
 	MainMenuLoadOrSaveMap(EditorInteractive& parent,
+	                      int no_of_bottom_rows,
 	                      const std::string& name,
-	                      const std::string& title);
+	                      const std::string& title,
+	                      const std::string& basedir = "maps");
 
 protected:
 	virtual void clicked_ok() = 0;
 	void toggle_mapnames();
+	// Sets the current dir and updates labels.
+	virtual void set_current_directory(const std::string& filename) = 0;
 	void fill_table();
 
 	bool compare_players(uint32_t, uint32_t);
@@ -57,6 +62,7 @@
 	std::vector<MapData> maps_data_;
 	MapDetails map_details_;
 
+	UI::Textarea directory_info_;
 	UI::Button ok_, cancel_;
 
 	const std::string basedir_;

=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc	2016-05-07 18:54:04 +0000
+++ src/editor/ui_menus/main_menu_save_map.cc	2016-06-07 09:07:42 +0000
@@ -49,7 +49,7 @@
 
 // TODO(GunChleoc): Arabic: Make directory dialog: buttons need more height for Arabic.
 MainMenuSaveMap::MainMenuSaveMap(EditorInteractive& parent)
-   : MainMenuLoadOrSaveMap(parent, "save_map_menu", _("Save Map")),
+   : MainMenuLoadOrSaveMap(parent, 3, "save_map_menu", _("Save Map"), "maps/My_Maps"),
 
      make_directory_(this,
                      "make_directory",
@@ -74,6 +74,7 @@
                     buth_,
                     _("Filename:"),
                     UI::Align::kLeft) {
+	set_current_directory(curdir_);
 
 	// Make room for edit_options_ button
 	map_details_.set_max_height(map_details_.get_h() - buth_ - padding_);
@@ -131,7 +132,7 @@
 
 	if (g_fs->is_directory(complete_filename.c_str()) &&
 		 !Widelands::WidelandsMapLoader::is_widelands_map(complete_filename)) {
-		curdir_ = complete_filename;
+		set_current_directory(complete_filename);
 		fill_table();
 	} else {  //  Ok, save this map
 		Widelands::Map& map = eia().egbase().map();
@@ -151,6 +152,7 @@
  * Called, when the make directory button was clicked.
  */
 void MainMenuSaveMap::clicked_make_directory() {
+	/** TRANSLATORS: A folder that hasn't been given a name yet */
 	MainMenuSaveMapMakeDirectory md(this, _("unnamed"));
 	if (md.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) {
 		g_fs->ensure_directory_exists(curdir_);
@@ -201,7 +203,7 @@
 	assert(table_.has_selection());
 	const MapData& mapdata = maps_data_[table_.get_selected()];
 	if (mapdata.maptype == MapData::MapType::kDirectory) {
-		curdir_ = mapdata.filename;
+		set_current_directory(mapdata.filename);
 		fill_table();
 	} else {
 		clicked_ok();
@@ -216,6 +218,13 @@
 	ok_.set_enabled(!editbox_->text().empty());
 }
 
+void MainMenuSaveMap::set_current_directory(const std::string& filename) {
+	curdir_ = filename;
+	/** TRANSLATORS: The folder that a file will be saved to. */
+	directory_info_.set_text((boost::format(_("Current Directory: %s"))
+	                          % (_("My Maps") + curdir_.substr(basedir_.size()))).str());
+}
+
 /**
  * Save the map in the current directory with
  * the current filename

=== modified file 'src/editor/ui_menus/main_menu_save_map.h'
--- src/editor/ui_menus/main_menu_save_map.h	2016-04-06 09:23:04 +0000
+++ src/editor/ui_menus/main_menu_save_map.h	2016-06-07 09:07:42 +0000
@@ -34,6 +34,10 @@
 struct MainMenuSaveMap : public MainMenuLoadOrSaveMap {
 	MainMenuSaveMap(EditorInteractive& parent);
 
+protected:
+	// Sets the current dir and updates labels.
+	void set_current_directory(const std::string& filename) override;
+
 private:
 	EditorInteractive & eia();
 	void clicked_ok();

=== modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.cc'
--- src/editor/ui_menus/main_menu_save_map_make_directory.cc	2016-04-23 08:05:31 +0000
+++ src/editor/ui_menus/main_menu_save_map_make_directory.cc	2016-06-07 09:07:42 +0000
@@ -66,6 +66,7 @@
 						 boost::ref(*this),
 						 UI::Panel::Returncodes::kBack));
 	center_to_parent();
+	edit_.focus();
 }
 
 

=== modified file 'src/wui/mapdata.cc'
--- src/wui/mapdata.cc	2016-04-16 07:10:03 +0000
+++ src/wui/mapdata.cc	2016-06-07 09:07:42 +0000
@@ -131,11 +131,20 @@
 }
 
 // static
+MapData MapData::create_empty_dir(const std::string& current_dir) {
+	/** TRANSLATORS: This label is shown when a folder is empty */
+	return MapData(current_dir, (boost::format("<%s>") % _("empty")).str());
+}
+
+// static
 MapData MapData::create_directory(const std::string& directory) {
 	std::string localized_name;
 	if (boost::equals(directory, "maps/MP_Scenarios")) {
 		/** TRANSLATORS: Directory name for MP Scenarios in map selection */
 		localized_name = _("Multiplayer Scenarios");
+	} else if (boost::equals(directory, "maps/My_Maps")) {
+		/** TRANSLATORS: Directory name for user maps in map selection */
+		localized_name = _("My Maps");
 	} else {
 		localized_name = FileSystem::fs_filename(directory.c_str());
 	}

=== modified file 'src/wui/mapdata.h'
--- src/wui/mapdata.h	2016-02-06 18:58:57 +0000
+++ src/wui/mapdata.h	2016-06-07 09:07:42 +0000
@@ -89,6 +89,9 @@
 	/// Get the ".." directory
 	static MapData create_parent_dir(const std::string& current_dir);
 
+	/// To display if the directory is empty and has no parent
+	static MapData create_empty_dir(const std::string& current_dir);
+
 	/// Create a subdirectory
 	static MapData create_directory(const std::string& directory);
 

=== modified file 'src/wui/maptable.cc'
--- src/wui/maptable.cc	2016-01-29 08:37:22 +0000
+++ src/wui/maptable.cc	2016-06-07 09:07:42 +0000
@@ -34,7 +34,7 @@
 
 	/** TRANSLATORS: Column title for number of players in map list */
 	add_column(35, _("Pl."), _("Number of players"), UI::Align::kHCenter);
-	add_column(get_w() - 35 - 115, "", _("The name of the map or scenario"), UI::Align::kLeft);
+	add_column(get_w() - 35 - 115, _("Filename"), _("The name of the map or scenario"), UI::Align::kLeft);
 	add_column(115, _("Size"), _("The size of the map (Width x Height)"), UI::Align::kLeft);
 	set_sort_column(0);
 }