@techee requested changes on this pull request.
See some more review comments.
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.
What I meant by this is that the whole "persistent temp files" tab in config could go away and the "Instant save" tab would be reused for this feature too. Te rationale about this is:
It reduces confusion because these features are mutually exclusive and enabling one disables the other and users don't see clearly why this happens. I'm slightly worried that users won't get the "persistent temp files" naming and what this feature is good for (as usual, nobody will read the documentation). So what I would suggest is to have a checkbox with something like "Scribble mode" inside the "Instant save" tab enabling this feature. Users probably already know the Scribble tab in Geany's message window and kind of know what to expect. Also, the generated filenames could start with scribble_ making it consistent with existing Geany features. If it's implemented this way, the code below would just go away, only the save interval would have to be added into "Instant save" (grayed out when "Scribble mode" not enabled) and the text explaining the persistence of the directory where the temp files are stored should be changed a little.
Now before implementing anything, I'd suggest waiting for others what they think about it - this would make sense to me but others may have a different opinion.
- }
+ + static glong temp_file_name_prefix_len = strlen(PERSISTENT_TEMP_FILE_NAME_PREFIX); + + max_temp_file_number = 0; + + foreach_dir(filename, dir) + { + utf8_filename = utils_get_utf8_from_locale(filename); + + if (is_temp_saved_file(utf8_filename)) + { + temp_file_number_str = g_utf8_substring(utf8_filename, temp_file_name_prefix_len, -1); + temp_file_number = atoi(temp_file_number_str); + + if (temp_file_number > max_temp_file_number)
`max_temp_file_number = MAX(max_temp_file_number, temp_file_number)` would be a little simpler.
temp_file_number = atoi(temp_file_number_str);
+ + if (temp_file_number > max_temp_file_number) + { + max_temp_file_number = temp_file_number; + } + + g_free(temp_file_number_str); + } + + g_free(utf8_filename); + } + + g_dir_close(dir); + + return g_strdup_printf(_("%s%d"), PERSISTENT_TEMP_FILE_NAME_PREFIX, max_temp_file_number + 1);
I don't think the returned file name should be translatable (i.e. remove `_()`).
+static gchar* create_new_temp_file_name(void)
+{ + GDir *dir; + GError *error = NULL; + gchar *utf8_filename, *temp_file_number_str; + gint temp_file_number, max_temp_file_number; + const gchar *filename; + + dir = g_dir_open(persistent_temp_files_target_dir, 0, &error); + if (dir == NULL) + { + dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("Persistent temp files directory not found")); + return NULL; + } + + static glong temp_file_name_prefix_len = strlen(PERSISTENT_TEMP_FILE_NAME_PREFIX);
The Geany code style is to place declarations at the beginning of the block - i.e. `static glong temp_file_name_prefix_len;` would go to the beginning of the function and you'd just assign the variable here.
Similarly, some of the declarations at the beginning of the function could go at the block beginnings of `for` and `if` blocks below so they are closer to their actual usage. Possibly also in the rest of the code.
+{
+ /* if total text length is 1 while line count is 2 - then the only character of whole text is a linebreak, + which is how completely empty document saved by Geany to disk looks like */ + return sci_get_length(doc->editor->sci) == 0 + || (sci_get_length(doc->editor->sci) == 1 && sci_get_line_count(doc->editor->sci) == 2); +} + + +/* Open document in idle - useful when trying to re-open document from 'document-close' callback + * + * @param pointer to document path (locale-encoded) + * + * @return always FALSE = Just a one shot execution + * + */ +static gboolean open_document_once_idle(gpointer p_locale_file_path)
It's a bit unfortunate this is necessary - maybe Geany could add some signal allowing plugins to disallow document closing.
/* we have to store old/new filename pair 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, new_file_path, g_strdup(old_file_path), g_free); + } + + g_free(old_file_name); + } +} + + +static void persistent_temp_files_document_save_cb(GObject *obj, GeanyDocument *doc, gpointer user_data) +{ + gchar *new_file_path, *old_file_path; + + new_file_path = DOC_FILENAME(doc); + old_file_path = plugin_get_document_data(geany_plugin, doc, new_file_path);
Instead of indexing it by `new_file_path` you could use just some static string like `"old_path"` - the extra data is attached to a document so there's no risk you'll get some other's document path.
GtkWidget *page_label = NULL;
+ + get_current_tab_label(&page_label); + + old_file_path = gtk_widget_get_tooltip_text(page_label); + new_file_path = DOC_FILENAME(doc); + + if (old_file_path == NULL) + { + ui_set_statusbar(TRUE, _("plugin error: failed to delete initial temp file " + "('failed to get notebook tab label text')")); + + return; + } + + old_file_name = g_path_get_basename(old_file_path);
I' just move `g_path_get_basename()` inside `is_temp_saved_file()` so it could be dropped here (and also inside ``is_temp_saved_file_doc()``)
- const gchar *filename;
+ + dir = g_dir_open(persistent_temp_files_target_dir, 0, &error); + if (dir == NULL) + { + dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("Persistent temp files directory not found")); + return NULL; + } + + static glong temp_file_name_prefix_len = strlen(PERSISTENT_TEMP_FILE_NAME_PREFIX); + + max_temp_file_number = 0; + + foreach_dir(filename, dir) + { + utf8_filename = utils_get_utf8_from_locale(filename);
The conversion is probably an overkill here - the file name will just always be ASCII so `locale == UTF8`.
+ g_free(old_file_name); + } +} + + +static void persistent_temp_files_document_save_cb(GObject *obj, GeanyDocument *doc, gpointer user_data) +{ + gchar *new_file_path, *old_file_path; + + new_file_path = DOC_FILENAME(doc); + old_file_path = plugin_get_document_data(geany_plugin, doc, new_file_path); + + if (old_file_path != NULL) + { + gchar *old_file_name = g_path_get_basename(old_file_path);
Also wouldn't be necessary if moved inside `is_temp_saved_file()`.
@@ -462,6 +462,18 @@ gboolean main_is_realized(void)
}
+/** + * Checks whether Geany is 'closing all' tabs right now. + * + * @return @c TRUE if the Geany is 'closing all' tabs right now or @c FALSE otherwise. + **/ +GEANY_API_SYMBOL +gboolean geany_is_closing_all_tabs(void)
I'd probably call it `geany_is_closing_all_documents()`.
}
+ 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)
What I had in mind was using `document-before-save-as` together with `document-save`. In `before-save-as` you'd get the old name, store it to `doc` using `plugin_set_document_data()` and inside the `save-as` handler you'd retrieve it using `plugin_get_document_data()`.
Basically I wanted to get rid of the slightly hacky `get_current_tab_label()` as it relies on the fact that the tooltip contains the path which is not guaranteed to stay this way.