Merge pull request #2884 from newsboat/feature/2326-remove-implicit-conversions-from-file-dir-browserfeature/2326-filepath

Remove implicit conversions from {File,Dir}BrowserFormAction

8 files changed, 201 insertions, 94 deletions

diff --git a/include/dirbrowserformaction.h b/include/dirbrowserformaction.h
index 0bdaa503..e0b8fdb3 100644
--- a/include/dirbrowserformaction.h
+++ b/include/dirbrowserformaction.h
@@ -43,7 +43,7 @@ private:
 	std::vector<file_system::FileSystemEntry> id_at_position;
 	std::vector<StflRichText> lines;
 
-	Filepath get_formatted_dirname(const Filepath& dirname, mode_t mode);
+	std::string get_formatted_dirname(const Filepath& dirname, mode_t mode);
 
 	LineView file_prompt_line;
 
diff --git a/include/filebrowserformaction.h b/include/filebrowserformaction.h
index 534e7edd..941133d6 100644
--- a/include/filebrowserformaction.h
+++ b/include/filebrowserformaction.h
@@ -48,7 +48,7 @@ private:
 	std::vector<file_system::FileSystemEntry> id_at_position;
 	std::vector<StflRichText> lines;
 
-	Filepath get_formatted_filename(const Filepath& filename, mode_t mode);
+	std::string get_formatted_filename(const Filepath& filename, mode_t mode);
 
 	LineView file_prompt_line;
 	Filepath default_filename;
diff --git a/include/filepath.h b/include/filepath.h
index 6308f2ae..795a6dfb 100644
--- a/include/filepath.h
+++ b/include/filepath.h
@@ -64,6 +64,10 @@ public:
 
 	bool operator==(const Filepath&) const;
 	bool operator!=(const Filepath&) const;
+	bool operator<(const Filepath&) const;
+	bool operator<=(const Filepath&) const;
+	bool operator>(const Filepath&) const;
+	bool operator>=(const Filepath&) const;
 
 	/// Returns the filepath as a string in locale encoding.
 	///
@@ -95,8 +99,9 @@ public:
 
 	// Return `true` if Filepath start with `base`, `false` otherwise.
 	//
-	// \note `ext` is interpreted as bytes in locale encoding.
-	bool starts_with(const std::string& base) const;
+	// Only considers whole path components to match, i.e. "/foo" is **not**
+	// a prefix of "/foobar/baz".
+	bool starts_with(const Filepath& base) const;
 
 	/// Returns the final component of the path, if there is one.
 	nonstd::optional<Filepath> file_name() const;
diff --git a/rust/libnewsboat-ffi/src/filepath.rs b/rust/libnewsboat-ffi/src/filepath.rs
index 778ed0de..602c3fa2 100644
--- a/rust/libnewsboat-ffi/src/filepath.rs
+++ b/rust/libnewsboat-ffi/src/filepath.rs
@@ -19,13 +19,14 @@ mod bridged {
         fn create_empty() -> Box<PathBuf>;
         fn create(filepath: Vec<u8>) -> Box<PathBuf>;
         fn equals(lhs: &PathBuf, rhs: &PathBuf) -> bool;
+        fn less_than(lhs: &PathBuf, rhs: &PathBuf) -> bool;
         fn into_bytes(filepath: &PathBuf) -> Vec<u8>;
         fn display(filepath: &PathBuf) -> String;
         fn push(filepath: &mut PathBuf, component: &PathBuf);
         fn clone(filepath: &PathBuf) -> Box<PathBuf>;
         fn is_absolute(filepath: &PathBuf) -> bool;
         fn set_extension(filepath: &mut PathBuf, extension: Vec<u8>) -> bool;
-        fn starts_with(filepath: &PathBuf, str: Vec<u8>) -> bool;
+        fn starts_with(filepath: &PathBuf, base: &PathBuf) -> bool;
         fn file_name(filepath: &PathBuf) -> Vec<u8>;
     }
 }
@@ -44,6 +45,10 @@ fn equals(lhs: &PathBuf, rhs: &PathBuf) -> bool {
     lhs.0 == rhs.0
 }
 
