Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
@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().
In src/libmain.c:
> @@ -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, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.