@elextr commented on this pull request.

@@ -3392,6 +3392,16 @@ static void force_close_all(void)

main_status.closing_all = FALSE; }

+void force_reload_all(void)

Since this is only called once suggest you put it in the callback, Geany is exhausting enough playing spaghetti functions :grin:

@Davidy22 commented on this pull request.

@@ -154,6 +154,7 @@ enum GeanyKeyBindingID

GEANY_KEYS_FILE_CLOSE, /**< Keybinding. */ GEANY_KEYS_DOCUMENT_REPLACETABS, /**< Keybinding. */ GEANY_KEYS_FILE_RELOAD, /**< Keybinding. */ + GEANY_KEYS_FILE_RELOAD_ALL, /**< Keybinding. */

Oh, sorry, didn't know so I put it next to the other reload. I see an API number and version in a comment, should I add one and increment?

@Davidy22 commented on this pull request.

@@ -3392,6 +3392,16 @@ static void force_close_all(void)

main_status.closing_all = FALSE; }

+void force_reload_all(void)

Done

@Davidy22 commented on this pull request.

@@ -154,6 +154,7 @@ enum GeanyKeyBindingID

GEANY_KEYS_FILE_CLOSE, /**< Keybinding. */ GEANY_KEYS_DOCUMENT_REPLACETABS, /**< Keybinding. */ GEANY_KEYS_FILE_RELOAD, /**< Keybinding. */ + GEANY_KEYS_FILE_RELOAD_ALL, /**< Keybinding. */

couldn't find apidata but a plugindata.h had a number to increment

@eht16 requested changes on this pull request.

Looks good and works as expected.

Beside my other minor comments, adding the keybinding to https://github.com/geany/geany/blob/master/doc/geany.txt#file-keybindings would be cool.

@@ -244,6 +244,8 @@ gboolean document_reload_prompt(GeanyDocument *doc, const gchar *forced_enc);

void document_reload_config(GeanyDocument *doc);

+void force_reload_all(void);

Seems like a left-over.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data)

The function name somewhat suggests there is a toolbar button while it is not. It might be unlikely that we will add a toolbar item for this functionality, so I think it's worth to rename the function to make it less confusing.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

Fine! What do you think about additionally ignoring documents which have `doc->changed` set? It might be surprising for the user if changed documents get reloaded and the unsaved changes will be lost without a warning.

@Davidy22 commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

But when a user hits CTRL+R to reload a single file that they've changed, they're expecting it to reload regardless of changed status. Hitting the keybinding being a conscious choice should mean that the user knows what they want

@Davidy22 commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data)

sort of was hoping there might be one but I'll rename

@eht16 commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

