@techee requested changes on this pull request.

@LiquidCake Thanks, looks very good in general!

I didn't go through the config-dialog-related code yet so the review comments aren't complete.

I think there's one main theme of the review comments - function names and variable names got brutally long with the persistent_untitled_document_file prefix. I'd suggest the following - dropping untitled, file and shortening document to doc only so the above becomes persistent_doc - I think this is sufficient and actually improves readability of the code. It would be best if this naming could be used consistently everywhere.

Or if you have some other suggestion, please make it.

(Maybe I said something slightly different in some of the review comments, my opinion kind of evolved while I was reviewing the code.)

In any case, functionality-wise it seems to work very well. I was also curious about the situation where someone has "instant save" enabled from previous Geany versions and upgrades to new Geany version - this settings is still preserved with your code so there should be seamless transition for existing users.

In plugins/saveactions.c:

>  }
 pref_widgets;
 
 
-#define PERSISTENT_TEMP_FILE_NAME_PREFIX "gtemp_"
+#define PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX "unttld_"

I would suggest untitled_ here.

In plugins/saveactions.c:

> @@ -285,19 +296,26 @@ static void backupcopy_document_save_cb(GObject *obj, GeanyDocument *doc, gpoint
 }
 
 
+static GeanyFiletype *get_doc_filetype_or_default(GeanyDocument *doc) {

I'd drop the _or_default from the function name.

In plugins/saveactions.c:

>  		{
 			dialogs_show_msgbox(GTK_MESSAGE_ERROR,
-				_("Persistent temp files directory does not exist or is not writable."));
+				_("Persistent untitled documents files directory does not exist or is not writable."));

I'd drop "files" from the message.

In plugins/saveactions.c:

>  	if (dir == NULL)
 	{
-		dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("Persistent temp files directory not found"));
+		dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("Persistent untitled documents files directory not found"));

Drop "files", documents->document.

In plugins/saveactions.c:

> @@ -46,51 +47,93 @@ PLUGIN_SET_INFO(_("Save Actions"), _("This plugin provides different actions rel
 enum
 {
 	NOTEBOOK_PAGE_AUTOSAVE = 0,
-	NOTEBOOK_PAGE_INSTANTSAVE,
-	NOTEBOOK_PAGE_BACKUPCOPY
+	NOTEBOOK_PAGE_BACKUPCOPY,
+	NOTEBOOK_PAGE_UNTITLEDDOCUMENTSAVE

In the code, when a variable, function name, etc. contains "document", I'd suggest using just "doc" instead - they are getting a bit too long.

(in user-facing strings, keep using "document")

In plugins/saveactions.c:

>  
 		/* force saving the file to enable all the related actions(tab name, filetype, etc.) */
 		document_save_file(doc, TRUE);
-    }
+	}
+}
+
+
+static gboolean is_persistent_untitled_doc_file_name(const gchar *filename)
+{
+	if (filename == NULL)
+		return FALSE;
+
+	return g_str_has_prefix(filename, PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX);
+}
+
+
+static gboolean is_persistent_untitled_doc_file(const gchar *file_path_utf8)

I'd suggest calling it is_persistent_untitled_doc_file_path.

In plugins/saveactions.c:

> @@ -258,19 +296,26 @@ static void backupcopy_document_save_cb(GObject *obj, GeanyDocument *doc, gpoint
 }
 
 