+fn less_than(lhs: &PathBuf, rhs: &PathBuf) -> bool {
+    lhs.0 < rhs.0
+}
+
 fn into_bytes(filepath: &PathBuf) -> Vec<u8> {
     filepath.0.as_os_str().as_bytes().to_owned()
 }
@@ -68,8 +73,8 @@ fn set_extension(filepath: &mut PathBuf, extension: Vec<u8>) -> bool {
     filepath.0.set_extension(OsStr::from_bytes(&extension))
 }
 
-fn starts_with(filepath: &PathBuf, base: Vec<u8>) -> bool {
-    filepath.0.starts_with(OsStr::from_bytes(&base))
+fn starts_with(filepath: &PathBuf, base: &PathBuf) -> bool {
+    filepath.0.starts_with(&base.0)
 }
 
 fn file_name(filepath: &PathBuf) -> Vec<u8> {
diff --git a/src/dirbrowserformaction.cpp b/src/dirbrowserformaction.cpp
index 62568abe..d2cbaf49 100644
--- a/src/dirbrowserformaction.cpp
+++ b/src/dirbrowserformaction.cpp
@@ -1,5 +1,3 @@
-#define ENABLE_IMPLICIT_FILEPATH_CONVERSIONS
-
 #include "dirbrowserformaction.h"
 
 #include <algorithm>
@@ -62,24 +60,17 @@ bool DirBrowserFormAction::process_operation(Operation op,
 						selection.name,
 						status);
 					files_list.set_position(0);
-					std::string fn = utils::getcwd();
+					const auto fn = utils::getcwd();
 					update_title(fn);
 
-					if (fn.back() != NEWSBEUTER_PATH_SEP) {
-						fn.push_back(NEWSBEUTER_PATH_SEP);
-					}
-
-					set_value("filenametext", fn);
+					const auto fn_with_trailing_slash = fn.join(Filepath{});
+					set_value("filenametext", fn_with_trailing_slash.to_locale_string());
 					do_redraw = true;
 				}
 				break;
 				case file_system::FileType::RegularFile: {
-					std::string fn = utils::getcwd();
-					if (fn.back() != NEWSBEUTER_PATH_SEP) {
-						fn.push_back(NEWSBEUTER_PATH_SEP);
-					}
-					fn.append(selection.name);
-					set_value("filenametext", fn);
+					const auto filename = utils::getcwd().join(selection.name);
+					set_value("filenametext", filename.to_locale_string());
 					f.set_focus("filename");
 				}
 				break;
@@ -184,31 +175,31 @@ void DirBrowserFormAction::update_title(const Filepath& working_directory)
 	FmtStrFormatter fmt;
 	fmt.register_fmt('N', PROGRAM_NAME);
 	fmt.register_fmt('V', utils::program_version());
-	fmt.register_fmt('f', working_directory);
+	fmt.register_fmt('f', working_directory.display());
 
 	set_title(fmt.do_format(
 			cfg->get_configvalue("dirbrowser-title-format"), width));
 }
 
