Comments from lm regarding previous commit

2 files changed, 25 insertions, 10 deletions

diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp
index 3c0d52bd1..4278766b4 100644
--- a/src/slic3r/GUI/GUI_App.cpp
+++ b/src/slic3r/GUI/GUI_App.cpp
@@ -1242,7 +1242,7 @@ bool GUI_App::on_init_inner()
         preset_updater = new PresetUpdater();
         Bind(EVT_SLIC3R_VERSION_ONLINE, &GUI_App::on_version_read, this);
         Bind(EVT_SLIC3R_EXPERIMENTAL_VERSION_ONLINE, [this](const wxCommandEvent& evt) {
-            app_config->save();
+            app_config->save();//lm:What is the purpose?
             if (this->plater_ != nullptr && app_config->get("notify_release") == "all") {
                 std::string evt_string = into_u8(evt.GetString());
                 if (*Semver::parse(SLIC3R_VERSION) < *Semver::parse(evt_string)) {
@@ -1257,6 +1257,7 @@ bool GUI_App::on_init_inner()
             }
             });
         Bind(EVT_SLIC3R_APP_DOWNLOAD_PROGRESS, [this](const wxCommandEvent& evt) {
+            //lm:This does not force a render. The progress bar only updateswhen the mouse is moved.
             if (this->plater_ != nullptr)
                 this->plater_->get_notification_manager()->set_download_progress_percentage((float)std::stoi(into_u8(evt.GetString())) / 100.f );
         });
@@ -3354,6 +3355,7 @@ void GUI_App::app_updater(bool from_user)
     }
     if (boost::filesystem::exists(app_data.target_path))
     {
+        //lm:UI confirmation dialog?
         BOOST_LOG_TRIVIAL(error) << "App download: File on target path already exists and will be overwritten. Path: " << app_data.target_path;
     }
     // start download
diff --git a/src/slic3r/Utils/AppUpdater.cpp b/src/slic3r/Utils/AppUpdater.cpp
index 8b3a2f9f0..26bf883de 100644
--- a/src/slic3r/Utils/AppUpdater.cpp
+++ b/src/slic3r/Utils/AppUpdater.cpp
@@ -35,7 +35,7 @@ namespace {
 			// run updater. Original args: /silent -restartapp prusa-slicer.exe -startappfirst
 
 			// Using quoted string as mentioned in CreateProcessW docs, silent execution parameter.
-			std::wstring wcmd = L"\"" + path.wstring();
+			std::wstring wcmd = L"\"" + path.wstring(); //lm: closing quote?
 
 			// additional information
 			STARTUPINFOW si;
@@ -64,7 +64,7 @@ namespace {
 				return true;
 			}
 			else {
-				BOOST_LOG_TRIVIAL(error) << "Failed to run " << wcmd;
+				BOOST_LOG_TRIVIAL(error) << "Failed to run " << wcmd; //lm: maybe a UI error message?
 			}
 		}
 		return false;
