@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.

• 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.)

+ 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?

• 	}
  

+ 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`:

https://github.com/geany/geany/pull/3572

@@ -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.

• 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`.

• }

+} + + +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.

@@ -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).

	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 ```C 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.

+ 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.

@techee commented on this pull request.

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

Maybe one more idea - wouldn't it be better to introduce this feature just as a special mode of "Instant save"? Because I think that's what this feature really is - it just picks the file names automatically, prevents some accidental tab closing but it's instant save otherwise. It would also prevent possible confusion why the Enable button in preferences gets disabled.

@techee commented on this pull request.

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

Rather better would be buffer saving, all modified buffers get saved and can be restored on restart. Saving buffers means that there is no need to do encoding conversion, or trailing space removal or any of the other modifications that are made on save and just write metadata (filename if it exists and other per document settings) as well. That way documents both with and without names would be safe. Might need to be in core to interact properly with existing session management, although that may also be simplified.

Agree in principle but someone has to rewrite the session management in Geany and someone has to review and approve it. And until something like that happens, possibly imperfect solution like this PR is just fine IMO and can be removed if we have something better.

@LiquidCake commented on this pull request.

@@ -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 },

done

@LiquidCake commented on this pull request.

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

about extending Instantsave - this was an initial idea i tried, but then i decided to do a separate function since unification becomes too complicated both in terms of code and in terms of usage - e.g. instantsave's target directory has a bit different usecase then ours, and there are other complications.

Btw i changed mutual exclusion of instantsave vs temp_files from blocking one another's checkbox to just automatically de-activating one when another is activated. This seems a good-enough UX to me as newly created files start to look differently when you switch mode anyway

@techee commented on this pull request.

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

about extending Instantsave - this was an initial idea i tried, but then i decided to do a separate function since unification becomes too complicated both in terms of code and in terms of usage - e.g. instantsave's target directory has a bit different usecase then ours, and there are other complications.

OK, I noticed this post only now, my previous post is a bit outdated then.

I agree the semantics of the target directory are a bit different so maybe not a good idea to merge these two. But I'd still try to unify the two features a bit. First, I think the "Instant save" tab naming is quite horrible (I had to look up in Geany's documentation what it really does). So I'd suggest calling it something like "Empty files" which would contain 2 radio buttons: - Save on creation (which would serve for instant save settings) - this would contain the target directory - Use as Scribble - it would contain the current temp files settings

The settings in each group would be grayed-out as necessary when the other feature is enabled, default filetype combo box could be below and be shared by both.

@LiquidCake commented on this pull request.

@@ -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 guess this is doable. Not sure we necessary want to allow picking default filetype for temp files but maybe it can be useful in some case

@techee requested changes on this pull request.

Looks pretty good, I've just added a few more comments.

There are two things that should still be resolved: 1. The naming of this feature and its placement in preferences 2. How it should behave on project opens/closes.

For (1) my current suggestion is (if others dislike Scribble) "Persistent notebooks" and the corresponding preferences would be merged "Instant save" and this feature under the "Untitled Document Actions". It would look something like this: ``` Untitled Document Behavior

() Default () Instant Save Directory to save files in [entry ] Current description () Persistent Notebook Directory to save persistent notebook files in [entry ] Save interval [entry ] Default filetype [Combo] ``` and things will get greyed-out depending on the selection.

Now to (2), i.e. behavior on project opens/closes. I've been playing with it a little and I find it kind of stupid that persistent files don't survive project opens: 1. One may need to "transfer" some piece of code from one project to another and opening it separately adds extra complexity. 2. There might be more and more "stale" temp files stored as some projects get deleted and unused and these would become inaccessible from the normal UI.

One option could possibly be a new menu item "Open all persistent notebooks" which would open all temp files stored in the target directory but I think even better if these are opened automatically. (And I know, I was the one who suggested it should just rely on Geany's session handling and now I think I was wrong, sorry.)

Ideally this feature should do nothing if all the persistent files are already stored in the session and preserve the active document stored in the session. Only when some persistent files are missing in the current session, those should be opened.

Implementing would be a bit fiddly as Geany doesn't report when it opened a project or the default session but I think doable.

There could be a variable, let's call it `session_opening` indicating whether a session (default or project) is being opened. It would be set to TRUE in these situations: 1. `plugin_init()` - even though the session isn't loading now, the plugin load should open the notebooks upon load. 2. "project-open" signal handler - project session files will be loaded afterwards 3. "project-close" signal handler - project is being closed, defult session will be loaded afterwards

Then the plugin should connect to the "document-activate" signal. When session is being opened, it's only fired for the tab that becomes active once all the session files are loaded so you can be sure it's called only once. In its handler you can check if `session_opening == TRUE` (and reset it back to FALSE). If TRUE, it means it was session loading and not normal tab switching.

Then you can go through the contents of the temp file directory, check if there are open documents for all the files, and if not, open them.

In any case, before spending any more time on this, better to finally ask someone wise ;-). @b4n What do you think about this feature and the possible ways it could be implemented?

@@ -304,6 +314,274 @@ static void instantsave_document_new_cb(GObject *obj, GeanyDocument *doc, gpoint

}

+static gboolean is_temp_saved_file(const gchar *file_path_utf8)

Since you already pass the whole path in most cases (I think there's just one case where the directory will have to be added), this function could also check whether the directory corresponds to the configured target directory. Better to be on the save side since closing temp saved files may lead to their deletion.

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

Possibly to be updated later when we agree on some "correct" naming of this feature. But I'd not refer to file here - could be something like "Notebook is not saved".

• {
  

+ /* we have to store old filename inside document data to be able to somehow + pass it to document-save callback that is called directly after this one */ + plugin_set_document_data_full(geany_plugin, doc, "file-name-before-save-as", + g_strdup(old_file_path_utf8), g_free); + } + } +} + + +static void persistent_temp_files_document_save_cb(GObject *obj, GeanyDocument *doc, gpointer user_data) +{ + gchar *new_file_path_utf8, *old_file_path_utf8; + + new_file_path_utf8 = DOC_FILENAME(doc); + old_file_path_utf8 = plugin_get_document_data(geany_plugin, doc, "file-name-before-save-as");

Call `plugin_set_document_data(..., "file-name-before-save-as", NULL)` so the previous `old_file_path_utf8` isn't attached to the document (could cause problems when ordinary "save" would think it's "save as").

+ +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); + msg2 = _("Do you want to save it before closing?"); + + response = run_unsaved_dialog_for_persistent_temp_files_tab_closing(msg, msg2); + g_free(msg); + + locale_file_path = g_strdup(doc->real_path);

The `g_strdup()` and maybe even the whole `locale_file_path` could be removed here and `doc->real_path` could be used instead everywhere.

@LiquidCake commented on this pull request.

@@ -304,6 +314,274 @@ static void instantsave_document_new_cb(GObject *obj, GeanyDocument *doc, gpoint

}

+static gboolean is_temp_saved_file(const gchar *file_path_utf8)

added dir check