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