@techee requested changes on this pull request.

Thanks for the patch - looks good to me in general except for some of the notes below. I personally think that having such a feature in Geany (i.e. multi-tab Scrilbble backed by Scintilla with all the Geany features) would be nice, but depends what others think.

I didn't go through all the things in detail and this is just a rough quick review.

Includes one small extension to API (new function in main.h) - if i understood correctly, this does not require incrementing any versions

Yes, it does, but it can be done one everything is ironed-out.

In plugins/saveactions.c:

> +		ui_set_statusbar(TRUE, "%s", message);
+		g_warning("%s", message);
+		g_free(message);
+		g_free(new_filename);
+		return;
+	}
+
+	close(fd); /* close the returned file descriptor as we only need the filename */
+
+	doc->file_name = new_filename;
+
+	if (ft != NULL && ft->id == GEANY_FILETYPES_NONE)
+		document_set_filetype(doc, filetypes_lookup_by_name(default_ft));
+
+	/* force saving the file to enable all the related actions(tab name, filetype, etc.) */
+	document_save_file(doc, TRUE);

Couldn't document_save_file_as be used here instead? It should do the filetype detection for you and shouldn't require the kind of hacky doc->file_name = new_filename. (I know this was copied from the previous code but this might be better unless I overlook something.)

In plugins/saveactions.c:

> +
+	if (file_path == NULL)
+	{
+		return FALSE;
+	}
+
+	filename = g_path_get_basename(file_path);
+	matched = is_temp_saved_file(filename);
+
+	g_free(filename);
+
+	return matched;
+}
+
+
+static void load_all_temp_files_into_editor(const gchar *path)

Is this whole function needed? Couldn't persistent temp files just use the standard Geany session management so they are stored either in the global session or a project based session and restored from it automatically by Geany?

In plugins/saveactions.c:

> +			}
+			else
+			{
+				g_remove(locale_file_path);
+
+				ui_set_statusbar(TRUE, _("Empty temp file %s was deleted"), short_filename);
+			}
+
+			g_free(short_filename);
+			g_free(locale_file_path);
+		}
+	}
+}
+
+
+static void persistent_temp_files_document_before_save_cb(GObject *obj, GeanyDocument *doc, gpointer user_data)

I guess you are trying to handle the "save as" case here, right? Then you could use the newly added document-before-save-as:

#3572

In plugins/saveactions.c:

> @@ -347,8 +706,13 @@ static gboolean on_document_focus_out(GObject *object, GeanyEditor *editor,
 PluginCallback plugin_callbacks[] =
 {
 	{ "document-new", (GCallback) &instantsave_document_new_cb, FALSE, NULL },
+	{ "document-new", (GCallback) &persistent_temp_files_document_new_cb, FALSE, NULL },

I'd suggest having a single handler which then dispatches it to either the instantsave case or the persistent temp file case. Same for document-save below.

In plugins/saveactions.c:

> +		locale_file_path = g_build_path(G_DIR_SEPARATOR_S, path, filename, NULL);
+
+		if (is_temp_saved_file(utf8_filename))
+		{
+			document_open_file(locale_file_path, FALSE, NULL, NULL);
+		}
+
+		g_free(utf8_filename);
+		g_free(locale_file_path);
+	}
+
+	g_dir_close(dir);
+}
+
+
+static gboolean file_exists(gchar *utf8_file_path)

Instead of this function you can just check doc->real_path != NULL.

In plugins/saveactions.c:

> +	}
+}
+
+
+static void persistent_temp_files_document_close_cb(GObject *obj, GeanyDocument *doc, gpointer user_data)
+{
+	gchar *file_path, *locale_file_path, *short_filename;
+
+	file_path = doc->file_name;
+
+	if (enable_persistent_temp_files && file_path != NULL && ! geany_is_quitting())
+	{
+		if (is_temp_saved_file_doc(doc) && file_exists(doc->file_name))
+		{
+			short_filename = document_get_basename_for_display(doc, -1);
+			locale_file_path = utils_get_locale_from_utf8(file_path);

You can use doc->real_path instead.

In src/libmain.c:

> @@ -462,6 +462,18 @@ gboolean main_is_realized(void)
 }
 
 
+/**
+ *  Checks whether Geany is quitting completely right now.
+ *
+ *  @return @c TRUE if the Geany is exiting right now or @c FALSE otherwise.
+ **/
+GEANY_API_SYMBOL
+gboolean geany_is_quitting(void)

I don't think you want main_status.quitting here - IMO you want main_status.closing_all instead. Note that the plugin should work both for:

the default session (list of open files) when no project is open
project session when some project is open

I think you can just safely rely on Geany's session management and let Geany store persistent temp files into the corresponding session and reopen them from there.

You need to use main_status.closing_all to detect situations when a session is being closed including all its files and this can happen either when a project is closed (for project sessions) or when Geany quits (default session).

In plugins/saveactions.c:

>  			return;
 		}
 
-		close(fd); /* close the returned file descriptor as we only need the filename */
+		old_file_name = g_path_get_basename(old_file_path);
+
+		if (is_temp_saved_file(old_file_name) && ! g_str_equal(old_file_path, new_file_path))
+		{
+			/* we have to store old/new filename pair in a global hashtable to be able to somehow
+			pass it to document-save callback that is called directly after this one */
+			g_hash_table_insert(

You can use

gpointer plugin_get_document_data(struct GeanyPlugin *plugin,
	struct GeanyDocument *doc, const gchar *key);

void plugin_set_document_data(struct GeanyPlugin *plugin, struct GeanyDocument *doc,
	const gchar *key, gpointer data);

to assign/retrieve arbitrary data to/from a document which is probably more elegant in this case.

In plugins/saveactions.c:

> +
+	if (file_path == NULL)
+	{
+		return FALSE;
+	}
+
+	filename = g_path_get_basename(file_path);
+	matched = is_temp_saved_file(filename);
+
+	g_free(filename);
+
+	return matched;
+}
+
+
+static void load_all_temp_files_into_editor(const gchar *path)

Seems to work when I comment-out this function call.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.