Extension of saveactions plugin that allows Geany to store/load data for new, unnamed tabs when Geany window is closed/reopened. This is a convenience feature inspired by Notepad++ and discussed here https://github.com/geany/geany/issues/905 Includes one small extension to API (new function in main.h) - if i understood correctly, this does not require incrementing any versions
https://github.com/geany/geany/assets/9273621/8bdd67a1-97a4-4cf3-b224-81f194...
You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3911
-- Commit Summary --
* persistent-temp-files-plugin: added feature to saveactions plugin
-- File Changes --
M plugins/saveactions.c (605) M src/libmain.c (12) M src/main.h (2)
-- Patch Links --
https://github.com/geany/geany/pull/3911.patch https://github.com/geany/geany/pull/3911.diff
@LiquidCake commented on this pull request.
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(
we have to use this hack with hashtable to remember name of original 'temp' file, during performing 'save as' which assigns a new 'final' name to file.
Ideally we would like to do all these actions in a more proper manner but this would require core code modifications
@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.
+ 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)
Another note - automatic Geany session management will only work when Preferences->General->Startup->Load files from the last session is used. But I think it's kind of expected that Geany wouldn't auto-load persistent temp files in when this option is 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)
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.
@elextr 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.
@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.
thanks for your comments, will look at it
@LiquidCake pushed 1 commit.
a8c882687ff5fc633ba055c165dc7ae94c9daf9a persistent-temp-files-plugin: code review - fixes
@LiquidCake pushed 1 commit.
7ef14a845f76aabcdf8917d6d419e41e4ac0be82 persistent-temp-files-plugin: code review - fixes
@LiquidCake pushed 1 commit.
6616f7e1443a11d820666006be881c8616328b69 persistent-temp-files-plugin: code review - fixes
@LiquidCake pushed 1 commit.
064011ad2e00ecb18e2b5177b2149c8908098027 persistent-temp-files-plugin: code review - fixes
@LiquidCake commented on this pull request.
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(
done
@LiquidCake commented on this pull request.
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);
this method was re-used from Instantsave, which seems to need arbitrary file type setting. Anyway im not re-using this code anymore - switched to `document_save_file_as` as you suggested. So Instantsave's code is left intact
@LiquidCake commented on this pull request.
+ 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)
removed, see comment about `is_quitting vs closing_all`
@LiquidCake commented on this pull request.
}
+ 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)
using before-save-as wont work because at the moment that event is fired we dont yet have "new" document name defined, and we need both "old" and "new"
@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.
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)
done
@LiquidCake commented on this pull request.
- }
+} + + +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);
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)
initial idea was to force all temp files to be open in Geany all the time, or at least open all of them on startup. Even ones created by concurrent Geany instances or manually on disk. Thats why we had that function "load all temp files" and why we used "is quitting" instead of "is closing all" - to force user saving or deleting temp files after clicking "close all" and avoid dialogs only when closing window.
Now after few changes (temp files are now properly numbered by integers and are properly readable in "recent files" menu) i guess we got a better solution and this idea of keeping all existing temp files open is probably not actual. So i did as was suggested - switched to "closing all" status and started completely relying on session management mechanism to re-open those temp files which it finds appropriate. This seems to work fine, including general session vs project sessions.
@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 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.
@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 pushed 1 commit.
583afefbbbe8d1135085cac8c05ac539406e9393 persistent-temp-files-plugin: code review 2 - fixes
@LiquidCake commented on this pull request.
}
+ 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)
right, missed that, done
@LiquidCake commented on this pull request.
- }
+ + 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)
done
@LiquidCake commented on this pull request.
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);
done
@LiquidCake commented on this pull request.
+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);
fixed
@LiquidCake commented on this pull request.
/* 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);
done
@LiquidCake commented on this pull request.
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);
done
@LiquidCake commented on this pull request.
- 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);
removed
@LiquidCake commented on this pull request.
- 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);
removed
@LiquidCake commented on this pull request.
+ 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);
done
@LiquidCake commented on this pull request.
@@ -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)
renamed
@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
So what I would suggest is to have a checkbox with something like "Scribble mode" inside the "Instant save" tab enabling this feature
"Does it save the Geany Scribble window then?" [user who didn't read the docs question]
Whilst I didn't get the "persistent temp files" either and I still don't know what the use-case is beyond persisting unnamed buffers and I don't understand what the reason for saving unnamed buffers, if its unnamed how do I know whats in it to switch to it when they are all named "Untitled"? So I can't suggest anything better than the descriptive "save unnamed buffers".
Whilst I didn't get the "persistent temp files" either and I still don't know what the use-case is beyond persisting unnamed buffers and I don't understand what the reason for saving unnamed buffers is, if its unnamed how do I know whats in it to switch to it when they are all named "Untitled"?
They will get some file name (see the screencast, I personally would suggest names `scribble_1`, `scribble_2`, etc.)
So I can't suggest anything better than the descriptive "save unnamed buffers".
That's not the whole thing the patch does though - it really is some "scribble mode" with additional features: 1. On opening an unnamed buffer, it saves the file (same as instant save) 2. It auto-saves this buffer in user-specified intervals so the user doesn't have to care about saving it 3. When you close the buffer, you are asked if you really want to close it - when you do, the underlying file is deleted so, again, you don't have to care about files here. When you select that you don't want to close it, the open buffer is preserved - with a slight limitation. The limitation is that plugins can't interrupt file closing so what the plugin does it that it reopens the underlying file after it's closed (and of course it doesn't delete it in this case) - since this only happens when the user makes a mistake and tries to close the file by accident, I think it's fine to have it implemented this way. 4. When you use "Save as", the underlying "scribble" file is deleted automatically and only the file under which you saved it is preserved.
Now to why people (https://github.com/geany/geany/issues/905), including me, want to have this feature - the Scribble panel is extremely limited and the basic editor features don't work there (see https://github.com/geany/geany/issues/3558). Having a full-blown Scintilla editor used for Scribble with all editing options and syntax highlighting in a big window would be really nice. And this PR is quite a nice way how to implement it in a pretty much invisible and care-free way for users.
I know the scribble window is very limited, thats why I'm confused by you wanting to call this "scribble".
I know the scribble window is very limited, thats why I'm confused by you wanting to call this "scribble".
Well, it doesn't really have to be called this way, I'd just call it based on what it's supposed to do instead of how it's implemented (i.e. "persistent temp files"). Could be for instance "persistent notepad" and the individual tabs would be `notepad_1`, `notepad_2`, etc.
Ok, at least "notepad" is a term thats not used so far AFAICT so it won't get confused with something else.
@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 pushed 1 commit.
c5eb438483f58875e97f24428ab2c8ab4c1fb60b persistent-temp-files-plugin: code review 3 - fixes
@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
@LiquidCake commented on this pull request.
{
+ /* 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");
should not cause issues but you're right, added cleanup
@LiquidCake commented on this pull request.
+ +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);
strdup is needed because path is changed after save actions is performed below. But i lowered scope of that variable and used real_path where possible
@LiquidCake pushed 1 commit.
16b252d8b4244391470f5380707ce5515bb74ebd persistent-temp-files-plugin: code review 3 - fixes
@techee commented on this pull request.
+ +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);
You're right, I missed that.
One more thing. The plugin should pre-fill `default_persistent_temp_files_dir_utf8` into the directory entry in the plugin configuration. When you delete the saveactions configuration file, this entry is empty by default.
Also, when this empty directory gets confirmed, the plugin shows a dialog with error, but this invalid value is stored into the config file anyway (I know this is how the Instantsave feature has it implemented now and you just copied it from there, but it shouldn't be possible to confirm the save action preferences with the invalid value and the preferences dialog shouldn't be closed in this case).
Finally, when you confirm the empty directory entry, the plugin crashes in
``` #1 0x0000ffffec113d54 in is_temp_saved_file (file_path_utf8=0xaaaaab716d80 "/home/parallels/projects/geany/doc/plugins.dox") at saveactions.c:336 #2 0x0000ffffec113eb4 in persistent_temp_files_update (data=<optimized out>) at saveactions.c:733 ```
This should be fixed too.
@LiquidCake pushed 1 commit.
54e7aa218af8d3d311fffa5bfb07d06c85f050d3 persistent-temp-files-plugin: code review 3 - fixes
One more thing. The plugin should pre-fill `default_persistent_temp_files_dir_utf8` into the directory entry in the plugin configuration. When you delete the saveactions configuration file, this entry is empty by default.
Also, when this empty directory gets confirmed, the plugin shows a dialog with error, but this invalid value is stored into the config file anyway (I know this is how the Instantsave feature has it implemented now and you just copied it from there, but it shouldn't be possible to confirm the save action preferences with the invalid value and the preferences dialog shouldn't be closed in this case).
Finally, when you confirm the empty directory entry, the plugin crashes in
#1 0x0000ffffec113d54 in is_temp_saved_file (file_path_utf8=0xaaaaab716d80 "/home/parallels/projects/geany/doc/plugins.dox") at saveactions.c:336 #2 0x0000ffffec113eb4 in persistent_temp_files_update (data=<optimized out>) at saveactions.c:733This should be fixed too.
not sure how any of these is possible - target_dir value in config file may stay empty while in-memory variable is filled with default value anyway. And saving empty path into config should not be possible since we have multiple checks which allow only non-empty absolute path to valid existing directory to be saved to config file. I wasnt able to reproduce any of these issues but anyway i added pre-filling of target_dir into a config file on plugin init, hope this helps.
ah found it, the issue appears not when you delete config file but when you dont have directory itself existing on disc, will try to do something with it
@LiquidCake pushed 1 commit.
b451a0a18f2ccb63258c1cb664753b650f4306c3 persistent-temp-files-plugin: code review 4 - fixes - reworked target dir path setting
so i reworked settings behavior for this feature, now everything should be fine: - when geany starts without existing temp files dir setting or settings file - it populates default dir path setting and creates folder on disk - when geany starts with existing dir setting that is invalid (e.g. folder was deleted manually) - feature gets auto-disabled and status message sent. - when user tries to save feature settings - dialog wont close until valid dir is set (or 'cancel' clicked). - enabling feature with invalid dir path in a textbox is also not possible anymore.
also, i fulfilled old TODO of instantsave code i reused and added one of utils function to plugins API (checking if file is writable)
Overall, now everything seems working fine to me. As it is expected - doing any changes in C may be hilariously tedious, and current code will anyway have to be re-wired to new UI (if we want it to change), so i guess we need to confirm an approach and then (if this happens) - try to finish everything. e.g. this looks good to me https://github.com/geany/geany/pull/3911#pullrequestreview-2136554129
@LiquidCake pushed 1 commit.
be79838a3149900b666f3b800b37b6cb6c567345 persistent-temp-files-plugin: code review 5 - fixes - added re-opening of all temp files
added re-opening for all temp files as described here - https://github.com/geany/geany/pull/3911#pullrequestreview-2136554129 Implementation appeared to be surprisingly tricky and there are some slightly rough edges, but at the end looks good to me.
@LiquidCake pushed 1 commit.
8cfd39c94ea6f4b577e928f970a9a959fe54d1c4 persistent-temp-files-plugin: code review 5 - fixes - added re-opening of all temp files
@LiquidCake Thanks for the updates! I definitely like that the temp files are preserved across project opens/closes and it seems to work very well for me.
Some more observations when playing with the feature: - when closing a temp file and choosing "Save", I'd suggest using some other default directory than the one where temp files are stored so users aren't tempted to store the files there (and be surprised by the consequences if they choose a `gtemp_x` file name) - since all temp files are opened on session load, there could be one more option in the dialog asking what to do when closing a temp file - "Close". This would just close the temp file but keep it saved on the disk. I can imagine it could be useful when someone is not interested to see the temp file for some project but still wants to keep it for other projects. I'd also maybe rename "Don't save" to "Delete" so it's clear the contents of the tab is lost. - it would be worth considering whether `create_new_temp_file_name()` shouldn't rather iterate over integers and test whether `gtemp_i` exists - this way, when e.g. `gtemp_5` is open and all other temp files closed, `gtemp_1-4` file names could be reused again for new tabs - I'm not completely sure if `utils_is_file_writable()` should be part of Geany's API. Personally I'd rather sacrifice this check and not bloat Geany's API with another random function.
@LiquidCake pushed 2 commits.
1fd9ab7c3ff9e5ace5c7407e566d5ff52021bb8d persistent-temp-files-plugin: code review 6 - reverted plugins api changes, fixed losing of active tab 10a0dc03353523b1906035b0c5af91f562cfb95d persistent-temp-files-plugin: code review 7 - allowed recycling of temp file numbers
Added workaround to prevent temp files hijacking active tab on session change.
And about these:
@LiquidCake Thanks for the updates! I definitely like that the temp files are preserved across project opens/closes and it seems to work very well for me.
Some more observations when playing with the feature:
* when closing a temp file and choosing "Save", I'd suggest using some other default directory than the one where temp files are stored so users aren't tempted to store the files there (and be surprised by the consequences if they choose a `gtemp_x` file name)
I would also like this but we wont be able to do it on plugin level, directory is defined and set to save-as window in core code and also very early. Would work if we could check some specific property of document at this point, see that it is temporary and prevent using existing file path as default value for save-as window. But i guess we wont be able to do it.
* since all temp files are opened on session load, there could be one more option in the dialog asking what to do when closing a temp file - "Close". This would just close the temp file but keep it saved on the disk. I can imagine it could be useful when someone is not interested to see the temp file for some project but still wants to keep it for other projects. I'd also maybe rename "Don't save" to "Delete" so it's clear the contents of the tab is lost.
To me such UX seems to allow user too many options to chose from, and also whole idea of allowing user to "keep" temp file on disk while not having it open in editor seems contradicting the essence of feature - user should see 'temp' tab as if it was an 'untitled' tab that just doesnt disappear after we close program. If user wants to save file and close tab - they should explicitly do it with save-as. Also, if user could just close temp tab while keeping file - it will anyway re-appear in editor after any session change. This is also why i just copied tab-closing dialog for temp tabs from regular unsaved tab closing dialog, without any changes - it should look and feel the same (ideally). Unfortunately, there is one case we allow user to close temp tabs without deleting/saving them - it is when 'close all' option is clicked. But this is a necessary evil, not a feature - we cannot differentiate 'close all' from changing session to/from project. So essentially - what you are proposing can be done, but i personally would avoid it.
* it would be worth considering whether `create_new_temp_file_name()` shouldn't rather iterate over integers and test whether `gtemp_i` exists - this way, when e.g. `gtemp_5` is open and all other temp files closed, `gtemp_1-4` file names could be reused again for new tabs
done
* I'm not completely sure if `utils_is_file_writable()` should be part of Geany's API. Personally I'd rather sacrifice this check and not bloat Geany's API with another random function.
removed
Added workaround to prevent temp files hijacking active tab on session change.
What is this and where is the commit?
when closing a temp file and choosing "Save", I'd suggest using some other default directory than the one where temp files are stored so users aren't tempted to store the files there (and be surprised by the consequences if they choose a `gtemp_x` file name)
I would also like this but we wont be able to do it on plugin level, directory is defined and set to save-as window in core code and also very early.
You could just avoid using `dialogs_show_save_as()` and implement it manually similarly to e.g. `target_directory_button_clicked_cb()` but for saving files in this case (note the changes related to #3861 - if you copy the code of `target_directory_button_clicked_cb()` and modify it, you can reuse `file_chooser_run()` and `file_chooser_destroy()`).
- since all temp files are opened on session load, there could be one more option in the dialog asking what to do when closing a temp file - "Close". This would just close the temp file but keep it saved on the disk. I can imagine it could be useful when someone is not interested to see the temp file for some project but still wants to keep it for other projects. I'd also maybe rename "Don't save" to "Delete" so it's clear the contents of the tab is lost.
To me such UX seems to allow user too many options ...
Fair enough, makes sense.
- it would be worth considering whether `create_new_temp_file_name()` shouldn't rather iterate over integers and test whether `gtemp_i` exists - this way, when e.g. `gtemp_5` is open and all other temp files closed, `gtemp_1-4` file names could be reused again for new tabs
Instead of using GDir, storing the result into a hash table, and then iterating over integers, wouldn't it be easier to do something like ```C gint i;
for (i = 1; i < 1000; i++) { gchar *name = g_strdup_printf("%s%c%s%d", persistent_temp_files_target_dir, G_DIR_SEPARATOR, PERSISTENT_TEMP_FILE_NAME_PREFIX, i); gboolean file_exists = g_file_test(dirpath_locale, G_FILE_TEST_EXISTS); g_free(name;) if (!file_exists) break; }
return g_strdup_printf("%s%d", PERSISTENT_TEMP_FILE_NAME_PREFIX, i); ```
Untested, possibly missing some details.
What is this and where is the commit?
https://github.com/geany/geany/pull/3911/commits/1fd9ab7c3ff9e5ace5c7407e566... 
At any session change, including initial startup - we first load session and then call `load_all_temp_files_idle()` which (re)loads all temp files and changes active tab to last loaded temp file. I added a workaround to re-activate originally active tab by re-opening it (i guess no other way to do it)
You could just avoid using `dialogs_show_save_as()` and implement it manually similarly to e.g. `target_directory_button_clicked_cb()` but for saving files in this case (note the changes related to #3861 - if you copy the code of `target_directory_button_clicked_cb()` and modify it, you can reuse `file_chooser_run()` and `file_chooser_destroy()`).
i could try, but this would anyway be used only when we try to close temp file tab and chose save-as. But when user just tries to save-as temp file using menu - it will still be opening on temp folder
@LiquidCake pushed 1 commit.
b4333c7186bd982fef55e1f89829f3c57b515289 persistent-temp-files-plugin: code review 8 - changed new file name generation logic
Instead of using GDir, storing the result into a hash table, and then iterating over integers, wouldn't it be easier to do something like
lol you're right, changed logic to this one
At any session change, including initial startup - we first load session and then call load_all_temp_files_idle() which (re)loads all temp files and changes active tab to last loaded temp file. I added a workaround to re-activate originally active tab by re-opening it (i guess no other way to do it)
Ah, right, I overlooked it - I thought it only removed the API call.
i could try, but this would anyway be used only when we try to close temp file tab and chose save-as. But when user just tries to save-as temp file using menu - it will still be opening on temp folder
OK, I didn't think of this possibility - I guess it doesn't make sense to spend time on this then.
lol you're right, changed logic to this one
Better ;-)
so i think this feature is complete, if we are considering merging it - we should settle on config UI (either keep as is or change to point #1 from https://github.com/geany/geany/pull/3911#pullrequestreview-2136554129) And probably rename everything to "persistent notepad" or something like that.
otherwise it is usable with manual geany build, helps me a lot
@techee commented on this pull request.
so i think this feature is complete, if we are considering merging it -
Agree, I think it's mostly ready.
I just had a look at the code once more and found some minor issues but nothing big.
we should settle on config UI (either keep as is or change to point https://github.com/geany/geany/pull/1 from https://github.com/geany/geany/pull/3911#pullrequestreview-2136554129)
And probably rename everything to "persistent notepad" or something like that.
I've been thinking about it and maybe "persistent untitled documents" would be best so there's no new terminology. I still think it would be best to merge the configuration with "instant save" but depends on what others (@b4n @eht16) think.
Note that we seem to be in the middle of the "Geany summer vacation" period and even normally it takes quite some time to get things merged (so please be patient) but from me at least this PR has full support - it's very useful and it also fixes the issue with the most thumbs up reactions.
{
+ /* 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");
`old_file_path_utf8` isn't freed anywhere, is it?
}
+ + g_free(locale_file_path); + } + + g_dir_close(dir); + + /* create new empty file/tab if this is a "fresh" session start without any opened files */ + if (gtk_notebook_get_n_pages(GTK_NOTEBOOK(geany->main_widgets->notebook)) == 0) + document_new_file(NULL, NULL, NULL); +} + + +static gboolean load_all_temp_files_idle(gpointer p_cur_doc) +{ + //remember and re-open document from originaly focused tab
Use `/* */` comments to match the rest of the code.
if (doc != NULL && document_is_empty(doc))
+ document_close(doc); + } + + g_free(locale_file_path); + } + + g_dir_close(dir); + + /* create new empty file/tab if this is a "fresh" session start without any opened files */ + if (gtk_notebook_get_n_pages(GTK_NOTEBOOK(geany->main_widgets->notebook)) == 0) + document_new_file(NULL, NULL, NULL); +} + + +static gboolean load_all_temp_files_idle(gpointer p_cur_doc)
The `p_cur_doc` parameter isn't used - rename to something generic like `data` or whatever to avoid confusion.
+{
+ gint i, max = gtk_notebook_get_n_pages(GTK_NOTEBOOK(geany->main_widgets->notebook)); + + for (i = 0; i < max; i++) + { + GeanyDocument *doc = document_get_from_page(i); + + if (doc->real_path != NULL && is_temp_saved_file(doc->file_name)) + { + document_save_file(doc, FALSE); + } + } + + return TRUE; +} +
Add one more empty line.
configdir_utf8 = utils_get_utf8_from_locale(geany->app->configdir);
+ default_persistent_temp_files_dir_utf8 = g_strconcat(configdir_utf8, G_DIR_SEPARATOR_S, "plugins", + G_DIR_SEPARATOR_S, "saveactions", G_DIR_SEPARATOR_S, "persistent_temp_files", NULL); + g_free(configdir_utf8); + + g_key_file_set_string(config, "persistent_temp_files", "target_dir", + default_persistent_temp_files_dir_utf8); + + tmp = utils_get_locale_from_utf8(default_persistent_temp_files_dir_utf8); + g_free(default_persistent_temp_files_dir_utf8); + + utils_mkdir(tmp, TRUE); + g_free(tmp); + } + + tmp = utils_get_setting_string(config, "persistent_temp_files", "target_dir", NULL);
This `tmp` is leaked - use `SETPTR() on the line below.
@LiquidCake pushed 1 commit.
b74ab16413b0f6da44377a4127a7532178ae7568 persistent-temp-files-plugin: code review 9 - fixes
@LiquidCake commented on this pull request.
}
+ + g_free(locale_file_path); + } + + g_dir_close(dir); + + /* create new empty file/tab if this is a "fresh" session start without any opened files */ + if (gtk_notebook_get_n_pages(GTK_NOTEBOOK(geany->main_widgets->notebook)) == 0) + document_new_file(NULL, NULL, NULL); +} + + +static gboolean load_all_temp_files_idle(gpointer p_cur_doc) +{ + //remember and re-open document from originaly focused tab
fixed
@LiquidCake commented on this pull request.
if (doc != NULL && document_is_empty(doc))
+ document_close(doc); + } + + g_free(locale_file_path); + } + + g_dir_close(dir); + + /* create new empty file/tab if this is a "fresh" session start without any opened files */ + if (gtk_notebook_get_n_pages(GTK_NOTEBOOK(geany->main_widgets->notebook)) == 0) + document_new_file(NULL, NULL, NULL); +} + + +static gboolean load_all_temp_files_idle(gpointer p_cur_doc)
fixed
@LiquidCake commented on this pull request.
configdir_utf8 = utils_get_utf8_from_locale(geany->app->configdir);
+ default_persistent_temp_files_dir_utf8 = g_strconcat(configdir_utf8, G_DIR_SEPARATOR_S, "plugins", + G_DIR_SEPARATOR_S, "saveactions", G_DIR_SEPARATOR_S, "persistent_temp_files", NULL); + g_free(configdir_utf8); + + g_key_file_set_string(config, "persistent_temp_files", "target_dir", + default_persistent_temp_files_dir_utf8); + + tmp = utils_get_locale_from_utf8(default_persistent_temp_files_dir_utf8); + g_free(default_persistent_temp_files_dir_utf8); + + utils_mkdir(tmp, TRUE); + g_free(tmp); + } + + tmp = utils_get_setting_string(config, "persistent_temp_files", "target_dir", NULL);
fixed
@LiquidCake commented on this pull request.
+{
+ gint i, max = gtk_notebook_get_n_pages(GTK_NOTEBOOK(geany->main_widgets->notebook)); + + for (i = 0; i < max; i++) + { + GeanyDocument *doc = document_get_from_page(i); + + if (doc->real_path != NULL && is_temp_saved_file(doc->file_name)) + { + document_save_file(doc, FALSE); + } + } + + return TRUE; +} +
fixed
@LiquidCake commented on this pull request.
{
+ /* 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");
it is freed by underlying glib data structure when we clear value: `plugin_set_document_data(geany_plugin, doc, "file-name-before-save-as", NULL); /* clear value */`
@techee commented on this pull request.
{
+ /* 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");
Oh yeah, right, I missed that.
One more problem - when you launch Geany from the command-line by ``` geany some_file.txt ``` where `some_file.txt` doesn't exist yet, the file gets opened in a new tab but the "persistent temp file" becomes the active tab instead. I haven't checked the code what Geany does exactly but the file apparently gets opened at some later point.
In any case, I believe this could be mitigated if inside `load_all_temp_files_into_editor()` you only call `document_open_file()` when the temp file isn't opened in any tab (by going through open documents and checking their path). This would also eliminate some other problems like when "Switch to last used document after closing a tab" is selected in "Notebook tabs" inside preferences which leads to switching to "persistent temp files" when closing some other tabs.
One more request - could you rebase this PR on top of current Geany master (or merge master into this branch)? I'd like to give this feature some more testing but in parallel test the recently merged https://github.com/geany/geany/pull/3849 together with the LSP plugin.
@LiquidCake pushed 1 commit.
fe1357c04f57bb7795ff86857546ca080f3d8909 persistent-temp-files-plugin: code review 10 - persistent files wont be reopened if already loaded
@LiquidCake pushed 1 commit.
fd6fb72cef812960d3eaccc3401c67a4eb579946 Merge branch 'geany:master' into saveactions_persistent_temp_files
One more problem - when you launch Geany from the command-line by
geany some_file.txtwhere `some_file.txt` doesn't exist yet, the file gets opened in a new tab but the "persistent temp file" becomes the active tab instead. I haven't checked the code what Geany does exactly but the file apparently gets opened at some later point.
In any case, I believe this could be mitigated if inside `load_all_temp_files_into_editor()` you only call `document_open_file()` when the temp file isn't opened in any tab (by going through open documents and checking their path). This would also eliminate some other problems like when "Switch to last used document after closing a tab" is selected in "Notebook tabs" inside preferences which leads to switching to "persistent temp files" when closing some other tabs.
One more request - could you rebase this PR on top of current Geany master (or merge master into this branch)? I'd like to give this feature some more testing but in parallel test the recently merged #3849 together with the LSP plugin.
merged master, fixed unconditional reopening of files. This is indeed beneficial, though the core issue with keeping right tab active after opening non-existent file from console is related to fact that we cannot manually activate tab that does not yet have underlying file saved to disk (we can activate it only by re-opening file from disc right now). So now everything works correctly, unless you put persistent temp file on disc manually and then open non-existent file from console - then focus will still be on a last-opened persistent file. Doesnt matter though
@techee requested changes on this pull request.
Looks good and works well (apart from the two nitpicky style comments).
the core issue with keeping right tab active after opening non-existent file from console is related to fact that we cannot manually activate tab that does not yet have underlying file saved to disk
Right, that's the reason.
One more thing though - when playing with it, I didn't quite like the behavior of "close all" documents which also closes the temp files - I think these should stay open as they are not normal files (I typically use "close all" when the number of open files gets out of my control but closing temp files kind of kills this feature because one would have to open them manually and spend time on something that should happen automatically).
I believe this could be achieved by reopening all temp files when the number of open tabs drops to 0. I think one could schedule an idle function inside `persistent_temp_files_document_close_cb()` (in the `geany_is_closing_all_documents()` case) and inside this idle function do something like ```C static gboolean persistent_temp_files_document_close_idle(gpointer data) { if (gtk_notebook_get_n_pages(GTK_NOTEBOOK(geany->main_widgets->notebook)) == 0) load_all_temp_files_into_editor();
} ``` I think at the point this idle function is called, Geany will have all the documents closed.
(One slight problem is that when closing 100 files, the idle function will be scheduled 100 times and when there are no temp files, `load_all_temp_files_into_editor();` will be also called 100 times and try to find some temp files but I think even this will be very quick and not worth taking care of.)
if (is_temp_saved_file_name(filename))
{ - GeanyDocument *doc = document_open_file(locale_file_path, FALSE, NULL, NULL); + gchar *locale_file_path, *file_path_utf8; + + locale_file_path = g_build_path(G_DIR_SEPARATOR_S, persistent_temp_files_target_dir, filename, NULL); + file_path_utf8 = utils_get_utf8_from_locale(locale_file_path); + GeanyDocument *doc = document_find_by_filename(file_path_utf8);
Move `GeanyDocument *doc;` declaration to the beginning of the block.
if (is_temp_saved_file_name(filename))
{ - GeanyDocument *doc = document_open_file(locale_file_path, FALSE, NULL, NULL); + gchar *locale_file_path, *file_path_utf8; + + locale_file_path = g_build_path(G_DIR_SEPARATOR_S, persistent_temp_files_target_dir, filename, NULL); + file_path_utf8 = utils_get_utf8_from_locale(locale_file_path); + GeanyDocument *doc = document_find_by_filename(file_path_utf8); + + g_free(file_path_utf8); + + if (doc == NULL) {
Drop `{}` (or move `{` to the next line).
@LiquidCake pushed 1 commit.
cd2af4211944b6db25a04c61a48262df7a200190 persistent-temp-files-plugin: code review 11 - close-all fix
@LiquidCake commented on this pull request.
if (is_temp_saved_file_name(filename))
{ - GeanyDocument *doc = document_open_file(locale_file_path, FALSE, NULL, NULL); + gchar *locale_file_path, *file_path_utf8; + + locale_file_path = g_build_path(G_DIR_SEPARATOR_S, persistent_temp_files_target_dir, filename, NULL); + file_path_utf8 = utils_get_utf8_from_locale(locale_file_path); + GeanyDocument *doc = document_find_by_filename(file_path_utf8); + + g_free(file_path_utf8); + + if (doc == NULL) {
fixed
@LiquidCake commented on this pull request.
if (is_temp_saved_file_name(filename))
{ - GeanyDocument *doc = document_open_file(locale_file_path, FALSE, NULL, NULL); + gchar *locale_file_path, *file_path_utf8; + + locale_file_path = g_build_path(G_DIR_SEPARATOR_S, persistent_temp_files_target_dir, filename, NULL); + file_path_utf8 = utils_get_utf8_from_locale(locale_file_path); + GeanyDocument *doc = document_find_by_filename(file_path_utf8);
fixed
One more thing though - when playing with it, I didn't quite like the behavior of "close all" documents which also closes
good idea, done, seems to work good
@techee requested changes on this pull request.
Looks good and works well, again just some minor comments.
I also noticed that I get ``` (geany:47456): Geany-CRITICAL **: 22:18:37.629: document_open_file_full: assertion 'filename' failed ``` when starting Geany with `geany some_nonexistent_file.txt`. The corresponding backtrace when using `--g-fatal-warnings` is ``` #5 0x0000fffff7c27dc0 in document_open_file_full (doc=doc@entry=0x0, filename=<optimized out>, pos=pos@entry=0, readonly=readonly@entry=0, ft=ft@entry=0x0, forced_enc=forced_enc@entry=0x0) at document.c:1313 #6 0x0000fffff7c28140 in document_open_file (locale_filename=<optimized out>, readonly=readonly@entry=0, ft=ft@entry=0x0, forced_enc=forced_enc@entry=0x0) at document.c:908 #7 0x0000ffffeccd53fc in load_all_temp_files_idle (data=<optimized out>) at saveactions.c:624 ```
@@ -109,6 +109,12 @@ static gchar *config_file;
static gboolean session_is_changing = FALSE;
+int count_opened_notebook_tabs()
Make `static` and `int` -> `gint`.
show_unsaved_dialog_for_persistent_temp_files_tab_closing(
+ doc, + short_filename + ); + } + else + { + g_remove(doc->real_path); + + ui_set_statusbar(TRUE, _("Empty temp file %s was deleted"), short_filename); + } + + g_free(short_filename); + + } + else if (geany_is_closing_all_documents() && count_opened_notebook_tabs() == 1)
I wasn't sure if this would work but I just checked Geany code and documents are closed one by one so the check here should be OK.
{
+ /* remove temp 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, _("Temp file %s was deleted"), old_file_path_utf8); + } + + plugin_set_document_data(geany_plugin, doc, "file-name-before-save-as", NULL); /* clear value */ + } +} + + +static void load_all_temp_files_into_editor()
Also `void` here.
@@ -109,6 +109,12 @@ static gchar *config_file;
static gboolean session_is_changing = FALSE;
+int count_opened_notebook_tabs()
Plus `void`: `count_opened_notebook_tabs(void)` - this is not C++ as @b4n would say every time I forgot to add it :-).
@LiquidCake pushed 1 commit.
ea458866887dc9bc9dd2c18c82c7ec8d4f06d034 persistent-temp-files-plugin: code review 12 - fixes
@LiquidCake commented on this pull request.
@@ -109,6 +109,12 @@ static gchar *config_file;
static gboolean session_is_changing = FALSE;
+int count_opened_notebook_tabs()
fixed
@LiquidCake commented on this pull request.
{
+ /* remove temp 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, _("Temp file %s was deleted"), old_file_path_utf8); + } + + plugin_set_document_data(geany_plugin, doc, "file-name-before-save-as", NULL); /* clear value */ + } +} + + +static void load_all_temp_files_into_editor()
fixed
Any news on this?
> Any news on this?
you can build this branch and use the feature whether it is going to be merged and released at some point - i dont know but i guess later it might
also anyone (other than the PR creator) testing it, TELL US IT WORKS, that gets it more likely to be accepted
Based on my testing, it works well and I think it would be a nice feature.
I just think that merging the configuration with instant save would be good as I mentioned in https://github.com/geany/geany/pull/3911#pullrequestreview-2213059775 (but depends on what others think).
I had a quick look at the functionality (and the great video, it helps a lot to understand the feature) and this is a great addition to Geany.
I'm unsure about merging the configuration into the "instant save" tab. Though this feature kind of enhances the existing "instant save" feature and so it is probably a good idea.
Could we add a little explanation about the feature to the documentation at https://www.geany.org/manual/#save-actions (resp. https://github.com/geany/geany/blob/master/doc/geany.txt?plain=1#L5391)?
> I'm unsure about merging the configuration into the "instant save" tab. Though this feature kind of enhances the existing "instant save" feature and so it is probably a good idea.
Maybe to clarify what I had in mind - I would rename the "Instant Save" tab to "Untitled Document Save". At the moment, it's really unclear "Instant Save of what" it is. This tab would then look something like this: ``` | Untitled Document Save |
(o) Disabled ( ) Instant Save Directory to save files in [entry ] Current description ( ) Persistent Untitled Documents Directory to save persistent untitled documents in [entry ] Save interval [entry ] Default filetype [Combo] ```
This would 1. Improve the clarity of what "instant save" actually is 2. Improve UI because "Instant Save" and "Persistent Untitled Documents" are mutually exclusive and enabling one disables the other which is strange when this happens across tabs 3. Possibly allow Default filetype to be used by both the features as I think it makes sense in both cases.
> Could we add a little explanation about the feature to the documentation at
Before spending time on writing the documentation, we should probably first agree on how the configuration should look like and how this feature should be called. My suggestion is "persistent untitled documents" because it's the "untitled" that appears in the tab so it's clearer to users what it's about and I'd avoid the word "file" because it isn't a normal file.
> > I'm unsure about merging the configuration into the "instant save" tab. Though this feature kind of enhances the existing "instant save" feature and so it is probably a good idea. > > Maybe to clarify what I had in mind - I would rename the "Instant Save" tab to "Untitled Document Save". At the moment, it's really unclear "Instant Save of what" it is. This tab would then look something like this: > > ``` > | Untitled Document Save | > > (o) Disabled > ( ) Instant Save > Directory to save files in > [entry ] > Current description > ( ) Persistent Untitled Documents > Directory to save persistent untitled documents in > [entry ] > Save interval > [entry ] > Default filetype [Combo] > ``` > > This would > > 1. Improve the clarity of what "instant save" actually is > > 2. Improve UI because "Instant Save" and "Persistent Untitled Documents" are mutually exclusive and enabling one disables the other which is strange when this happens across tabs > > 3. Possibly allow Default filetype to be used by both the features as I think it makes sense in both cases.
Full agree, sounds reasonable. Thanks for the explanation.
> > Could we add a little explanation about the feature to the documentation at > > Before spending time on writing the documentation, we should probably first agree on how the configuration should look like and how this feature should be called. My suggestion is "persistent untitled documents" because it's the "untitled" that appears in the tab so it's clearer to users what it's about and I'd avoid the word "file" because it isn't a normal file.
:+1: We can also just create a follow-up issue for the documentation to not block this PR and save ourselves from forgetting it.
Added #4033 so docs don't get forgot.
@LiquidCake would you mind implementing the changes suggested in https://github.com/geany/geany/pull/3911#issuecomment-2455718032? I think then we can merge this PR.
> @LiquidCake would you mind implementing the changes suggested in [#3911 (comment)](https://github.com/geany/geany/pull/3911#issuecomment-2455718032)? I think then we can merge this PR.
yep will try
@LiquidCake pushed 1 commit.
1ad42976e31d8ed153a5eaa37ab7b22d8ebe7461 persistent-temp-files-plugin: renamed feature to 'persistent-untitled-documents'
@LiquidCake pushed 2 commits.
892ff56f74d33edcdeebd96d78c8bdfcef3e45d8 persistent-temp-files-plugin: created unified plugin config tab for IS and PUD 96ccb4b1029f8338a89064efadc0b9ad69ed5e0e persistent-temp-files-plugin: moved IS to common tab
@LiquidCake pushed 3 commits.
ba89848d469c0d46acb855c75f3d7c9ec315a20a persistent-temp-files-plugin: fix for AS label positioning e7ef20b96f19e399e0add645a0731e00a748d638 persistent-temp-files-plugin: fix b2a3979438ad500cae8b284d0508fe4d9837ffcb persistent-temp-files-plugin: fix
@LiquidCake pushed 1 commit.
5a5678f3131b98ca894de7f14d81f4503c32fd7b persistent-temp-files-plugin: added default file type to PUD
@LiquidCake pushed 1 commit.
4f74c086b646a200a45ba7abaf28dbfedd697885 persistent-temp-files-plugin: fix
done - instantsave and persistent untitled documents are merged into 1 config tab and config file section. default file type is now working for PUD. Not sure if it works correctly for IS but this is out of scope.
@LiquidCake pushed 1 commit.
05d65b3170d5be837f80bb1141375ea8c9796502 persistent-temp-files-plugin: fix
@eht16 commented on this pull request.
pref_widgets.persistent_untitled_docs_interval_spin = spin = gtk_spin_button_new_with_range(1, 600000, 100);
+ gtk_spin_button_set_value(GTK_SPIN_BUTTON(spin), persistent_untitled_docs_interval_ms); + gtk_label_set_mnemonic_widget(GTK_LABEL(label), spin); + + label = gtk_label_new(_("milliseconds")); + + gtk_box_pack_start(GTK_BOX(hbox), spin, TRUE, TRUE, 0); + gtk_box_pack_start(GTK_BOX(hbox), label, FALSE, FALSE, 5); + + gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0); + + /* Common */ + + label = gtk_label_new_with_mnemonic(_("Default _filetype to use for new files:")); + gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5); + gtk_widget_set_margin_top(GTK_LABEL(label), 15);
The cast to `GTK_LABEL` is wrong, just remove it.
@eht16 commented on this pull request.
+ image = gtk_image_new_from_stock(GTK_STOCK_OPEN, GTK_ICON_SIZE_BUTTON); + gtk_container_add(GTK_CONTAINER(button), image); + + hbox = gtk_hbox_new(FALSE, 6); + gtk_box_pack_start(GTK_BOX(hbox), entry_dir, TRUE, TRUE, 0); + gtk_box_pack_start(GTK_BOX(hbox), button, FALSE, FALSE, 0); + gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0); + + help_label = gtk_label_new( + _("<i>If you set the Instant Save directory to a directory " + "which is not automatically cleared,\nyou will need to cleanup instantly saved files " + "manually. The Instant Save plugin will not delete the created files.</i>")); + gtk_label_set_use_markup(GTK_LABEL(help_label), TRUE); + gtk_misc_set_alignment(GTK_MISC(help_label), 0, 0.5); + gtk_widget_set_margin_bottom(GTK_LABEL(help_label), 8);
The cast to `GTK_LABEL` is wrong, just remove it.
@eht16 commented on this pull request.
@@ -635,7 +1237,46 @@ static void checkbox_toggled_cb(GtkToggleButton *tb, gpointer data)
break; } } +} + + +static void radio_toggled_cb(GtkRadioButton *rb, gpointer data) +{ + gboolean enable = gtk_toggle_button_get_active(rb);
Here a cast is missng: ```suggestion gboolean enable = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(rb)); ```
I didn't review all the code changes in detail but tested it and works fine for me.
I wonder if we could make it visually more obvious that the directory and interval options belongs to the respective radio options. Maybe indenting them or wrap them in a `GtkFrame`?
@LiquidCake pushed 1 commit.
69668b87ba58fadd11901547f4e93112a1695438 Update plugins/saveactions.c
@LiquidCake pushed 1 commit.
ee34610bfb64044c00e8aad728f55c5c7229e2c6 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
+ image = gtk_image_new_from_stock(GTK_STOCK_OPEN, GTK_ICON_SIZE_BUTTON); + gtk_container_add(GTK_CONTAINER(button), image); + + hbox = gtk_hbox_new(FALSE, 6); + gtk_box_pack_start(GTK_BOX(hbox), entry_dir, TRUE, TRUE, 0); + gtk_box_pack_start(GTK_BOX(hbox), button, FALSE, FALSE, 0); + gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0); + + help_label = gtk_label_new( + _("<i>If you set the Instant Save directory to a directory " + "which is not automatically cleared,\nyou will need to cleanup instantly saved files " + "manually. The Instant Save plugin will not delete the created files.</i>")); + gtk_label_set_use_markup(GTK_LABEL(help_label), TRUE); + gtk_misc_set_alignment(GTK_MISC(help_label), 0, 0.5); + gtk_widget_set_margin_bottom(GTK_LABEL(help_label), 8);
removed
@LiquidCake commented on this pull request.
pref_widgets.persistent_untitled_docs_interval_spin = spin = gtk_spin_button_new_with_range(1, 600000, 100);
+ gtk_spin_button_set_value(GTK_SPIN_BUTTON(spin), persistent_untitled_docs_interval_ms); + gtk_label_set_mnemonic_widget(GTK_LABEL(label), spin); + + label = gtk_label_new(_("milliseconds")); + + gtk_box_pack_start(GTK_BOX(hbox), spin, TRUE, TRUE, 0); + gtk_box_pack_start(GTK_BOX(hbox), label, FALSE, FALSE, 5); + + gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0); + + /* Common */ + + label = gtk_label_new_with_mnemonic(_("Default _filetype to use for new files:")); + gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5); + gtk_widget_set_margin_top(GTK_LABEL(label), 15);
removed
I didn't review all the code changes in detail but tested it and works fine for me.
I wonder if we could make it visually more obvious that the directory and interval options belongs to the respective radio options. Maybe indenting them or wrap them in a `GtkFrame`?
i tried indenting blocks initially but rolled back because increase of overall height for this tab increases height for all of them. Both indentation and frame wrapping does not look ideal to me, but if we want we may use one or another. I committed indentation for now
@LiquidCake pushed 1 commit.
37b18c3426729bdbea42173891f0adcb6d95c7e2 persistent-temp-files-plugin: fix
@eht16 commented on this pull request.
@@ -635,7 +1237,46 @@ static void checkbox_toggled_cb(GtkToggleButton *tb, gpointer data)
break; } } +} + + +static void radio_toggled_cb(GtkRadioButton *rb, gpointer data) +{ + gboolean enable = gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(rb)); + + switch (GPOINTER_TO_INT(data)) + { + case NOTEBOOK_UNTITLEDDOCUMENTSAVE_RADIO_DISABLED:
I think we should also disable the "Default filetype to use for new files" combo box if both variants are disabled and enable it again if one of them is enabled. Suggestion: ```diff diff --git a/plugins/saveactions.c b/plugins/saveactions.c index f20cafcad..8d441a88f 100644 --- a/plugins/saveactions.c +++ b/plugins/saveactions.c @@ -1253,6 +1253,8 @@ static void radio_toggled_cb(GtkRadioButton *rb, gpointer data)
gtk_widget_set_sensitive(pref_widgets.persistent_untitled_docs_entry_dir, FALSE); gtk_widget_set_sensitive(pref_widgets.persistent_untitled_docs_interval_spin, FALSE); + + gtk_widget_set_sensitive(pref_widgets.untitled_document_save_ft_combo, FALSE); } break; } @@ -1263,6 +1265,8 @@ static void radio_toggled_cb(GtkRadioButton *rb, gpointer data)
gtk_widget_set_sensitive(pref_widgets.persistent_untitled_docs_entry_dir, FALSE); gtk_widget_set_sensitive(pref_widgets.persistent_untitled_docs_interval_spin, FALSE); + + gtk_widget_set_sensitive(pref_widgets.untitled_document_save_ft_combo, TRUE); } break; } @@ -1273,6 +1277,8 @@ static void radio_toggled_cb(GtkRadioButton *rb, gpointer data)
gtk_widget_set_sensitive(pref_widgets.persistent_untitled_docs_entry_dir, TRUE); gtk_widget_set_sensitive(pref_widgets.persistent_untitled_docs_interval_spin, TRUE); + + gtk_widget_set_sensitive(pref_widgets.untitled_document_save_ft_combo, TRUE); } break; } ```
I didn't review all the code changes in detail but tested it and works fine for me. I wonder if we could make it visually more obvious that the directory and interval options belongs to the respective radio options. Maybe indenting them or wrap them in a `GtkFrame`?
i tried indenting blocks initially but rolled back because increase of overall height for this tab increases height for all of them. Both indentation and frame wrapping does not look ideal to me, but if we want we may use one or another. I committed indentation for now
Sorry, I meant horizontal indendation. Like this: ```diff diff --git a/plugins/saveactions.c b/plugins/saveactions.c index f20cafcad..8d441a88f 100644 --- a/plugins/saveactions.c +++ b/plugins/saveactions.c @@ -1457,7 +1463,6 @@ GtkWidget *plugin_configure(GtkDialog *dialog) instantsave_radio = gtk_radio_button_new_with_mnemonic_from_widget( GTK_RADIO_BUTTON(disabled_radio), _("Instant Save")); pref_widgets.untitled_document_save_instantsave_radio = instantsave_radio; - gtk_widget_set_margin_top(instantsave_radio, 8); gtk_label_set_mnemonic_widget(GTK_LABEL(label), instantsave_radio); gtk_button_set_focus_on_click(GTK_BUTTON(instantsave_radio), FALSE); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(instantsave_radio), enable_instantsave); @@ -1469,6 +1474,7 @@ GtkWidget *plugin_configure(GtkDialog *dialog) _("_Directory to save files in (leave empty to use the default: %s):"), g_get_tmp_dir()); label = gtk_label_new_with_mnemonic(entry_dir_label_text); gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5); + gtk_widget_set_margin_left(label, 12); gtk_box_pack_start(GTK_BOX(inner_vbox), label, FALSE, FALSE, 0); g_free(entry_dir_label_text);
@@ -1487,6 +1493,7 @@ GtkWidget *plugin_configure(GtkDialog *dialog) hbox = gtk_hbox_new(FALSE, 6); gtk_box_pack_start(GTK_BOX(hbox), entry_dir, TRUE, TRUE, 0); gtk_box_pack_start(GTK_BOX(hbox), button, FALSE, FALSE, 0); + gtk_widget_set_margin_left(hbox, 12); gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0);
help_label = gtk_label_new( @@ -1495,6 +1502,7 @@ GtkWidget *plugin_configure(GtkDialog *dialog) "manually. The Instant Save plugin will not delete the created files.</i>")); gtk_label_set_use_markup(GTK_LABEL(help_label), TRUE); gtk_misc_set_alignment(GTK_MISC(help_label), 0, 0.5); + gtk_widget_set_margin_left(help_label, 12); gtk_widget_set_margin_bottom(help_label, 8); gtk_box_pack_start(GTK_BOX(inner_vbox), help_label, FALSE, FALSE, 0);
@@ -1503,7 +1511,6 @@ GtkWidget *plugin_configure(GtkDialog *dialog) persistent_radio = gtk_radio_button_new_with_mnemonic_from_widget( GTK_RADIO_BUTTON(disabled_radio), _("Persistent Untitled Documents")); pref_widgets.untitled_document_save_persistent_radio = persistent_radio; - gtk_widget_set_margin_top(persistent_radio, 8); gtk_label_set_mnemonic_widget(GTK_LABEL(label), persistent_radio); gtk_button_set_focus_on_click(GTK_BUTTON(persistent_radio), FALSE); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(persistent_radio), enable_persistent_untitled_docs); @@ -1513,6 +1520,7 @@ GtkWidget *plugin_configure(GtkDialog *dialog)
label = gtk_label_new_with_mnemonic(_("_Directory to save persistent untitled documents in:")); gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5); + gtk_widget_set_margin_left(label, 12); gtk_box_pack_start(GTK_BOX(inner_vbox), label, FALSE, FALSE, 0);
pref_widgets.persistent_untitled_docs_entry_dir = entry_dir = gtk_entry_new(); @@ -1531,6 +1539,7 @@ GtkWidget *plugin_configure(GtkDialog *dialog) gtk_box_pack_start(GTK_BOX(hbox), entry_dir, TRUE, TRUE, 0); gtk_box_pack_start(GTK_BOX(hbox), button, FALSE, FALSE, 0);
+ gtk_widget_set_margin_left(hbox, 12); gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0);
hbox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 1); @@ -1538,6 +1547,7 @@ GtkWidget *plugin_configure(GtkDialog *dialog) gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5); gtk_box_pack_start(GTK_BOX(hbox), label, TRUE, TRUE, 0);
+ gtk_widget_set_margin_left(hbox, 12); gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 5);
hbox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 1); @@ -1550,13 +1560,14 @@ GtkWidget *plugin_configure(GtkDialog *dialog) gtk_box_pack_start(GTK_BOX(hbox), spin, TRUE, TRUE, 0); gtk_box_pack_start(GTK_BOX(hbox), label, FALSE, FALSE, 5);
+ gtk_widget_set_margin_left(hbox, 12); gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0);
/* Common */
label = gtk_label_new_with_mnemonic(_("Default _filetype to use for new files:")); gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5); - gtk_widget_set_margin_top(label, 20); + gtk_widget_set_margin_top(label, 15); gtk_box_pack_start(GTK_BOX(inner_vbox), label, FALSE, FALSE, 0);
pref_widgets.untitled_document_save_ft_combo = combo = gtk_combo_box_text_new(); ``` This is reverting the vertical and adding horizontal indendation.
So the settings should visually better grouped to the according radio options.
While touching the code, I noticed two minor issues which would be nice to be fixed: - many lines have trailing white space - there is a mix of spaces and tabs used for indentation, should be tabs only (see the first two items on https://geany.org/manual/hacking.html#style)
@LiquidCake pushed 1 commit.
9d63587cd704c6877eb550823ec35fc9dff9c042 persistent-temp-files-plugin: fix
applied all proposed fixes
@LiquidCake pushed 1 commit.
9ca588e81efd3113973ce62317b4fa30adde998b persistent-temp-files-plugin: fix
@eht16 approved this pull request.
@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.)
@techee commented on this pull request.
{
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."));
Also documents->document
@techee commented on this pull request.
@@ -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)
`GEANY_API_VERSION` inside plugindata.h
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.
Maybe to elaborate a little more: - `untitled_doc` could be used for things that are common both for instant save and persistent untitled docs like the various config file dialog elements - `persistent_doc` could be used for things related to persistent untitled docs - `instantsave` for instantsave related things (which I think is the case already)
@LiquidCake pushed 1 commit.
62ebac6b405d62be5a9686649d1c39e217f4626f persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
- 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);
renamed. Dot is from original method which we sadly cannot just re-use 
@LiquidCake pushed 1 commit.
d7bdb70288d222f4c57da130cec02836f1494d73 persistent-temp-files-plugin: fix
@LiquidCake pushed 1 commit.
876dfdd94e78efce933c041c6d36ecad12c4b17c persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
@@ -285,19 +296,26 @@ static void backupcopy_document_save_cb(GObject *obj, GeanyDocument *doc, gpoint
}
+static GeanyFiletype *get_doc_filetype_or_default(GeanyDocument *doc) {
done
@LiquidCake pushed 1 commit.
cf11153f192f85bc55011d2416170b9594505c91 persistent-temp-files-plugin: fix
@LiquidCake pushed 1 commit.
e0bd7be7da2df69a88cd9e9c2c22e1b4bc16d2c5 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
{
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."));
done
@LiquidCake commented on this pull request.
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"));
done
@LiquidCake pushed 1 commit.
7b521da03d443b22966f328fc02013f5fcecb432 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
/* 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)
done
@LiquidCake pushed 1 commit.
fbcb3c409638502d466b04921c09597c7b55a113 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
@@ -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); +
done
@LiquidCake pushed 1 commit.
cea3e9c84dd21c724be8c67e7099e8a20187fafe persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
- 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 = "";
done
@LiquidCake pushed 1 commit.
5fa0f8897bb11e75b4d6c9f881d0ed5d04ea7007 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
- 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);
done
@LiquidCake commented on this pull request.
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 {
done
@LiquidCake pushed 1 commit.
cfe5c9908bcb7181aecc54744fe364051e4912d2 persistent-temp-files-plugin: fix
@LiquidCake pushed 1 commit.
2a228fc5f12d7be327b6240c9efc11deec7e6209 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
+ 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);
done
@LiquidCake commented on this pull request.
- 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);
done
@LiquidCake pushed 1 commit.
6d9d248bc4ced0e58f41b4456bd83a5e6e2f846d persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
@@ -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)
done
@LiquidCake pushed 1 commit.
15248a7675c7c8d3eb9354f9bd8a724f75ecf3d8 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
@@ -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)
done
@LiquidCake pushed 1 commit.
507795e982e71cf315dc9befad2a806ea2923268 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
{
/* 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);
done
@LiquidCake commented on this pull request.
}
pref_widgets;
-#define PERSISTENT_TEMP_FILE_NAME_PREFIX "gtemp_" +#define PERSISTENT_UNTITLED_DOC_FILE_NAME_PREFIX "unttld_"
done
@LiquidCake pushed 1 commit.
d68620011d6b4dd68d12d532597f740e63cdd629 persistent-temp-files-plugin: fix
@LiquidCake pushed 1 commit.
3f7b2da324631a60e8c54e56cb28176d1d544c78 persistent-temp-files-plugin: fix
@LiquidCake pushed 1 commit.
ac70c19a57e86cd876165ee9b9761eb87d1b0010 persistent-temp-files-plugin: updated API version
@LiquidCake commented on this pull request.
@@ -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
done
@LiquidCake commented on this pull request.
};
+enum +{ + NOTEBOOK_UNTITLEDDOCUMENTSAVE_RADIO_DISABLED = 0,
done
@LiquidCake commented on this pull request.
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;
done
@LiquidCake commented on this pull request.
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;
done
@LiquidCake commented on this pull request.
@@ -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)
done
@LiquidCake commented on this pull request.
@@ -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(
done
@LiquidCake commented on this pull request.
- @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)
done
applied proposed fixes
`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.` Should it? If so, probably someone may want to add some install script or smth to re-write config file
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.
Should it? If so, probably someone may want to add some install script or smth to re-write config file
Maybe there's a misunderstanding - it works alright. I.e. when someone uses Geany 2.0 with instantsave enabled and installs Geany 2.1, instantsave is still enabled (you reused the config file settings which is good).
I haven't checked but maybe one thing - when someone messes with the config file by hand and enables both instantsave and persistent docs, it would be good if only one became active (whichever, this is invalid settings, just not both of them because this could cause really unpredictable behavior).
@LiquidCake pushed 1 commit.
ca4589ff7fee905ed6fb75a0453b45ff465f58cc persistent-temp-files-plugin: added validation
`when someone messes with the config file by hand and enables both instantsave and persistent docs, it would be good if only one became active (whichever, this is invalid settings, just not both of them because this could cause really unpredictable behavior).` added check and default
`when someone uses Geany 2.0 with instantsave enabled and installs Geany 2.1, instantsave is still enabled (you reused the config file settings which is good).` I changed config file structure to reflect new config window and code structure. Existing 'enabled' flags appeared to fit ok into new structure so they were not changed, and this means that indeed, user dont have to re-enable instantsave after update. But other settings for instantsave are moved and thus not preserved and will go to defaults. Also old settings will not be cleaned up from config file automatically.
@techee requested changes on this pull request.
Looks promising! Variable name lengths are under control now :-)
I have a few minor comments but apart from them, I think we are close to merging this! 🎉
g_free(next_file_name);
- } else { + } + else + { if (strlen(extension_postfix))
Because of the added `g_strdup("")` above this check should be removed too.
config, "saveactions", "enable_persistent_untitled_documents", FALSE);
+ if (enable_instantsave && enable_persistent_docs) + { + dialogs_show_msgbox(GTK_MESSAGE_ERROR, + _("Invalid config file state: multiple features of 'Persistent Untitled Documents' are enabled at once")); + + enable_instantsave = FALSE; + }
Yes, this is what I had in mind, thanks. I'd just drop the message box - it may just be confusing for users what to do. Since the plugin does a reasonable thing by itself, I think it's OK without the message.
@@ -551,7 +554,7 @@ static void show_unsaved_dialog_for_persistent_untitled_docs_tab_closing(
case GTK_RESPONSE_NO: g_remove(doc->real_path);
- ui_set_statusbar(TRUE, _("Untitled document file %s was deleted"), short_filename); + msgwin_status_add(_("Untitled document file %s was deleted"), short_filename);
Not sure if I made a mistake before or you but I think `ui_set_statusbar` should be used here as this is the destructive action.
g_signal_emit_by_name(pref_widgets.checkbox_enable_backupcopy, "toggled");
+ g_signal_emit_by_name(pref_widgets.untitled_doc_disabled_radio, "toggled"); + g_signal_emit_by_name(pref_widgets.untitled_doc_instantsave_radio, "toggled"); + g_signal_emit_by_name(pref_widgets.untitled_doc_persistent_radio, "toggled");
Should be probably sufficient to emit it for just one of them, right?
g_free(tmp);
+ } + + tmp = utils_get_setting_string(config, "untitled_document_save", "persistent_untitled_documents_target_dir", NULL); + SETPTR(tmp, utils_get_locale_from_utf8(tmp)); + /* Set target dir variable with value from config, regardless if dir is valid or not */ + SETPTR(persistent_docs_target_dir, g_strdup(tmp)); + + if (enable_persistent_docs && ! is_directory_accessible(tmp)) + { + /* switch functionality off, so invalid target directory cannot be actually used */ + enable_persistent_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_doc_disabled_radio), TRUE); + gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(pref_widgets.untitled_doc_persistent_radio), FALSE);
This line should not be necessary I believe because of the previous line and because it's a radio.
}
+ } else {
`}` and `{` on separate lines
{
- g_key_file_set_string(config, "backupcopy", "backup_dir", backupcopy_text_dir); + /* If target dir path ends with dir separator - we consider it invalid */
Why? If it's a problem somewhere, it can be corrected by the plugin easily: ``` persistent_docs_text_dir[strlen(persistent_docs_text_dir) - 1] = '\0'; ```
if (enable_instantsave)
{ if (EMPTY(instantsave_text_dir)) { - g_key_file_set_string(config, "instantsave", "target_dir", ""); + g_key_file_set_string(config, "untitled_document_save", "instantsave_target_dir", "");
I think the whole plugin would deserve to define section and config names as macros - this is a bit error-prone.
Probably not necessary as part of this PR, I might open a separate PR with this afterwards.
+ 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);
Yes, it was you who made a mistake :-). Compared to the case above, `msgwin_status_add` should be used here.
I changed config file structure to reflect new config window and code structure.
Well, I think at least `target_dir` should be preserved under `[instantsave]`. Because if users changed it to some specific directory and upgrade to a new Geany version, it will be reset and files will suddenly be stored somewhere else which really isn't expected. I think the `default_ft` isn't so critical and can stay the way you changed it.
@techee commented on this pull request.
- label = gtk_label_new_with_mnemonic( - _("Directory _levels to include in the backup destination:")); + label = gtk_label_new(_("milliseconds")); + + gtk_box_pack_start(GTK_BOX(hbox), spin, TRUE, TRUE, 0); + gtk_box_pack_start(GTK_BOX(hbox), label, FALSE, FALSE, 5); + gtk_widget_set_margin_left(hbox, 12); + + gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0); + + /* Common */ + + label = gtk_label_new_with_mnemonic(_("Default _filetype to use for new files:"));
"new files" -> "untitled documents"
@techee commented on this pull request.
gtk_box_pack_start(GTK_BOX(notebook_vbox), inner_vbox, TRUE, TRUE, 5);
+ gtk_notebook_insert_page(GTK_NOTEBOOK(notebook), + notebook_vbox, gtk_label_new(_("Untitled Document Save")), NOTEBOOK_PAGE_UNTITLEDDOC); + + disabled_radio = gtk_radio_button_new_with_mnemonic(NULL, _("Disabled")); + pref_widgets.untitled_doc_disabled_radio = disabled_radio; + gtk_label_set_mnemonic_widget(GTK_LABEL(label), disabled_radio); + gtk_button_set_focus_on_click(GTK_BUTTON(disabled_radio), FALSE); + gtk_container_add(GTK_CONTAINER(inner_vbox), disabled_radio); + g_signal_connect(disabled_radio, "toggled", + G_CALLBACK(radio_toggled_cb), GINT_TO_POINTER(NOTEBOOK_UNTITLEDDOC_RADIO_DISABLED)); + + /* Instantsave */ + + instantsave_radio = gtk_radio_button_new_with_mnemonic_from_widget( + GTK_RADIO_BUTTON(disabled_radio), _("Instant Save"));
What about "Instant Save After Creation" here so it's clear when this happens (and that saving happens just once compared to persistent documents)?
@techee commented on this pull request.
+ gtk_widget_destroy(dialog); + + return ret; +} + + +static void show_dialog_for_persistent_doc_tab_closing( + GeanyDocument *doc, const gchar *short_filename) +{ + gchar *msg, *old_file_path_locale; + const gchar *msg2; + gint response; + + msg = g_strdup_printf(_("Untitled document %s is not saved."), short_filename); + msg2 = _("Do you want to save it before closing?");
I'd maybe drop "before closing" because it sounds like that you just close it and then can reopen it back - instead, the contents is just gone.
@techee commented on this pull request.
- return FALSE;
+} + + +static gint run_dialog_for_persistent_doc_tab_closing(const gchar *msg, const gchar *msg2) +{ + GtkWidget *dialog, *button; + gint ret; + + dialog = gtk_message_dialog_new(GTK_WINDOW(geany->main_widgets->window), GTK_DIALOG_DESTROY_WITH_PARENT, + GTK_MESSAGE_QUESTION, GTK_BUTTONS_NONE, "%s", msg); + gtk_window_set_title(GTK_WINDOW(dialog), _("Question")); + gtk_message_dialog_format_secondary_text(GTK_MESSAGE_DIALOG(dialog), "%s", msg2); + gtk_dialog_add_button(GTK_DIALOG(dialog), GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL); + + button = ui_button_new_with_image(GTK_STOCK_CLEAR, _("_Don't save"));
I'm a little worried that "don't save" doesn't sound as destructive as it should so users might lose their work.
What about calling this "_Don't save (discard)"?
@LiquidCake pushed 1 commit.
38521d712e1d6f4b705d8124b35a1bb5c6fe8ca7 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
g_free(next_file_name);
- } else { + } + else + { if (strlen(extension_postfix))
done
@LiquidCake commented on this pull request.
config, "saveactions", "enable_persistent_untitled_documents", FALSE);
+ if (enable_instantsave && enable_persistent_docs) + { + dialogs_show_msgbox(GTK_MESSAGE_ERROR, + _("Invalid config file state: multiple features of 'Persistent Untitled Documents' are enabled at once")); + + enable_instantsave = FALSE; + }
done
@LiquidCake commented on this pull request.
@@ -551,7 +554,7 @@ static void show_unsaved_dialog_for_persistent_untitled_docs_tab_closing(
case GTK_RESPONSE_NO: g_remove(doc->real_path);
- ui_set_statusbar(TRUE, _("Untitled document file %s was deleted"), short_filename); + msgwin_status_add(_("Untitled document file %s was deleted"), short_filename);
done
@LiquidCake commented on this pull request.
}
+ } else {
done
@LiquidCake commented on this pull request.
- label = gtk_label_new_with_mnemonic( - _("Directory _levels to include in the backup destination:")); + label = gtk_label_new(_("milliseconds")); + + gtk_box_pack_start(GTK_BOX(hbox), spin, TRUE, TRUE, 0); + gtk_box_pack_start(GTK_BOX(hbox), label, FALSE, FALSE, 5); + gtk_widget_set_margin_left(hbox, 12); + + gtk_box_pack_start(GTK_BOX(inner_vbox), hbox, FALSE, FALSE, 0); + + /* Common */ + + label = gtk_label_new_with_mnemonic(_("Default _filetype to use for new files:"));
done
@LiquidCake commented on this pull request.
gtk_box_pack_start(GTK_BOX(notebook_vbox), inner_vbox, TRUE, TRUE, 5);
+ gtk_notebook_insert_page(GTK_NOTEBOOK(notebook), + notebook_vbox, gtk_label_new(_("Untitled Document Save")), NOTEBOOK_PAGE_UNTITLEDDOC); + + disabled_radio = gtk_radio_button_new_with_mnemonic(NULL, _("Disabled")); + pref_widgets.untitled_doc_disabled_radio = disabled_radio; + gtk_label_set_mnemonic_widget(GTK_LABEL(label), disabled_radio); + gtk_button_set_focus_on_click(GTK_BUTTON(disabled_radio), FALSE); + gtk_container_add(GTK_CONTAINER(inner_vbox), disabled_radio); + g_signal_connect(disabled_radio, "toggled", + G_CALLBACK(radio_toggled_cb), GINT_TO_POINTER(NOTEBOOK_UNTITLEDDOC_RADIO_DISABLED)); + + /* Instantsave */ + + instantsave_radio = gtk_radio_button_new_with_mnemonic_from_widget( + GTK_RADIO_BUTTON(disabled_radio), _("Instant Save"));
done
@LiquidCake commented on this pull request.
+ gtk_widget_destroy(dialog); + + return ret; +} + + +static void show_dialog_for_persistent_doc_tab_closing( + GeanyDocument *doc, const gchar *short_filename) +{ + gchar *msg, *old_file_path_locale; + const gchar *msg2; + gint response; + + msg = g_strdup_printf(_("Untitled document %s is not saved."), short_filename); + msg2 = _("Do you want to save it before closing?");
done
@LiquidCake commented on this pull request.
g_signal_emit_by_name(pref_widgets.checkbox_enable_backupcopy, "toggled");
+ g_signal_emit_by_name(pref_widgets.untitled_doc_disabled_radio, "toggled"); + g_signal_emit_by_name(pref_widgets.untitled_doc_instantsave_radio, "toggled"); + g_signal_emit_by_name(pref_widgets.untitled_doc_persistent_radio, "toggled");
each one launches its own handler which disabled ui elements of unused features
@LiquidCake commented on this pull request.
g_free(tmp);
+ } + + tmp = utils_get_setting_string(config, "untitled_document_save", "persistent_untitled_documents_target_dir", NULL); + SETPTR(tmp, utils_get_locale_from_utf8(tmp)); + /* Set target dir variable with value from config, regardless if dir is valid or not */ + SETPTR(persistent_docs_target_dir, g_strdup(tmp)); + + if (enable_persistent_docs && ! is_directory_accessible(tmp)) + { + /* switch functionality off, so invalid target directory cannot be actually used */ + enable_persistent_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_doc_disabled_radio), TRUE); + gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(pref_widgets.untitled_doc_persistent_radio), FALSE);
same - each one launches its own handler which disabled ui elements of unused features - `radio_toggled_cb()` This code could be written in some other way but existing repeats existing handling for checkboxes and is generally ok
@LiquidCake commented on this pull request.
- return FALSE;
+} + + +static gint run_dialog_for_persistent_doc_tab_closing(const gchar *msg, const gchar *msg2) +{ + GtkWidget *dialog, *button; + gint ret; + + dialog = gtk_message_dialog_new(GTK_WINDOW(geany->main_widgets->window), GTK_DIALOG_DESTROY_WITH_PARENT, + GTK_MESSAGE_QUESTION, GTK_BUTTONS_NONE, "%s", msg); + gtk_window_set_title(GTK_WINDOW(dialog), _("Question")); + gtk_message_dialog_format_secondary_text(GTK_MESSAGE_DIALOG(dialog), "%s", msg2); + gtk_dialog_add_button(GTK_DIALOG(dialog), GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL); + + button = ui_button_new_with_image(GTK_STOCK_CLEAR, _("_Don't save"));
done
@LiquidCake pushed 1 commit.
c1c3673e91cc8982a678916d6df4474be08a5075 persistent-temp-files-plugin: fix
@LiquidCake commented on this pull request.
{
- g_key_file_set_string(config, "backupcopy", "backup_dir", backupcopy_text_dir); + /* If target dir path ends with dir separator - we consider it invalid */
done
applied proposed fixes, added comments to the rest
@LiquidCake Looks pretty good! I think we are pretty close to getting this merged.
Would you squash the commits and rebase the result on top of current geany master? I'd say - one commit with the added Geany API (i.e. all modified files except saveactions.c) - the modification of saveactions.c in another commit
By the way I did run into a strange issue during Geany quitting when this feature is used in combination with the LSP plugin. Not a problem of this PR, I'll try to address it with #4069.
@LiquidCake pushed 0 commits.
Closed #3911.
Reopened #3911.
@techee i've reset branch and re-applied changes as 2 commits, i guess this is what we want
I've just diffed it against the previous version and apart from a fixed whitespace, there's no other difference in saveactions.c 👍
I'll wait until the next weekend and unless anybody has some more comments, I would merge it afterwards 🎉
If you feel bored, it would be good to address https://github.com/geany/geany/issues/4033 - possibly as a separate PR so possible review comments don't block this PR. I think you don't have to go into too much detail and just describe the general functionality of this feature in https://github.com/geany/geany/blob/4264d0c507abc32c4e9f13aebaaec53d0c9e376b... (and possibly update the Instant Save section as the config is different now)
@techee approved this pull request.
Created PR for doc update https://github.com/geany/geany/pull/4077
I created 3 PRs with fixes to existing bugs i found during development. They are in my local fork, will re-target or smth if we want to accept those PRs.
Autosave cannot be disabled https://github.com/LiquidCake/geany/pull/4
Configured file type is not set to GeanyDocument https://github.com/LiquidCake/geany/pull/3
File templates are not supported https://github.com/LiquidCake/geany/pull/2
Autosave cannot be disabled
I think this patch should be added to this PR - it's pretty straightforward and fixes a problem introduced here.
Configured file type is not set to GeanyDocument
Does not affect this PR (it works for untitled documents) and was probably present before - I think you can open it once this PR is merged.
File templates are not supported
Alright, maybe I miss something but I think you can determine a non-file-backed documents just by checking if `doc->real_path` is NULL or not. But if you do, it would be best to "convert" such a document to "persistent" only when it gets modified, otherwise the result may be annoying (e.g., in the LSP plugin I show LSP server initialization handshake result in a new document. Unless modified, such documents can be closed in Geany without any warnings and you don't want to save such temporary documents).
In addition, I don't think you could change the "document-new" signal without affecting existing plugins. Also, plugins would have to be modified to use `document_new_file_with_creation_type()` consistently and it's kind of error-prone.
In any case, it can wait until this PR is merged.
Autosave cannot be disabled
I think this patch should be added to this PR - it's pretty straightforward and fixes a problem introduced here.
all 3 problems are in master branch, you can tell from how config window looks on videos, and PR for Autosave is directed to master (of my fork). But we can add Autosave fix to this PR no problem
Configured file type is not set to GeanyDocument
Does not affect this PR (it works for untitled documents) and was probably present before - I think you can open it once this PR is merged.
right, it does not affect PUD unless templates start working, then it would in a minor way. But we could just merge changes from that PR into current one or create new one afterwards or just forget about it - as you prefer
File templates are not supported
Alright, maybe I miss something but I think you can determine a non-file-backed documents just by checking if `doc->real_path` is NULL or not. But if you do, it would be best to "convert" such a document to "persistent" only when it gets modified, otherwise the result may be annoying (e.g., in the LSP plugin I show LSP server initialization handshake result in a new document. Unless modified, such documents can be closed in Geany without any warnings and you don't want to save such temporary documents).
In addition, I don't think you could change the "document-new" signal without affecting existing plugins. Also, plugins would have to be modified to use `document_new_file_with_creation_type()` consistently and it's kind of error-prone.
In any case, it can wait until this PR is merged.
Here is how i see it: Initially, Instantsave was working with files, created using template - for half a year, or never worked at all, who knows. But this exists in code and in documentation:
 
Then, we have this commit - now filename is pre-generated for template-created document and is set to document struct. (templates.c, `on_new_with_file_template()`) 
So from this point on, Instantsave is not working with such new files. But - the original intent was for it to work with it as with normal files - instantly create backing file at configured folder, not to do it "after tab recieves changes". Despite this might be annoying.
And this is how it works now in my PR, once we let Instantsave pass beyond `if (enable_instantsave && doc->file_name == NULL)` again. This could be fixed directly in master just for Instantsave, but my PR suggest changes that make PUD also work the same way.
If we dont fix this - we should at least remove mentions of supporting templates from geany.txt, in my PR https://github.com/geany/geany/pull/4077 i kept mentions and furthermore moved it to common part that describes both IS and PUD since their code in that place is common. Or ofc we could skip all these, again as you prefer.
In addition, I don't think you could change the "document-new" signal without affecting existing plugins.
This is not cool, yes, alternatives would be worse - add field to GeanyDocument or just change template-spawned file name to something definitive like "untitled-tpl" and check for this kind of name here `if (enable_instantsave && doc->file_name == NULL)` (as a hack).
Also, plugins would have to be modified to use `document_new_file_with_creation_type()` consistently and it's kind of error-prone.
Noone is oblidged to use it as old document_new_file() func still exists. If this doesnt brake ABI things should work for any plugin without re-compiling even. But here im not sure
i wont insist on doing anything as i dont personally need any of these to work and dont have that much spare time to keep working on it, but if we want/can complete this work and cut all tails - would be nice.
all 3 problems are in master branch, you can tell from how config window looks on videos, and PR for Autosave is directed to master (of my fork).
But we can add Autosave fix to this PR no problem
I don't care much, you can post it afterwards too.
right, it does not affect PUD unless templates start working, then it would in a minor way. But we could just merge changes from that PR into current one or create new one afterwards or just forget about it - as you prefer
I'm slightly lost in what you are proposing here - as I said, I think it can wait until this PR is merged.
So from this point on, Instantsave is not working with such new files.
I'm not questioning the fact that it doesn't work - I just don't think your implementation is the right one. You can't change the "document-new" signal - you break API towards plugins this way. Also, as I said, plugins would have to be modified to use `document_new_file_with_creation_type()` consistently and plugin authors would have to remember to do it correctly and I think this is the potential source of problems.
As I said, instead, the plugin itself should try to do it right by itself without any extra API. `doc->real_path` tells you whether there's a file backing the tab or not plus I think it should detect whether the file is modified or not before converting it to a persistent untitled document.
If we dont fix this - we should at least remove mentions of supporting templates from geany.txt, in my PR https://github.com/geany/geany/pull/4077 i kept mentions and furthermore moved it to common part that describes both IS and PUD since their code in that place is common.
This is an option too, depends which way you decide to go.
This is not cool, yes, alternatives would be worse - add field to GeanyDocument or just change template-spawned file name to something definitive like "untitled-tpl" and check for this kind of name here if (enable_instantsave && doc->file_name == NULL)
Again, I'd point you to `doc->real_path` (this isn't `doc->file_name`) - it tells you whether there's a file to which a tab is saved and based on this you can detect all "untitled" , template files, or any similar files created by plugins. No extra API needed IMO.
I don't care much, you can post it afterwards too.
Then i'll just create another PR after this one is merged
I'm slightly lost in what you are proposing here - as I said, I think it can wait until this PR is merged.
I proposed to add those changes right to this PR, but doesnt matter, i'll create another PR after this one is merged
I'm not questioning the fact that it doesn't work
ok got it about `real_path`, not sure about `document_new_file_with_creation_type` but anyway i guess we are not doing these changes as i wont be able to introduce new logic like detecting modifications etc. So this means templates issues is out of question, closing that PR.
Which leads us to the last open question - since templates are not working - what to do with geany.txt from another PR - should i remove mentions of templates from `Untitled Document Save` section, or move it back to `Instant Save` or just keep as is?
ok got it about real_path, not sure about document_new_file_with_creation_type but anyway i guess we are not doing these changes as i wont be able to introduce new logic like detecting modifications etc.
So this means templates issues is out of question, closing that PR.
To me it doesn't sound too difficult so I'll try to do it myself once this PR is merged. But of course on the way I can realize it's not so easy as I imagine and may decide it's not worth it.
Which leads us to the last open question - since templates are not working - what to do with geany.txt from another PR - should i remove mentions of templates from Untitled Document Save section, or move it back to Instant Save or just keep as is?
Since this is a separate PR, I'd say let's wait if I manage to implement it in which case we can keep it; otherwise we can drop it.
I don't care much, you can post it afterwards too.
Then i'll just create another PR after this one is merged
I'm slightly lost in what you are proposing here - as I said, I think it can wait until this PR is merged.
I proposed to add those changes right to this PR, but doesnt matter, i'll create another PR after this one is merged
In the light of the above, I think it would actually be best if you added both of the fixes to this PR so I can start the work on the finished result. I think you can keep these as separate commits (no rebasing needed).
@LiquidCake pushed 1 commit.
8cb2871dc2613ee41a747c5ccf049696ec3dcb4c untitled docs plugin: fixed issue with unset document file type
@LiquidCake pushed 1 commit.
a6d1346a76b17deb2180ad04ea62c64d059f3c6d autosave: fixed inability to disabled feature
done
To me it doesn't sound too difficult so I'll try to do it myself once this PR is merged. But of course on the way I can realize it's not so easy as I imagine and may decide it's not worth it.
@LiquidCake I just tried this in
https://github.com/techee/geany/commit/534a655da7fb64190b2c2ab5d16f1790116dd...
Maybe I'm overlooking something but IMO it's as simple as that. Could you have a look at it and give it a try?
looks good to me additionally there is another scenario of creation of non-empty document which is Document->Clone it is not considered by this code but seems to work ok anyway
looks good to me
Good to hear. Would it be OK if I push the change to this PR so we don't have to open another one?
additionally there is another scenario of creation of non-empty document which is Document->Clone
it is not considered by this code but seems to work ok anyway
And other from the plugins - e.g. the VC plugin shows git history in a non-saved tab. But you can start editing this document and then you probably want it to be persistent with this feature (on the other hand, when you don't edit it, you just want to close it without any warning which is why I added the modification check).
Would it be OK if I push the change to this PR
sure
And other from the plugins
i see, then original (broken) logic from instantsave was not really enough but we didnt know because it was blocked
@techee pushed 2 commits.
99a292e9b389f954c2c37729e63a3f5a84cb5404 Make instantsave and persistent document save work for templates and similar b40afcb856a222b1217b953bfadc2267c3b2bb42 Make padding identical to instant save settings in the dialog
Merged #3911 into master.
@LiquidCake The day has come, merging. 🎉
Thanks for all the work and your patience, really appreciated!
nice, thanks for you support
github-comments@lists.geany.org