@techee requested changes on this pull request.

Looks promising! Variable name lengths are under control now :-)

I have a few minor comments but apart from them, I think we are close to merging this! 🎉

In plugins/saveactions.c:

>  			g_free(next_file_name);
-		} else {
+		}
+		else
+		{
 			if (strlen(extension_postfix))

Because of the added g_strdup("") above this check should be removed too.

In plugins/saveactions.c:

>  		config, "saveactions", "enable_persistent_untitled_documents", FALSE);
 
+	if (enable_instantsave && enable_persistent_docs)
+	{
+		dialogs_show_msgbox(GTK_MESSAGE_ERROR,
+			_("Invalid config file state: multiple features of 'Persistent Untitled Documents' are enabled at once"));
+
+		enable_instantsave = FALSE;
+	}

Yes, this is what I had in mind, thanks. I'd just drop the message box - it may just be confusing for users what to do. Since the plugin does a reasonable thing by itself, I think it's OK without the message.

In plugins/saveactions.c:

> @@ -551,7 +554,7 @@ static void show_unsaved_dialog_for_persistent_untitled_docs_tab_closing(
 		case GTK_RESPONSE_NO:
 			g_remove(doc->real_path);
 
-			ui_set_statusbar(TRUE, _("Untitled document file %s was deleted"), short_filename);
+			msgwin_status_add(_("Untitled document file %s was deleted"), short_filename);

Not sure if I made a mistake before or you but I think ui_set_statusbar should be used here as this is the destructive action.

In plugins/saveactions.c:

>  	g_signal_emit_by_name(pref_widgets.checkbox_enable_backupcopy, "toggled");
+	g_signal_emit_by_name(pref_widgets.untitled_doc_disabled_radio, "toggled");
+	g_signal_emit_by_name(pref_widgets.untitled_doc_instantsave_radio, "toggled");
+	g_signal_emit_by_name(pref_widgets.untitled_doc_persistent_radio, "toggled");

Should be probably sufficient to emit it for just one of them, right?

In plugins/saveactions.c:

> +		g_free(tmp);
+	}
+
+	tmp = utils_get_setting_string(config, "untitled_document_save", "persistent_untitled_documents_target_dir", NULL);
+	SETPTR(tmp, utils_get_locale_from_utf8(tmp));
+	/* Set target dir variable with value from config, regardless if dir is valid or not */
+	SETPTR(persistent_docs_target_dir, g_strdup(tmp));
+
+	if (enable_persistent_docs && ! is_directory_accessible(tmp))
+	{
+		/* switch functionality off, so invalid target directory cannot be actually used */
+		enable_persistent_docs = FALSE;
+		g_key_file_set_boolean(config, "saveactions", "enable_persistent_untitled_documents", FALSE);
+
+		gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(pref_widgets.untitled_doc_disabled_radio), TRUE);
+		gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(pref_widgets.untitled_doc_persistent_radio), FALSE);

This line should not be necessary I believe because of the previous line and because it's a radio.

In plugins/saveactions.c:

>  			}
+		} else {

} and { on separate lines

In plugins/saveactions.c:

>  			{
-				g_key_file_set_string(config, "backupcopy", "backup_dir", backupcopy_text_dir);
+				/* If target dir path ends with dir separator - we consider it invalid */

Why? If it's a problem somewhere, it can be corrected by the plugin easily:

persistent_docs_text_dir[strlen(persistent_docs_text_dir) - 1] = '\0';

In plugins/saveactions.c:

>  		if (enable_instantsave)
 		{
 			if (EMPTY(instantsave_text_dir))
 			{
-				g_key_file_set_string(config, "instantsave", "target_dir", "");
+				g_key_file_set_string(config, "untitled_document_save", "instantsave_target_dir", "");

I think the whole plugin would deserve to define section and config names as macros - this is a bit error-prone.

Probably not necessary as part of this PR, I might open a separate PR with this afterwards.

In plugins/saveactions.c:

> +
+	new_file_path_utf8 = DOC_FILENAME(doc);
+	old_file_path_utf8 = plugin_get_document_data(geany_plugin, doc, "file-name-before-save-as");
+
+	if (old_file_path_utf8 != NULL)
+	{
+		if (is_persistent_untitled_doc_file(old_file_path_utf8)
+			&& ! g_str_equal(old_file_path_utf8, new_file_path_utf8))
+		{
+			/* remove untitled doc file if it was saved as some other file */
+			gchar *locale_old_file_path = utils_get_locale_from_utf8(old_file_path_utf8);
+			g_remove(locale_old_file_path);
+
+			g_free(locale_old_file_path);
+
+			ui_set_statusbar(TRUE, _("Untitled document file %s was deleted"), old_file_path_utf8);

Yes, it was you who made a mistake :-). Compared to the case above, msgwin_status_add should be used here.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <geany/geany/pull/3911/review/2454576131@github.com>