@techee requested changes on this pull request.
@LiquidCake Thanks, looks very good in general!
I didn't go through the config-dialog-related code yet so the review comments
aren't complete.
I think there's one main theme of the review comments - function names and
variable names got brutally long with the `persistent_untitled_document_file`
prefix. I'd suggest the following - dropping `untitled`, `file` and shortening
`document` to `doc` only so the above becomes `persistent_doc` - I think this
is sufficient and actually improves readability of the code. It would be best
if this naming could be used consistently everywhere.
Or if you have some other suggestion, please make it.
(Maybe I said something slightly different in some of the review comments, my
opinion kind of evolved while I was reviewing the code.)
In any case, functionality-wise it seems to work very well. I was also curious
about the situation where someone has "instant save" enabled from previous
Geany versions and upgrades to new Geany version - this settings is still
preserved with your code so there should be seamless transition for existing
users.
> }
pref_widgets;
-#define PERSISTENT_TEMP_FILE_NAME_PREFIX "gtemp_"
+#define PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX "unttld_"
I would suggest `untitled_` here.
> @@ -285,19 +296,26 @@ static void backupcopy_document_save_cb(GObject *obj,
> GeanyDocument *doc, gpoint
}
+static GeanyFiletype *get_doc_filetype_or_default(GeanyDocument *doc) {
I'd drop the `_or_default` from the function name.
> {
dialogs_show_msgbox(GTK_MESSAGE_ERROR,
- _("Persistent temp files directory does not
exist or is not writable."));
+ _("Persistent untitled documents files
directory does not exist or is not writable."));
I'd drop "files" from the message.
> if (dir == NULL)
{
- dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("Persistent temp files
directory not found"));
+ dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("Persistent untitled
documents files directory not found"));
Drop "files", documents->document.
> @@ -46,51 +47,93 @@ PLUGIN_SET_INFO(_("Save Actions"), _("This plugin
> provides different actions rel
enum
{
NOTEBOOK_PAGE_AUTOSAVE = 0,
- NOTEBOOK_PAGE_INSTANTSAVE,
- NOTEBOOK_PAGE_BACKUPCOPY
+ NOTEBOOK_PAGE_BACKUPCOPY,
+ NOTEBOOK_PAGE_UNTITLEDDOCUMENTSAVE
In the code, when a variable, function name, etc. contains "document", I'd
suggest using just "doc" instead - they are getting a bit too long.
(in user-facing strings, keep using "document")
>
/* force saving the file to enable all the related actions(tab
name, filetype, etc.) */
document_save_file(doc, TRUE);
- }
+ }
+}
+
+
+static gboolean is_persistent_untitled_doc_file_name(const gchar *filename)
+{
+ if (filename == NULL)
+ return FALSE;
+
+ return g_str_has_prefix(filename,
PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX);
+}
+
+
+static gboolean is_persistent_untitled_doc_file(const gchar *file_path_utf8)
I'd suggest calling it `is_persistent_untitled_doc_file_path`.
> @@ -258,19 +296,26 @@ static void backupcopy_document_save_cb(GObject *obj,
> GeanyDocument *doc, gpoint
}
+static GeanyFiletype *get_doc_filetype_or_default(GeanyDocument *doc) {
+ GeanyFiletype *ft = doc->file_type;
+
+ if (ft == NULL || ft->id == GEANY_FILETYPES_NONE)
+ /* ft is NULL when a new file without template was opened, so
use the
+ * configured default file type */
+ ft =
filetypes_lookup_by_name(untitled_document_save_default_ft);
+
Do Document->Strip trailing spaces - there are a few of them around.
> + g_free(filename);
+
+ return matched;
+}
+
+
+static gchar* create_new_persistent_untitled_doc_file_name(GeanyDocument *doc)
+{
+ gint i;
+ gchar *extension_postfix;
+ GeanyFiletype *filetype = get_doc_filetype_or_default(doc);
+
+ if (filetype != NULL && !EMPTY(filetype->extension))
+ extension_postfix = g_strconcat(".", filetype->extension, NULL);
+ else
+ extension_postfix = "";
I'd rather suggest `g_strdup("")` and using `g_free()` below in all cases.
> + gchar *extension_postfix;
+ GeanyFiletype *filetype = get_doc_filetype_or_default(doc);
+
+ if (filetype != NULL && !EMPTY(filetype->extension))
+ extension_postfix = g_strconcat(".", filetype->extension, NULL);
+ else
+ extension_postfix = "";
+
+ for (i = 1; i < 1000; i++)
+ {
+ gchar *next_file_name, *next_file_path;
+
+ next_file_name = g_strdup_printf("%s%d%s",
PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX, i, extension_postfix);
+ next_file_path = g_strdup_printf("%s%c%s",
persistent_untitled_docs_target_dir, G_DIR_SEPARATOR, next_file_name);
+
+ gboolean file_exists = g_file_test(next_file_path,
G_FILE_TEST_EXISTS);
`gboolean file_exists` to the beginning of the block.
> + extension_postfix = "";
+
+ for (i = 1; i < 1000; i++)
+ {
+ gchar *next_file_name, *next_file_path;
+
+ next_file_name = g_strdup_printf("%s%d%s",
PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX, i, extension_postfix);
+ next_file_path = g_strdup_printf("%s%c%s",
persistent_untitled_docs_target_dir, G_DIR_SEPARATOR, next_file_name);
+
+ gboolean file_exists = g_file_test(next_file_path,
G_FILE_TEST_EXISTS);
+
+ g_free(next_file_path);
+
+ if (file_exists) {
+ g_free(next_file_name);
+ } else {
both for `if` and `else` `{` on separate line
> +
+ new_file_path_utf8 = DOC_FILENAME(doc);
+ old_file_path_utf8 = plugin_get_document_data(geany_plugin, doc,
"file-name-before-save-as");
+
+ if (old_file_path_utf8 != NULL)
+ {
+ if (is_persistent_untitled_doc_file(old_file_path_utf8)
+ && ! g_str_equal(old_file_path_utf8,
new_file_path_utf8))
+ {
+ /* remove untitled doc file if it was saved as some
other file */
+ gchar *locale_old_file_path =
utils_get_locale_from_utf8(old_file_path_utf8);
+ g_remove(locale_old_file_path);
+
+ g_free(locale_old_file_path);
+
+ ui_set_statusbar(TRUE, _("Untitled document file %s was
deleted"), old_file_path_utf8);
I'd say use `msgwin_status_add` for this - the file wasn't permanently lost in
this case so the message in the status bar could be confusing for users.
> + if (! geany_is_closing_all_documents() && doc->real_path != NULL &&
> is_persistent_untitled_doc_file(doc->file_name))
+ {
+ gchar *short_filename = document_get_basename_for_display(doc,
-1);
+
+ if (! document_is_empty(doc))
+ {
+
show_unsaved_dialog_for_persistent_untitled_docs_tab_closing(
+ doc,
+ short_filename
+ );
+ }
+ else
+ {
+ g_remove(doc->real_path);
+
+ ui_set_statusbar(TRUE, _("Empty untitled document file
%s was deleted"), short_filename);
Use `msgwin_status_add` here.
> @@ -512,7 +551,7 @@ static void
> show_unsaved_dialog_for_persistent_temp_files_tab_closing(
case GTK_RESPONSE_NO:
g_remove(doc->real_path);
- ui_set_statusbar(TRUE, _("Temp file %s was deleted"),
short_filename);
+ ui_set_statusbar(TRUE, _("Untitled document file %s was
deleted"), short_filename);
This is the real destructive action so using `ui_set_statusbar()` is alright
here.
> };
+enum
+{
+ NOTEBOOK_UNTITLEDDOCUMENTSAVE_RADIO_DISABLED = 0,
I'd suggest `NOTEBOOK_UNTITLED_DOC_RADIO_DISABLED` and similarly below.
> GtkWidget *backupcopy_entry_dir;
GtkWidget *backupcopy_entry_time;
GtkWidget *backupcopy_spin_dir_levels;
+
+ GtkWidget *untitled_document_save_disabled_radio;
+ GtkWidget *untitled_document_save_instantsave_radio;
+ GtkWidget *untitled_document_save_persistent_radio;
+
+ GtkWidget *untitled_document_save_ft_combo;
I'd drop `_save` from the 4 variables so it gets a little shorter.
> + return next_file_name;
+ }
+ }
+
+ if (strlen(extension_postfix))
+ g_free(extension_postfix);
+
+ return NULL;
+}
+
+
+static void persistent_untitled_document_new_cb(GObject *obj, GeanyDocument
*doc, gpointer user_data)
+{
+ if (doc->file_name == NULL)
+ {
+ gchar *pers_docs_files_dir_utf8, *new_pers_doc_file_name_utf8,
*new_pers_doc_file_path_utf8;
these are local variables and already in "persistent untitled" function so
there's no need for "pres_doc_files" and similar prefix - it makes the code
hard to read.
Similarly in all other cases - for global variables/function names it's
probably alright to be clear to what they apply but there's no need for it for
local variables.
> + * @return always FALSE = Just a one shot execution
+ *
+ */
+static gboolean open_document_once_idle(gpointer p_locale_file_path)
+{
+ gchar *locale_file_path = (gchar *) p_locale_file_path;
+
+ document_open_file(locale_file_path, FALSE, NULL, NULL);
+
+ g_free(locale_file_path);
+
+ return FALSE;
+}
+
+
+static gint run_unsaved_dialog_for_persistent_untitled_docs_tab_closing(const
gchar *msg, const gchar *msg2)
Something shorter please, this isn't Java :-)
> @@ -462,6 +462,18 @@ gboolean main_is_realized(void)
}
+/**
+ * Checks whether Geany is 'closing all' documents right now.
+ *
+ * @return @c TRUE if the Geany is 'closing all' documents right now or @c
FALSE otherwise.
+ **/
+GEANY_API_SYMBOL
+gboolean geany_is_closing_all_documents(void)
Bump API for this change.
> @@ -475,7 +514,7 @@ static gint
> run_unsaved_dialog_for_persistent_temp_files_tab_closing(const gchar
}
-static void show_unsaved_dialog_for_persistent_temp_files_tab_closing(
+static void show_unsaved_dialog_for_persistent_untitled_docs_tab_closing(
Also, make it shorter a bit - it doesn't have to say all the details and such
functions are hard to read in the code.
> @@ -614,13 +653,21 @@ static void load_all_temp_files_into_editor(void)
}
-static gboolean load_all_temp_files_idle(gpointer data)
+static gboolean load_all_persistent_untitled_doc_files_idle(gpointer data)
-> `load_persistent_docs_idle` ?
> @@ -614,13 +653,21 @@ static void load_all_temp_files_into_editor(void)
}
-static gboolean load_all_temp_files_idle(gpointer data)
+static gboolean load_all_persistent_untitled_doc_files_idle(gpointer data)
+{
+ load_all_persistent_untitled_doc_files_into_editor();
+
+ return FALSE;
+}
+
+
+static gboolean
load_all_persistent_untitled_doc_files_and_reopen_current_idle(gpointer data)
-> `reload_persistent_docs_on_session_change_idle`?
> {
/* switch functionality off, so invalid target directory cannot
be actually used */
- enable_persistent_temp_files = FALSE;
- g_key_file_set_boolean(config, "saveactions",
"enable_persistent_temp_files", FALSE);
-
gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(pref_widgets.checkbox_enable_persistent_temp_files),
FALSE);
+ enable_persistent_untitled_docs = FALSE;
+ g_key_file_set_boolean(config, "saveactions",
"enable_persistent_untitled_documents", FALSE);
+
+
gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(pref_widgets.untitled_document_save_disabled_radio),
TRUE);
Double empty line within function - make it just single empty line (I think
I've seen it at other places too).
> + 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);
Alright, we have the "correct naming" now so I'd say "Untitle document %s is
not saved" (I haven't checked how other dialogs look like - should the final
`.` be there? Possibly yes, I just don't know.)
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/3911#pullrequestreview-2430731213
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany/pull/3911/review/[email protected]>