+static GeanyFiletype *get_doc_filetype_or_default(GeanyDocument *doc) {
+	GeanyFiletype *ft = doc->file_type;
+
+	if (ft == NULL || ft->id == GEANY_FILETYPES_NONE)
+		/* ft is NULL when a new file without template was opened, so use the
+			* configured default file type */
+		ft = filetypes_lookup_by_name(untitled_document_save_default_ft);
+	

Do Document->Strip trailing spaces - there are a few of them around.

In plugins/saveactions.c:

> +	g_free(filename);
+
+	return matched;
+}
+
+
+static gchar* create_new_persistent_untitled_doc_file_name(GeanyDocument *doc)
+{
+	gint i;
+	gchar *extension_postfix;
+	GeanyFiletype *filetype = get_doc_filetype_or_default(doc);
+
+	if (filetype != NULL && !EMPTY(filetype->extension))
+		extension_postfix = g_strconcat(".", filetype->extension, NULL);
+	else
+		extension_postfix = "";

I'd rather suggest g_strdup("") and using g_free() below in all cases.

In plugins/saveactions.c:

> +	gchar *extension_postfix;
+	GeanyFiletype *filetype = get_doc_filetype_or_default(doc);
+
+	if (filetype != NULL && !EMPTY(filetype->extension))
+		extension_postfix = g_strconcat(".", filetype->extension, NULL);
+	else
+		extension_postfix = "";
+
+	for (i = 1; i < 1000; i++)
+	{
+		gchar *next_file_name, *next_file_path;
+
+		next_file_name = g_strdup_printf("%s%d%s", PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX, i, extension_postfix);
+		next_file_path = g_strdup_printf("%s%c%s", persistent_untitled_docs_target_dir, G_DIR_SEPARATOR, next_file_name);
+
+		gboolean file_exists = g_file_test(next_file_path, G_FILE_TEST_EXISTS);

gboolean file_exists to the beginning of the block.

In plugins/saveactions.c:

> +		extension_postfix = "";
+
+	for (i = 1; i < 1000; i++)
+	{
+		gchar *next_file_name, *next_file_path;
+
+		next_file_name = g_strdup_printf("%s%d%s", PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX, i, extension_postfix);
+		next_file_path = g_strdup_printf("%s%c%s", persistent_untitled_docs_target_dir, G_DIR_SEPARATOR, next_file_name);
+
+		gboolean file_exists = g_file_test(next_file_path, G_FILE_TEST_EXISTS);
+
+		g_free(next_file_path);
+
+		if (file_exists) {
+			g_free(next_file_name);
+		} else {

both for if and else { on separate line

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);

I'd say use msgwin_status_add for this - the file wasn't permanently lost in this case so the message in the status bar could be confusing for users.

In plugins/saveactions.c:

> +	if (! geany_is_closing_all_documents() && doc->real_path != NULL && is_persistent_untitled_doc_file(doc->file_name))
+	{
+		gchar *short_filename = document_get_basename_for_display(doc, -1);
+
+		if (! document_is_empty(doc))
+		{
+			show_unsaved_dialog_for_persistent_untitled_docs_tab_closing(
+				doc,
+				short_filename
+			);
+		}
+		else
+		{
+			g_remove(doc->real_path);
+
+			ui_set_statusbar(TRUE, _("Empty untitled document file %s was deleted"), short_filename);

Use msgwin_status_add here.

In plugins/saveactions.c:

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

This is the real destructive action so using ui_set_statusbar() is alright here.

In plugins/saveactions.c:

>  };
 
+enum
+{
+	NOTEBOOK_UNTITLEDDOCUMENTSAVE_RADIO_DISABLED = 0,

I'd suggest NOTEBOOK_UNTITLED_DOC_RADIO_DISABLED and similarly below.

In plugins/saveactions.c:

>  	GtkWidget *backupcopy_entry_dir;
 	GtkWidget *backupcopy_entry_time;
 	GtkWidget *backupcopy_spin_dir_levels;
+
+	GtkWidget *untitled_document_save_disabled_radio;
+	GtkWidget *untitled_document_save_instantsave_radio;
+	GtkWidget *untitled_document_save_persistent_radio;
+
+	GtkWidget *untitled_document_save_ft_combo;

I'd drop _save from the 4 variables so it gets a little shorter.

In plugins/saveactions.c:

> +			return next_file_name;
+		}
+	}
+
+	if (strlen(extension_postfix))
+		g_free(extension_postfix);
+
+	return NULL;
+}
+
+
+static void persistent_untitled_document_new_cb(GObject *obj, GeanyDocument *doc, gpointer user_data)
+{
+	if (doc->file_name == NULL)
+	{
+		gchar *pers_docs_files_dir_utf8, *new_pers_doc_file_name_utf8, *new_pers_doc_file_path_utf8;

these are local variables and already in "persistent untitled" function so there's no need for "pres_doc_files" and similar prefix - it makes the code hard to read.

Similarly in all other cases - for global variables/function names it's probably alright to be clear to what they apply but there's no need for it for local variables.

In plugins/saveactions.c:

> + * @return always FALSE = Just a one shot execution
+ *
+ */
+static gboolean open_document_once_idle(gpointer p_locale_file_path)
+{
+	gchar *locale_file_path = (gchar *) p_locale_file_path;
+
+	document_open_file(locale_file_path, FALSE, NULL, NULL);
+
+	g_free(locale_file_path);
+
+	return FALSE;
+}
+
+
+static gint run_unsaved_dialog_for_persistent_untitled_docs_tab_closing(const gchar *msg, const gchar *msg2)

Something shorter please, this isn't Java :-)

In src/libmain.c:

> @@ -462,6 +462,18 @@ gboolean main_is_realized(void)
 }
 
 
+/**
+ *  Checks whether Geany is 'closing all' documents right now.
+ *
+ *  @return @c TRUE if the Geany is 'closing all' documents right now or @c FALSE otherwise.
+ **/
+GEANY_API_SYMBOL
+gboolean geany_is_closing_all_documents(void)

Bump API for this change.

In plugins/saveactions.c:

> @@ -475,7 +514,7 @@ static gint run_unsaved_dialog_for_persistent_temp_files_tab_closing(const gchar
 }
 
 
-static void show_unsaved_dialog_for_persistent_temp_files_tab_closing(
+static void show_unsaved_dialog_for_persistent_untitled_docs_tab_closing(

Also, make it shorter a bit - it doesn't have to say all the details and such functions are hard to read in the code.

In plugins/saveactions.c:

> @@ -614,13 +653,21 @@ static void load_all_temp_files_into_editor(void)
 }
 
 
-static gboolean load_all_temp_files_idle(gpointer data)
+static gboolean load_all_persistent_untitled_doc_files_idle(gpointer data)

-> load_persistent_docs_idle ?

In plugins/saveactions.c:

> @@ -614,13 +653,21 @@ static void load_all_temp_files_into_editor(void)
 }
 
 
-static gboolean load_all_temp_files_idle(gpointer data)
+static gboolean load_all_persistent_untitled_doc_files_idle(gpointer data)
+{
+	load_all_persistent_untitled_doc_files_into_editor();
+
+	return FALSE;
+}
+
+
+static gboolean load_all_persistent_untitled_doc_files_and_reopen_current_idle(gpointer data)

-> reload_persistent_docs_on_session_change_idle?

In plugins/saveactions.c:

>  	{
 		/* switch functionality off, so invalid target directory cannot be actually used */
-		enable_persistent_temp_files = FALSE;
-		g_key_file_set_boolean(config, "saveactions", "enable_persistent_temp_files", FALSE);
-		gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(pref_widgets.checkbox_enable_persistent_temp_files), FALSE);
+		enable_persistent_untitled_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_document_save_disabled_radio), TRUE);

Double empty line within function - make it just single empty line (I think I've seen it at other places too).

In plugins/saveactions.c:

> +	ret = gtk_dialog_run(GTK_DIALOG(dialog));
+
+	gtk_widget_destroy(dialog);
+
+	return ret;
+}
+
+
+static void show_unsaved_dialog_for_persistent_temp_files_tab_closing(
+	GeanyDocument *doc, const gchar *short_filename)
+{
+	gchar *msg, *old_file_path, *locale_file_path;
+	const gchar *msg2;
+	gint response;
+
+	msg = g_strdup_printf(_("The file '%s' is not saved."), short_filename);

Alright, we have the "correct naming" now so I'd say "Untitle document %s is not saved" (I haven't checked how other dialogs look like - should the final . be there? Possibly yes, I just don't know.)

—
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/2430731213@github.com>