@@ -86,6 +86,8 @@ namespace {
 	{
 		// this command can run the installer exe as well, but is it better than CreateProcessW?
 		ShellExecuteW(NULL, NULL, path.wstring().c_str(), NULL, NULL, SW_SHOWNORMAL);
+		//lm:I would make it explicit that the folder should be opened.
+		//lm:Also, this always returns true.
 		return true;
 	}
 
@@ -100,6 +102,9 @@ namespace {
 		wxString command = "xdg-user-dir DOWNLOAD";
 		wxArrayString output;
 		 
+		 //lm:It might be a good idea to make the following a separate function in GUI_Utils or something
+		 // It is already used in four different places in almost the same way.
+
 		//Check if we're running in an AppImage container, if so, we need to remove AppImage's env vars,
 		// because they may mess up the environment expected by the file manager.
 		// Mostly this is about LD_LIBRARY_PATH, but we remove a few more too for good measure.
@@ -131,7 +136,7 @@ namespace {
 			::wxExecute(command, output);
 		}
 		if (output.GetCount() > 0) {
-			return boost::nowide::narrow(output[0]);
+			return boost::nowide::narrow(output[0]); //lm:I would use wxString::ToUTF8(), although on Linux, nothing at all should work too.
 		}
 		return std::string();
 	}
@@ -141,6 +146,8 @@ namespace {
 		if (boost::filesystem::is_directory(path)) {
 			const char *argv[] = { "xdg-open", path.string().c_str(), nullptr };
 
+			//lm:This is a copy of desktop_open_datadir_folder, it would make sense to instead call it.
+
 			// Check if we're running in an AppImage container, if so, we need to remove AppImage's env vars,
 			// because they may mess up the environment expected by the file manager.
 			// Mostly this is about LD_LIBRARY_PATH, but we remove a few more too for good measure.
@@ -264,7 +271,7 @@ AppUpdater::priv::priv() :
 	if (!downloads_path.empty()) {
 		m_default_dest_folder = std::move(downloads_path);
 	}
-	BOOST_LOG_TRIVIAL(error) << "Default download path: " << m_default_dest_folder;
+	BOOST_LOG_TRIVIAL(error) << "Default download path: " << m_default_dest_folder; //lm:Is this an error?
 	
 }
 
@@ -277,7 +284,7 @@ bool  AppUpdater::priv::http_get_file(const std::string& url, size_t size_limit,
 			// progress function returns true as success (to continue) 
 			cancel = (this->m_cancel ? true : !progress_fn(std::move(progress)));
 			if (cancel) {
-				error_message = GUI::format("Error getting: `%1%`: Download was canceled.",
+				error_message = GUI::format("Error getting: `%1%`: Download was canceled.", //lm:typo
 					url);
 				BOOST_LOG_TRIVIAL(debug) << "AppUpdater::priv::http_get_file message: "<< error_message;
 			}
@@ -311,7 +318,7 @@ boost::filesystem::path AppUpdater::priv::download_file(const DownloadAppData& d
 		return boost::filesystem::path();
 	}
 	std::string error_message;
-	bool res = http_get_file(data.url, 80 * 1024 * 1024 //TODO: what value here
+	bool res = http_get_file(data.url, 80 * 1024 * 1024 //TODO: what value here //lm:I don't know, but larger. The binaries will grow.
 		// on_progress
 		, [&last_gui_progress, expected_size](Http::Progress progress) {
 			// size check
@@ -322,7 +329,7 @@ boost::filesystem::path AppUpdater::priv::download_file(const DownloadAppData& d
 				evt->SetString(message);
 				GUI::wxGetApp().QueueEvent(evt);
 				return false;
-			} else if (progress.dltotal > 0 && progress.dltotal < expected_size) {
+			} else if (progress.dltotal > 0 && progress.dltotal < expected_size) { //lm:When will this happen? Is that not an error?
 				BOOST_LOG_TRIVIAL(error) << GUI::format("Downloading new %1% has incorrect size. The download will continue. \nExpected size: %2%\nDownload size: %3%", SLIC3R_APP_NAME, expected_size, progress.dltotal);;
 			}
 			// progress event
@@ -341,6 +348,7 @@ boost::filesystem::path AppUpdater::priv::download_file(const DownloadAppData& d
 			// Size check. Does always 1 char == 1 byte?
 			size_t body_size = body.size(); 
 			if (body_size != expected_size) {
+				//lm:UI message?
 				BOOST_LOG_TRIVIAL(error) << "Downloaded file has wrong size. Expected size: " <<  expected_size << " Downloaded size: " << body_size;
 				return false;
 			}
@@ -366,7 +374,7 @@ boost::filesystem::path AppUpdater::priv::download_file(const DownloadAppData& d
 	{
 		if (this->m_cancel)
 		{
-			BOOST_LOG_TRIVIAL(error) << error_message;
+			BOOST_LOG_TRIVIAL(error) << error_message; //lm:Is this an error?
 		} else {
 			std::string message = GUI::format("Downloading new %1% has failed:\n%2%", SLIC3R_APP_NAME, error_message);
 			BOOST_LOG_TRIVIAL(error) << message;
@@ -403,6 +411,9 @@ void AppUpdater::priv::version_check(const std::string& version_check_url)
 		}
 		, error_message
 	);
+	//lm:In case the internet is not available, it will report no updates if run by user.
+	// We might save a flag that we don't know or try to run the version_check again, reporting
+	// the failure.
 	if (!res)
 		BOOST_LOG_TRIVIAL(error) << "Failed to download version file: " << error_message;
 }
@@ -444,6 +455,8 @@ void AppUpdater::priv::parse_version_string(const std::string& body)
 #else
 			"release:osx"
 #endif
+//lm:Related to the ifdefs. We should also support BSD, which behaves similar to Linux in most cases.
+// Unless you have a reason not to, I would consider doing _WIN32, elif __APPLE__, else ... Not just here.
 			) {
 			for (const auto& data : section.second) {
 				if (data.first == "url") {
@@ -517,7 +530,7 @@ void AppUpdater::priv::parse_version_string(const std::string& body)
 	GUI::wxGetApp().QueueEvent(evt);
 }
 
-#if 0
+#if 0 //lm:is this meant to be ressurected?
 void AppUpdater::priv::parse_version_string_old(const std::string& body) const
 {