@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3911#pullrequestreview-2134281997
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany/pull/3911/review/[email protected]>