-std::vector<std::string> get_sorted_dirlist()
+std::vector<Filepath> get_sorted_dirlist()
 {
-	std::vector<std::string> ret;
+	std::vector<Filepath> ret;
 
-	const std::string cwdtmp = utils::getcwd();
+	const auto cwdtmp = utils::getcwd();
 
-	DIR* dirp = ::opendir(cwdtmp.c_str());
+	DIR* dirp = ::opendir(cwdtmp.to_locale_string().c_str());
 	if (dirp) {
 		struct dirent* de = ::readdir(dirp);
 		while (de) {
 			if (strcmp(de->d_name, ".") != 0 &&
 				strcmp(de->d_name, "..") != 0) {
 				struct stat sb;
-				auto dpath = strprintf::fmt(
-						"%s/%s", cwdtmp, de->d_name);
-				if (::lstat(dpath.c_str(), &sb) == 0) {
+				auto entry = Filepath::from_locale_string(de->d_name);
+				const auto dpath = cwdtmp.join(entry);
+				if (::lstat(dpath.to_locale_string().c_str(), &sb) == 0) {
 					const auto ftype = file_system::mode_to_filetype(sb.st_mode);
 					if (ftype == file_system::FileType::Directory) {
-						ret.push_back(de->d_name);
+						ret.emplace_back(std::move(entry));
 					}
 				}
 			}
@@ -220,8 +211,8 @@ std::vector<std::string> get_sorted_dirlist()
 
 	std::sort(ret.begin(), ret.end());
 
-	if (cwdtmp != "/") {
-		ret.insert(ret.begin(), "..");
+	if (cwdtmp != Filepath::from_locale_string("/")) {
+		ret.emplace(ret.begin(), Filepath::from_locale_string(".."));
 	}
 
 	return ret;
@@ -235,14 +226,11 @@ void DirBrowserFormAction::prepare()
 	 * in the current directory.
 	 */
 	if (do_redraw) {
-		const std::string cwdtmp = utils::getcwd();
-		update_title(cwdtmp);
-
-		std::vector<std::string> directories = get_sorted_dirlist();
+		update_title(utils::getcwd());
 
 		id_at_position.clear();
 		lines.clear();
-		for (std::string directory : directories) {
+		for (auto directory : get_sorted_dirlist()) {
 			add_directory(id_at_position, directory);
 		}
 
@@ -334,15 +322,16 @@ void DirBrowserFormAction::add_directory(
 	}
 }
 
-Filepath DirBrowserFormAction::get_formatted_dirname(const Filepath& dirname,
+std::string DirBrowserFormAction::get_formatted_dirname(const Filepath& dirname,
 	mode_t mode)
 {
-	Filepath tmp = dirname;
+	const auto dirname_str = dirname.display();
 	const auto suffix = file_system::mode_suffix(mode);
 	if (suffix.has_value()) {
-		tmp.set_extension(std::to_string(suffix.value()));
+		return strprintf::fmt("%s%c", dirname_str, suffix.value());
+	} else {
+		return dirname_str;
 	}
-	return tmp;
 }
 
 std::string DirBrowserFormAction::title()
diff --git a/src/filebrowserformaction.cpp b/src/filebrowserformaction.cpp
index 464ac9d5..d489c647 100644
--- a/src/filebrowserformaction.cpp
+++ b/src/filebrowserformaction.cpp
@@ -1,5 +1,3 @@
-#define ENABLE_IMPLICIT_FILEPATH_CONVERSIONS
-
 #include "filebrowserformaction.h"
 
 #include <algorithm>
@@ -61,35 +59,20 @@ bool FileBrowserFormAction::process_operation(Operation op,
 						selection.name,
 						status);
 					files_list.set_position(0);
-					std::string fn = utils::getcwd();
+					auto fn = utils::getcwd();
 					update_title(fn);
 
-					if (fn.back() != NEWSBEUTER_PATH_SEP) {
-						fn.push_back(NEWSBEUTER_PATH_SEP);
-					}
-
-					const std::string fnstr =
-						f.get("filenametext");
-					const std::string::size_type base =
-						fnstr.find_last_of(NEWSBEUTER_PATH_SEP);
-					if (base == std::string::npos) {
-						fn.append(fnstr);
-					} else {
-						fn.append(fnstr,
-							base + 1,
-							std::string::npos);
+					const auto fnstr = Filepath::from_locale_string(f.get("filenametext")).file_name();
+					if (fnstr) {
+						fn.push(*fnstr);
 					}
-					set_value("filenametext", fn);
+					set_value("filenametext", fn.to_locale_string());
 					do_redraw = true;
 				}
 				break;
 				case file_system::FileType::RegularFile: {
-					std::string fn = utils::getcwd();
-					if (fn.back() != NEWSBEUTER_PATH_SEP) {
-						fn.push_back(NEWSBEUTER_PATH_SEP);
-					}
-					fn.append(selection.name);
-					set_value("filenametext", fn);
+					const auto filename = utils::getcwd().join(selection.name);
+					set_value("filenametext", filename.to_locale_string());
 					f.set_focus("filename");
 				}
 				break;
@@ -216,25 +199,25 @@ void FileBrowserFormAction::update_title(const Filepath& working_directory)
 	FmtStrFormatter fmt;
 	fmt.register_fmt('N', PROGRAM_NAME);
 	fmt.register_fmt('V', utils::program_version());
-	fmt.register_fmt('f', working_directory);
+	fmt.register_fmt('f', working_directory.display());
 
 	set_title(fmt.do_format(
 			cfg->get_configvalue("filebrowser-title-format"), width));
 }
 
-std::vector<std::string> get_sorted_filelist()
+std::vector<Filepath> get_sorted_filelist()
 {
-	std::vector<std::string> ret;
+	std::vector<Filepath> ret;
 
-	const std::string cwdtmp = utils::getcwd();
+	const auto cwdtmp = utils::getcwd();
 
-	DIR* dirp = ::opendir(cwdtmp.c_str());
+	DIR* dirp = ::opendir(cwdtmp.to_locale_string().c_str());
 	if (dirp) {
 		struct dirent* de = ::readdir(dirp);
 		while (de) {
 			if (strcmp(de->d_name, ".") != 0 &&
 				strcmp(de->d_name, "..") != 0) {
-				ret.push_back(de->d_name);
+				ret.push_back(Filepath::from_locale_string(de->d_name));
 			}
 			de = ::readdir(dirp);
 		}
@@ -243,8 +226,8 @@ std::vector<std::string> get_sorted_filelist()
 
 	std::sort(ret.begin(), ret.end());
 
-	if (cwdtmp != "/") {
-		ret.insert(ret.begin(), "..");
+	if (cwdtmp != Filepath::from_locale_string("/")) {
+		ret.emplace(ret.begin(), Filepath::from_locale_string(".."));
 	}
 
 	return ret;
@@ -258,18 +241,14 @@ void FileBrowserFormAction::prepare()
 	 * in the current directory.
 	 */
 	if (do_redraw) {
-		const std::string cwdtmp = utils::getcwd();
-		update_title(cwdtmp);
-
-		std::vector<std::string> files = get_sorted_filelist();
+		update_title(utils::getcwd());
 
 		id_at_position.clear();
 		lines.clear();
-		for (std::string filename : files) {
+		for (auto filename : get_sorted_filelist()) {
 			add_file(id_at_position, filename);
 		}
 
-
 		auto render_line = [this](std::uint32_t line, std::uint32_t width) -> StflRichText {
 			(void)width;
 			return lines[line];
@@ -310,7 +289,7 @@ void FileBrowserFormAction::init()
 	const int status = ::chdir(save_path.c_str());
 	LOG(Level::DEBUG, "view::filebrowser: chdir(%s) = %i", save_path, status);
 
-	set_value("filenametext", default_filename);
+	set_value("filenametext", default_filename.to_locale_string());
 
 	// Set position to 0 and back to ensure that the text is visible
 	draw_form();
@@ -359,14 +338,15 @@ void FileBrowserFormAction::add_file(
 	}
 }
 
-Filepath FileBrowserFormAction::get_formatted_filename(const Filepath& filename,
+std::string FileBrowserFormAction::get_formatted_filename(const Filepath& filename,
 	mode_t mode)
 {
+	const auto filename_str = filename.display();
 	const auto suffix = file_system::mode_suffix(mode);
 	if (suffix.has_value()) {
-		return strprintf::fmt("%s%c", filename, suffix.value());
+		return strprintf::fmt("%s%c", filename_str, suffix.value());
 	} else {
-		return filename;
+		return filename_str;
 	}
 }
 
diff --git a/src/filepath.cpp b/src/filepath.cpp
index b5ff097e..caaf7da5 100644
--- a/src/filepath.cpp
+++ b/src/filepath.cpp
@@ -63,6 +63,26 @@ bool Filepath::operator!=(const Filepath& other) const
 	return !(*this == other);
 }
 
+bool Filepath::operator<(const Filepath& other) const
+{
+	return filepath::bridged::less_than(*rs_object, *other.rs_object);
+}
+
+bool Filepath::operator<=(const Filepath& other) const
+{
+	return !(*this > other);
+}
+
+bool Filepath::operator>(const Filepath& other) const
+{
+	return !(*this < other) && (*this != other);
+}
+
+bool Filepath::operator>=(const Filepath& other) const
+{
+	return !(*this < other);
+}
+
 void Filepath::push(const Filepath& component)
 {
 	filepath::bridged::push(*rs_object, *component.rs_object);
@@ -85,9 +105,9 @@ bool Filepath::set_extension(const std::string& ext)
 	return filepath::bridged::set_extension(*rs_object, string_to_vec(ext));
 }
 
-bool Filepath::starts_with(const std::string& base) const
+bool Filepath::starts_with(const Filepath& base) const
 {
-	return filepath::bridged::starts_with(*rs_object, string_to_vec(base));
+	return filepath::bridged::starts_with(*rs_object, *base.rs_object);
 }
 
 nonstd::optional<Filepath> Filepath::file_name() const
diff --git a/test/filepath.cpp b/test/filepath.cpp
index 16ebb75f..5d500489 100644
--- a/test/filepath.cpp
+++ b/test/filepath.cpp
@@ -1,5 +1,3 @@
-#define ENABLE_IMPLICIT_FILEPATH_CONVERSIONS
-
 #include "3rd-party/catch.hpp"
 
 #include "filepath.h"
@@ -43,14 +41,33 @@ TEST_CASE("push() adds a new component to the path", "[Filepath]")
 	REQUIRE(dir == Filepath::from_locale_string("/tmp/newsboat/.local/share/cache/cache.db"));
 }
 
+TEST_CASE("push() still adds a separator to non-empty path if new component is empty",
+	"[Filepath]")
+{
+	auto dir = Filepath::from_locale_string("/root");
+	dir.push(Filepath::from_locale_string(""));
+	REQUIRE(dir.display() == "/root/");
+}
+
 TEST_CASE("Can be extended with join()", "[Filepath]")
 {
 	const auto tmp = Filepath::from_locale_string("/tmp");
 
-	const auto subdir = tmp.join("newsboat").join("tests");
+	const auto subdir =
+		tmp
+		.join(Filepath::from_locale_string("newsboat"))
+		.join(Filepath::from_locale_string("tests"));
 	REQUIRE(subdir == Filepath::from_locale_string("/tmp/newsboat/tests"));
 }
 
+TEST_CASE("join() still adds a separator to non-empty path if new component is empty",
+	"[Filepath]")
+{
+	const auto path = Filepath::from_locale_string("relative path");
+	const auto path_with_trailing_slash = path.join(Filepath{});
+	REQUIRE(path_with_trailing_slash.display() == "relative path/");
+}
+
 TEST_CASE("Can be copied", "[Filepath]")
 {
 	auto original = Filepath::from_locale_string("/etc/hosts");
@@ -84,11 +101,11 @@ TEST_CASE("Can't set extension for an empty path", "[Filepath]")
 
 TEST_CASE("Can set extension for non-empty path", "[Filepath]")
 {
-	Filepath path("file");
+	auto path = Filepath::from_locale_string("file");
 
 	SECTION("extension is UTF-8") {
 		REQUIRE(path.set_extension("exe"));
-		REQUIRE(path == "file.exe");
+		REQUIRE(path == Filepath::from_locale_string("file.exe"));
 	}
 
 	SECTION("extension is not a valid UTF-8 string") {
@@ -105,21 +122,21 @@ TEST_CASE("Can check if path is absolute", "[Filepath]")
 	}
 
 	SECTION("path that starts with a slash is absolute") {
-		path.push("/etc");
+		path.push(Filepath::from_locale_string("/etc"));
 		REQUIRE(path.display() == "/etc");
 		REQUIRE(path.is_absolute());
 
-		path.push("ca-certificates");
+		path.push(Filepath::from_locale_string("ca-certificates"));
 		REQUIRE(path.display() == "/etc/ca-certificates");
 		REQUIRE(path.is_absolute());
 	}
 
 	SECTION("path that doesn't start with a slash is not absolute") {
-		path.push("vmlinuz");
+		path.push(Filepath::from_locale_string("vmlinuz"));
 		REQUIRE(path.display() == "vmlinuz");
 		REQUIRE_FALSE(path.is_absolute());
 
-		path.push("undefined");
+		path.push(Filepath::from_locale_string("undefined"));
 		REQUIRE(path.display() == "vmlinuz/undefined");
 		REQUIRE_FALSE(path.is_absolute());
 	}
@@ -189,3 +206,94 @@ TEST_CASE("Can extract the final component of the path (file or directory name)"
 		REQUIRE(path.file_name().value() == Filepath::from_locale_string("one\x80two"));
 	}
 }
+
+TEST_CASE("Can be ordered lexicographically", "[Filepath]")
+{
+	const auto root = Filepath::from_locale_string("/");
+	const auto var_log = Filepath::from_locale_string("/var/log");
+	const auto home_minoru = Filepath::from_locale_string("/home/minoru");
+	const auto home_minoru_src_newsboat =
+		Filepath::from_locale_string("/home/minoru/src/newsboat");
+
+	SECTION("operator<") {
+		SECTION("Path to directory is less than the path to its subdirectory") {
+			REQUIRE(root < var_log);
+			REQUIRE(root < home_minoru);
+			REQUIRE(home_minoru < home_minoru_src_newsboat);
+
+			REQUIRE_FALSE(home_minoru_src_newsboat < root);
+		}
+
+		SECTION("Disparate paths are ordered lexicographically") {
+			REQUIRE(home_minoru < var_log);
+			REQUIRE(home_minoru_src_newsboat < var_log);
+
+			REQUIRE_FALSE(home_minoru_src_newsboat < home_minoru);
+		}
+	}
+
+	SECTION("operator>") {
+		SECTION("Path to subdirectory is greater than the path to its parent directory") {
+			REQUIRE(var_log > root);
+			REQUIRE(home_minoru > root);
+			REQUIRE(home_minoru_src_newsboat > home_minoru);
+
+			REQUIRE_FALSE(root > home_minoru_src_newsboat);
+		}
+
+		SECTION("Disparate paths are ordered lexicographically") {
+			REQUIRE(var_log > home_minoru);
+			REQUIRE(var_log > home_minoru_src_newsboat);
+
+			REQUIRE_FALSE(home_minoru > home_minoru_src_newsboat);
+		}
+	}
+
+	SECTION("operator<=") {
+		SECTION("Any path is less than or equal to itself") {
+			REQUIRE(root <= root);
+			REQUIRE(var_log <= var_log);
+			REQUIRE(home_minoru <= home_minoru);
+			REQUIRE(home_minoru_src_newsboat <= home_minoru_src_newsboat);
+		}
+
+		SECTION("Path to directory is less than or equal to the path to its subdirectory") {
+			REQUIRE(root <= var_log);
+			REQUIRE(root <= home_minoru);
+			REQUIRE(home_minoru <= home_minoru_src_newsboat);
+
+			REQUIRE_FALSE(home_minoru_src_newsboat <= root);
+		}
+
+		SECTION("Disparate paths are ordered lexicographically") {
+			REQUIRE(home_minoru <= var_log);
+			REQUIRE(home_minoru_src_newsboat <= var_log);
+
+			REQUIRE_FALSE(home_minoru_src_newsboat <= home_minoru);
+		}
+	}
+
+	SECTION("operator>=") {
+		SECTION("Any path is greater than or equal to itself") {
+			REQUIRE(root >= root);
+			REQUIRE(var_log >= var_log);
+			REQUIRE(home_minoru >= home_minoru);
+			REQUIRE(home_minoru_src_newsboat >= home_minoru_src_newsboat);
+		}
+
+		SECTION("Path to subdirectory is greater than or equal to the path to its parent directory") {
+			REQUIRE(var_log >= root);
+			REQUIRE(home_minoru >= root);
+			REQUIRE(home_minoru_src_newsboat >= home_minoru);
+
+			REQUIRE_FALSE(root >= home_minoru_src_newsboat);
+		}
+
+		SECTION("Disparate paths are ordered lexicographically") {
+			REQUIRE(var_log >= home_minoru);
+			REQUIRE(var_log >= home_minoru_src_newsboat);
+
+			REQUIRE_FALSE(home_minoru >= home_minoru_src_newsboat);
+		}
+	}
+}