I guess we are both right: there is a setting (in the preferences dialog in the "Various" tab): "files.keep_edit_history_on_reload" and if this option is not enabled and the document has been modified, CTRL+R *will* display a confirmation dialog (see https://github.com/geany/geany/blob/master/src/document.c#L1651).

And so we could maintain this behaviour and either skip all modified documents if this option is disabled or just display a similar confirmation dialog as the current one before the reloading is performed.

While I agree with you that using the keyboard shortcut is probably a dedicated decision for most users, we still should respect that setting. Reloading unsaved changes while keeping the Undo history is fine but without the history, it might affect the user.

@Davidy22 commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

So, just pop the window when files with changes are being refreshed with the setting off? Alright should be doable.

@Davidy22 commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

So I've been using it for a bit and I guess it's technically correct, it notifies when there's undo history on a tab about to be reloaded and the message reflects that, but it's kind of annoying when tabs with saved documents are throwing the notification. I can see why this setting is true by default. If this is actually what we want I'll add it to the pull request.

@elextr commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

Right, that function is normally called by the response to reload on the file change detection info bar so it only happens one file at a time as you switch to the tab, so its less annoying. Also the info bar doesn't show and the reload is automatic if the buffer is not changed since last save and `file_prefs.reload_clean_doc_on_file_change` is set. So probably you could include that test before you called `document_reload_prompt()` to fully replicate the behaviour of the automated detection.

But it does beg the question why you need reload all since the automatic detection will do it as you switch to the tab?

@elextr commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

Pretty sure automatic detection doesn't do it as you switch to the tab

Hmmm, on checking it seems as if it is only done if you switch tabs by the tab right menu, not if you click the tab, I wonder why? I had the impression it was done each time a tab switch happened to ensure the infobar came up. Seems more like omission than intention. Will raise an issue.

file_prefs.reload_clean_doc_on_file_change is on ... it sounds like a world of accidental data loss

Correct, which is why its off by default and its hidden in the various prefs. But some people use Geany to watch logs and other things that regularly update, and they insist they need it. So its available as caveat emptor.

keep_edit_history_on_reload to conditionally quietly reload files with an edit history

The prompt should only appear if `files.keep_edit_history_on_reload` is off, if its on then data loss is avoided because the reload is undo-able. So automatic reload can happen irrespective of the buffer state. That seems to be what https://github.com/geany/geany/blob/90c6096ed6ea167f9100ce8f74229a3f47acc29a... does, but please let us know if it seems not to work.

The various tab seems to have become a catch all for settings because making changes to the glade file for the other preferences tabs was too hard when it was necessary to use a Glade version that was GTK 2 compatible and that no distro provided, so the dev had to compile it themselves. That might get better now we no longer use GTK 2. But the reload_clean_doc_on_file_change should stay hidden IMO.

@elextr commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

Ahh, maybe the meaning of keep_edit_history_on_reload is not entirely clear, what it does is it also adds the _reload_ to the edit history, so its undo-able, and all previous edits are also still there. So you don't "lose" anything because you can undo, but if its off the edit history is cleared because it may not apply to the newly loaded file, and now you can't undo. In your case above, having it off means that a file with an edit history (even if its saved) may have changes in the buffer that you are about to lose because the file has been overwritten, and you are about to load that over the modified buffer. So its correct to pop a prompt.

Saving the reload in undo is unusual behaviour, I'm not aware of any other editor or IDE that does it, and for sure it can use lots of memory if you do a big file lots, thats why the option is there. But with today's gigabyte memories, outside unusual use-cases like watching large logs that update often, its better to operate with keep_edit_history_on_reload setting on, which is the default.

@elextr commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

The dialog is necessary only if:

any of the open documents has unsaved changes keep_edit_history_on_reload is off

Sure, a single dialog is fine, but as I noted above, in the situation that a buffer has been changed and saved and then the file on disk is changed, for example by git checkout, is another case where you are about to lose your changes as the buffer is reloaded with the changed disk file. So just testing for unsaved is not sufficient and that is why the tests in `document_reload_prompt()` also check for any undo-able changes as an indication that the buffer was edited and saved.

I guess that, at least in the git checkout use-case the user had to explicitly specify files to change, since git won't overwrite modified files when just specifying a branch or tag or commit so you don't lose them. IIUC thats the use-case that @Davidy22 has, so I can see why he is grumpy that he gets a prompt for each of the files that he has explicitly told git to change.

But other file-changing use-cases may not be so user friendly and we open those users (who are not having any say in this discussion) up to the risk of data loss if we only test for unsaved changes when keep_edit_history_on_reload is off.

I suggest that if there is to be only one prompt then testing all files for unsaved changes or undo/redo data should be ok, especially when it only happens if keep_edit_history_on_reload is off.

So to summarise to ensure we are all on the same page, I am suggesting that if keep_edit_history_on_reload is off then a loop tests buffers for unsaved changes and undo/redo data and as soon as one is found exit the loop and make one prompt. If the result is "reload" or keep_edit_history_on_reload was found to be on then simply loop and reload all files with no further prompt.

@eht16 commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

After having a closer look and some testing, the logic and dialog response handling is a litte wrong: combining the conditions to show the dialog and handling its response in a single if statement is error prone if not impossible. The current implementation doesn't work properly.

As a suggestion, I rewrote the condition handling as follows: ```diff diff --git a/src/callbacks.c b/src/callbacks.c index acbd8737..487c48ac 100644 --- a/src/callbacks.c +++ b/src/callbacks.c @@ -340,27 +340,35 @@ void on_reload_all(GtkAction *action, gpointer user_data) { guint i; gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); - + if (!file_prefs.keep_edit_history_on_reload) { + GeanyDocument *doc; foreach_document(i) { - if (!(documents[i]->changed || (document_can_undo(documents[i]) || - document_can_redo(documents[i]))) && dialogs_show_question_full(NULL, - _("_Reload"), GTK_STOCK_CANCEL, _("Any unsaved changes will be lost."), - _("Are you sure you want to reload all files?"))) - break; - else - return; + doc = documents[i]; + if (doc->changed || document_can_undo(doc) || document_can_redo(doc)) + { + if (dialogs_show_question_full(NULL, _("_Reload"), GTK_STOCK_CANCEL, + _("Any unsaved changes will be lost."), + _("Are you sure you want to reload all files?"))) + { + break; // break the loop and continue with reloading below + } + else + { + return; // cancel reloading + } + } } } - + foreach_document(i) { if (! (documents[i]->file_name == NULL)) document_reload_force(documents[i], documents[i]->encoding); } - + gtk_notebook_set_current_page(GTK_NOTEBOOK(main_widgets.notebook), cur_page); } ``` This is a bit more verbose but IMO easier to read and understand.

Btw, stripping trailing spaces is always a good idea (at least for Geany code :D).

@Davidy22 commented on this pull request.

@@ -335,6 +335,21 @@ void on_toolbutton_reload_clicked(GtkAction *action, gpointer user_data)

document_reload_prompt(doc, NULL); }

+/* reload all files */ +void on_toolbutton_reload_all_clicked(GtkAction *action, gpointer user_data) +{ + guint i; + gint cur_page = gtk_notebook_get_current_page(GTK_NOTEBOOK(main_widgets.notebook)); + + foreach_document(i) + { + if (! (documents[i]->file_name == NULL))

Oh hum, didn't run into an edge case with the conditions combined but then I didn't leave the flag on very long, diff applied. Got in the habit of letting black format for me, miss some stuff sometimes without its help

@elextr commented on this pull request.

@@ -3349,6 +3349,8 @@ Close Ctrl-W (C) Closes the current fil

Reload file Ctrl-R (C) Reloads the current file.

+Reload all Reloads all open files.

Suggestion: "Reloads all open files. If changes that will be lost are detected (unsaved or saved) the reload will be confirmed, otherwise the reload will proceed without confirmation"

@elextr commented on this pull request.

@@ -3349,6 +3349,8 @@ Close Ctrl-W (C) Closes the current fil

Reload file Ctrl-R (C) Reloads the current file.

+Reload all Reloads all open files.

Actually: "Reloads all open files. If the reload will not be 'undo'-able and changes that will be lost are detected (unsaved or saved) the reload will be confirmed, otherwise the reload will proceed without confirmation"