diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index b1dcc81296..ff14e563fb 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -293,6 +293,13 @@ inline std::string sanitize_file_basename(const std::string &name){ // Names made only of dots and spaces refer to the folder or its parent, or are stripped to nothing on Windows. return base.find_first_not_of(". ") == std::string::npos ? std::string() : base; } +// Marker file a download of this process writes to before it is renamed to filename. +boost::filesystem::path download_marker_path(const boost::filesystem::path &dest_folder, const std::string &filename); +// Finds a sanitized variant of filename, "name(N).ext" if needed, that neither an entry of dest_folder +// nor the download marker of another download uses. The marker at ignored_marker does not count. +// Returns true and the name in result, or false and the last name tried. +bool find_unused_filename(const boost::filesystem::path &dest_folder, const std::string &filename, + const boost::filesystem::path &ignored_marker, std::string &result); // File path / name / extension splitting utilities, working with UTF-8, // to be published to Perl. namespace PerlUtils { diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 9def5dad17..44fad7dc25 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -1321,6 +1321,31 @@ unsigned get_current_pid() #endif } +boost::filesystem::path download_marker_path(const boost::filesystem::path &dest_folder, const std::string &filename) +{ + return dest_folder / (filename + "." + std::to_string(get_current_pid()) + ".download"); +} + +bool find_unused_filename(const boost::filesystem::path &dest_folder, const std::string &filename, + const boost::filesystem::path &ignored_marker, std::string &result) +{ + // Probe the name that will be written, so a name the sanitizing maps onto an existing file is versioned too. + const std::string sanitized = sanitize_filename(filename); + const std::string extension = boost::filesystem::path(sanitized).extension().string(); + const std::string stem = sanitized.substr(0, sanitized.size() - extension.size()); + auto is_used = [&](const std::string &name) { + const boost::filesystem::path marker = download_marker_path(dest_folder, name); + return boost::filesystem::exists(dest_folder / name) || (marker != ignored_marker && boost::filesystem::exists(marker)); + }; + result = sanitized; + for (size_t version = 1; is_used(result); ++version) { + if (version > 999) + return false; + result = stem + "(" + std::to_string(version) + ")" + extension; + } + return true; +} + std::string per_user_temp_id() { #ifdef WIN32 diff --git a/src/slic3r/GUI/DownloaderFileGet.cpp b/src/slic3r/GUI/DownloaderFileGet.cpp index e20f7dfa93..b31aa29b4a 100644 --- a/src/slic3r/GUI/DownloaderFileGet.cpp +++ b/src/slic3r/GUI/DownloaderFileGet.cpp @@ -71,17 +71,6 @@ bool FileGet::is_subdomain(const std::string& url, const std::string& domain) return false; } -namespace { -unsigned get_current_pid() -{ -#ifdef WIN32 - return GetCurrentProcessId(); -#else - return ::getpid(); -#endif -} -} - // int = DOWNLOAD ID; string = file path wxDEFINE_EVENT(EVT_DWNLDR_FILE_COMPLETE, wxCommandEvent); // int = DOWNLOAD ID; string = error msg @@ -112,7 +101,6 @@ struct FileGet::priv priv(int ID, std::string&& url, const std::string& filename, wxEvtHandler* evt_handler, const boost::filesystem::path& dest_folder); void get_perform(); - std::string find_unused_filename(const std::string& filename) const; }; FileGet::priv::priv(int ID, std::string&& url, const std::string& filename, wxEvtHandler* evt_handler, const boost::filesystem::path& dest_folder) @@ -132,24 +120,6 @@ std::string extract_remote_filename(const std::string& str) { return ""; } } -// Returns a sanitized name that no file or other download in the destination folder uses yet, -// or an empty string when none is found. -std::string FileGet::priv::find_unused_filename(const std::string& filename) const -{ - const std::string extension = boost::filesystem::path(filename).extension().string(); - const std::string just_filename = filename.substr(0, filename.size() - extension.size()); - std::string final_filename = just_filename; - auto is_used = [this, &extension](const std::string& name) { - const boost::filesystem::path tmp_path = m_dest_folder / (name + extension + "." + std::to_string(get_current_pid()) + ".download"); - return boost::filesystem::exists(m_dest_folder / (name + extension)) || (tmp_path != m_tmp_path && boost::filesystem::exists(tmp_path)); - }; - for (size_t version = 1; is_used(final_filename); ++version) { - if (version > 999) - return {}; - final_filename = GUI::format("%1%(%2%)", just_filename, std::to_string(version)); - } - return sanitize_filename(final_filename + extension); -} void FileGet::priv::get_perform() { assert(m_evt_handler); @@ -163,10 +133,10 @@ void FileGet::priv::get_perform() std::string extension; if (m_written == 0) { - extension = (m_dest_folder / m_filename).extension().string(); std::string final_filename; + bool found = false; try { - final_filename = find_unused_filename(m_filename); + found = find_unused_filename(m_dest_folder, m_filename, m_tmp_path, final_filename); } catch (const boost::filesystem::filesystem_error& e) { wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_ERROR); @@ -175,17 +145,18 @@ void FileGet::priv::get_perform() m_evt_handler->QueueEvent(evt); return; } - if (final_filename.empty()) { + if (!found) { wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_ERROR); - evt->SetString(GUI::format_wxstr(L"Failed to find suitable filename. Last name: %1%." , (m_dest_folder / m_filename).string())); + evt->SetString(GUI::format_wxstr(L"Failed to find suitable filename. Last name: %1%." , (m_dest_folder / final_filename).string())); evt->SetInt(m_id); m_evt_handler->QueueEvent(evt); return; } m_filename = final_filename; + extension = boost::filesystem::path(m_filename).extension().string(); - m_tmp_path = m_dest_folder / (m_filename + "." + std::to_string(get_current_pid()) + ".download"); + m_tmp_path = download_marker_path(m_dest_folder, m_filename); wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_NAME_CHANGE); evt->SetString(boost::nowide::widen(m_filename)); @@ -233,16 +204,17 @@ void FileGet::priv::get_perform() std::string filename = extract_remote_filename(header); if (!filename.empty()) { // The name comes from the server: keep it inside the destination folder and never - // replace an existing file. Fall back to the URL-derived name if nothing usable remains. + // replace an existing file. Keep the current name if nothing usable remains. filename = sanitize_file_basename(filename); + std::string unused; try { - if (!filename.empty()) - filename = find_unused_filename(filename); + if (filename.empty() || !find_unused_filename(m_dest_folder, filename, m_tmp_path, unused)) + unused.clear(); } catch (const boost::filesystem::filesystem_error&) { - filename.clear(); + unused.clear(); } - if (!filename.empty()) - m_filename = filename; + if (!unused.empty()) + m_filename = unused; dest_path = m_dest_folder / m_filename; wxCommandEvent* evt = new wxCommandEvent(EVT_DWNLDR_FILE_NAME_CHANGE); evt->SetString(boost::nowide::widen(m_filename)); diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index b71597c6d6..2111173326 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -163,12 +163,13 @@ TEST_CASE("sanitize_file_basename keeps only a plain file name from an untrusted {"C:x.3mf", "C_x.3mf"}, {"/etc/x", "x"}, {"a/b\\c.gcode", "c.gcode"}, + {"x:stream", "x_stream"}, // no NTFS alternate data stream + {"x.", "x."}, + {".3mf", ".3mf"}, {unicode, unicode}, })); CAPTURE(input); - const std::string name = sanitize_file_basename(input); - CHECK(name == expected); - CHECK(name.find_first_of("/\\:") == std::string::npos); + CHECK(sanitize_file_basename(input) == expected); } TEST_CASE("sanitize_file_basename rejects names that do not name a file", "[Utils]") { @@ -176,3 +177,69 @@ TEST_CASE("sanitize_file_basename rejects names that do not name a file", "[Util CAPTURE(input); CHECK(sanitize_file_basename(input).empty()); } + +namespace { +void touch(const boost::filesystem::path &path) { std::ofstream(path.string()) << "existing"; } +std::string file_contents(const boost::filesystem::path &path) +{ + std::ifstream file(path.string()); + return std::string(std::istreambuf_iterator(file), std::istreambuf_iterator()); +} +} // namespace + +TEST_CASE("find_unused_filename keeps a name nothing uses", "[Utils]") { + ScopedTemporaryDir dir; + std::string name; + REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name)); + CHECK(name == "model.3mf"); +} + +TEST_CASE("find_unused_filename versions a name an existing file uses", "[Utils]") { + ScopedTemporaryDir dir; + touch(dir.path() / "model.3mf"); + std::string name; + REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name)); + CHECK(name == "model(1).3mf"); +} + +TEST_CASE("find_unused_filename versions a name that maps onto an existing file once sanitized", "[Utils]") { + ScopedTemporaryDir dir; + touch(dir.path() / "my_model.3mf"); + const std::string input = GENERATE(as{}, "my?model.3mf", "my:model.3mf", "my*model.3mf"); + CAPTURE(input); + std::string name; + REQUIRE(find_unused_filename(dir.path(), input, {}, name)); + CHECK(name == "my_model(1).3mf"); + CHECK(file_contents(dir.path() / "my_model.3mf") == "existing"); +} + +TEST_CASE("find_unused_filename treats the marker of another download as used", "[Utils]") { + ScopedTemporaryDir dir; + touch(download_marker_path(dir.path(), "model.3mf")); + std::string name; + REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name)); + CHECK(name == "model(1).3mf"); +} + +TEST_CASE("find_unused_filename ignores the marker of the download asking", "[Utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path own_marker = download_marker_path(dir.path(), "model.3mf"); + touch(own_marker); + std::string name; + REQUIRE(find_unused_filename(dir.path(), "model.3mf", own_marker, name)); + CHECK(name == "model.3mf"); +} + +TEST_CASE("find_unused_filename gives up after 999 versions", "[Utils]") { + ScopedTemporaryDir dir; + touch(dir.path() / "model.3mf"); + for (int version = 1; version < 999; ++version) + touch(dir.path() / ("model(" + std::to_string(version) + ").3mf")); + std::string name; + REQUIRE(find_unused_filename(dir.path(), "model.3mf", {}, name)); + CHECK(name == "model(999).3mf"); + + touch(dir.path() / name); + REQUIRE_FALSE(find_unused_filename(dir.path(), "model.3mf", {}, name)); + CHECK(name == "model(999).3mf"); +}