Please explain the need for this, what problem is it fixing?

Perhaps better to save the fold state on `document-before-save` signal?

The behavior I'm seeing is fairly straight forward to observe with the current state of the repository. In the 'document.c' file there is a function 'remove_page' which is the last function called to finish closing a document and remove references to it from Geany. This function is also where the 'document-close' signal is emitted by Geany, and the signal is sent to plugins really early on in the function. The code for this function up to the 'document-close' signal being sent looks like this:

``` static gboolean remove_page(guint page_num) { GeanyDocument *doc = document_get_from_page(page_num);

g_return_val_if_fail(doc != NULL, FALSE);

if (doc->changed && ! dialogs_show_unsaved_file(doc)) return FALSE;

/* tell any plugins that the document is about to be closed */ g_signal_emit_by_name(geany_object, "document-close", doc); ````

If you add in these lines:

``` if (doc->changed) { printf("Document %s has unsaved changes\n", doc->file_name); } else { printf("Document %s is up-to-date\n", doc->file_name); } ```

before the 'g_signal_emit_by_name' function call, you can print out the state of the document before plugin hooks are called to see what they see. The final function will now look like this:

``` static gboolean remove_page(guint page_num) { GeanyDocument *doc = document_get_from_page(page_num);

g_return_val_if_fail(doc != NULL, FALSE);

if (doc->changed && ! dialogs_show_unsaved_file(doc)) return FALSE;

if (doc->changed) { printf("Document %s has unsaved changes\n", doc->file_name); } else { printf("Document %s is up-to-date\n", doc->file_name); }

/* tell any plugins that the document is about to be closed */ g_signal_emit_by_name(geany_object, "document-close", doc); ```

Now just recompile Geany. Open Geany from the command line, then open a document. Make some changes to the document, close only that document using the UI tab and click "Don't Save" when prompted. The message you should see printed in the terminal is this:

``` Document /path/to/changed_file.c has unsaved changes ```

Now open that same document, make the same changes, and this time close Geany instead. Click "Don't Save" when prompted. The message you should see printed in the terminal is this:

``` Document /path/to/changed_file.c is up-to-date ```

To me this behavior looks like a bug. The documentation for the GeanyDocument struct states that the 'changed' flag shows if the document has been changed since it was last saved, but in this case that is not what plugins will see. Here is a screen shot of the behavior.


If this behavior isn't considered a bug, maybe the API documentation for document close can be updated to warn plugin developers of this behavior so they aren't tripped up by this when coding? If we can't use the 'changed' flag to check if there are unsaved changes, is there something else that I'm missing that can be checked, short of extracting the document contents and comparing them to the file on disk, to figure this out?

Things in Geany and plugins may depend on the "unchanged" status to prevent them doing stuff when the document is closed at shutdown.

I greped out the contents of the current plugins with this command at the root of the geany-plugins directory:

``` grep -Rni '->changed' * ```

and I get these results (you can obviously ignore git-changebar since changed_color_button is caught but unrelated):

``` geanylua/glspi_doc.c:331: SetTableBool("changed",doc->changed); geanypy/src/geanypy-document.c:120: if (self->doc->changed) geanyvc/src/geanyvc.c:595: if (doc->changed) geanyvc/src/geanyvc.c:683: if (doc->changed) geanyvc/src/geanyvc.c:831: if (doc->changed) geanyvc/src/geanyvc.c:896: if (doc->changed) geanyvc/src/geanyvc.c:974: if (doc->changed) git-changebar/src/gcb-plugin.c:1605: gtk_color_button_get_color (GTK_COLOR_BUTTON (cw->changed_color_button), git-changebar/src/gcb-plugin.c:1664: { "changed-color-button", &cw->changed_color_button }, git-changebar/src/gcb-plugin.c:1679: gtk_color_button_set_color (GTK_COLOR_BUTTON (cw->changed_color_button), scope/src/utils.c:365: document_set_text_changed(doc, doc->changed); /* to redraw tab & update sidebar */ ```

It looks like geanylua, geanypy, geanyvc, and scope are the only plugins that look at the 'changed' flag.

geanlua uses this flag to query information about documents already open, but doesn't do anything with document close that I can see.

geanypy uses this flag to provide functions to query the state of the document, but doesn't do anything with it during document close that I can see.

geanyvc uses this flag to see if it needs to auto save files, which in this case means the current behavior may cause a bug in this plugin, but I don't see anything with document-close so probably not. Maybe that plugin author can chime in? I don't know what the best way is to contact them so they can see what's going on in this thread.

scope uses this flag when re-painting a document, but never during document close that I can see.

AFAICT, the only reason Geany does this is because the 'document_account_for_unsaved' function pops up a dialog for all changed documents before closing *any* documents. If the 'changed' flag isn't set to FALSE, then the 'remove_page' function that will get called later would cause a second dialog to pop up for changed documents when closing. When playing around with this I actually observed this behavior. Setting changed to FALSE fixes that, and since Geany doesn't save any information WRT the changed flag - just a list of currently opened files AFAIK - it shouldn't cause any larger effects within Geany. I suspect this is also why the 'document_account_for_unsaved' function itself has a note that indicates a TRUE return from this function should *always* be followed by 'document_close_all', because otherwise opened documents with changes could get left in a weird invalid state.

I address this double pop up issue instead by inspecting the main_status.closing_all flag when 'remove_page' is called. This flag gets set whenever 'document_close_all' is being invoked, and since 'document_close_all' handles the dialogs for unchanged files with a call to 'document_account_for_unsaved', it makes sense that the 'remove_page' function should delegate this behavior further upstream in this case.

The only issue I can see with this approach is that 'document_account_for_unsaved' would now get called twice during a call to the 'main_quit' function triggering a double pop up again, so I removed the 'document_account_for_unsaved' function call there, and let the 'document_close_all' function that gets called in 'do_main_quit' call it instead.

Unfortunately the 'do_main_quit' function never inspects the return value of 'document_close_all' (since changes upstream woulds always return TRUE before), so I had to change the 'do_main_quit' function to return a FALSE value if the 'project_close' or 'document_close_all' returns FALSE, and TRUE otherwise so that the 'main_quit' function can still be aborted by the user, just how Geany works currently. AFAICT, 'document_close_all' and 'main_quit' are the only two places where the 'document_account_for_unsaved' function is called directly.

If it was added pre-git any discussions may not have been transferred from SVN, but they may also have happened on the dev ML, so the archive would need to be checked around the date the function was added.

I tried to track down when 'document_account_for_unsaved' was first added and the first commit I see is 2008 April 24 with the git SHA of 3afebc701c47f4de34c0bb9b52fadaa2f5adb781. This predates what I can see in the mailing list archives. Also, it looks like this commit was really just a refactoring of function calls in 'callbacks.c', which moved the 'account_for_unsaved' function in 'callbacks.c' to 'documents.c' and renamed it.

Prior to this the 'account_for_unsaved' function didn't alter the 'changed' flag, but instead this behavior was in the 'force_close_all' function in 'callbacks.c' which was invoked during Geany quit. The 'force_close_all' function first pops up ~3 weeks prior in 2008 April 4 with the git SHA 9a67c39a70f7a13b1c6c56a86a43688b80a2da6c. This commit also looks like it was updating the close all function to prompt dialogs for unsaved changes as well as refactoring code using the 'force_close_all' function to simplify the 'quit_app' function which is where this 'changed' flag alteration was occurring previously.

The first commit that I can see that introduces the 'quit_app' function in Geany goes back to a commit in 2006 November 7 with the git SHA 20637b939b916f8082a5ce3a5a957c316efc93dd. This commit looks like it is refactoring the 'on_exit_clicked' function and altering how dialog pop ups are used when closing a document with unsaved changes.

Prior to that, Geany never touched the 'changed' flag when closing all documents, and instead let the 'document_remove' function handle the dialog pop up when closing a document with unsaved changes. I believe this is why the commit was added in the first place. Prior to the Nov. 7 commit, Geany would start closing documents when exiting, and if the user canceled in the middle of it, that would leave everything open except for the documents that had been closed. That would be really annoying. The message that goes along with this commit appears to support this:

``` Author: Nick Treleaven nick.treleaven@btinternet.com Date: Tue Nov 7 11:24:22 2006 +0000

Don't close any tabs when quitting until all unsaved changes have been accounted for; switch to each unsaved file before showing the unsaved dialog. Remove limit of ~256 chars for session filenames. Make dialogs_show_unsaved_file() fail if the Save As dialog was cancelled.

git-svn-id: https://geany.svn.sourceforge.net/svnroot/geany/trunk@972 ea778897-0a13-0410-b9d1-a72fbfd435f5 ```

At the time, if the 'changed' flag was left unaltered the same double pop up would have happened that I mentioned earlier. So that would explain needing to set it to FALSE in the first place. Inspecting the main_status.closing_all flag instead makes this a non-issue however. I couldn't find anything in the SVN project page in source forge from around that time that referenced this commit or the behavior it would have addressed, so I don't know what other discussion would have occurred to talk about these changes.

From looking at the commits, it looks like Nick was maintaining this code, with the 2008 April 24 commit being the last refactoring of this function. The function has remained virtually unchanged since and has only been updated as variable names referenced were changed or new macros were added. The last commit I see from Nick Treleaven (@ntrel) is over a year ago, so I don't know how closely he is watching this, but maybe he can chime in if at all possible and offer some context.

Are there others who have worked on / have knowledge of these functions that could comment on whether this looks wrong or not?

Also needs an ABI break, otherwise any plugin that accidentally or deliberately depends on the current behaviour will fail without any indication. Although the G-P plugins seem not to depend on it, there are other plugins and personal plugins too.

BTW, a workaround for getting a more correct value before this bug is resolved could be using Scintilla's [`SCI_GETMODIFY`](https://scintilla.org/ScintillaDoc.html#SCI_GETMODIFY): I would think it should be reliable but for encoding changes (enabling/disabling BOM, choosing another encoding to save the file, etc.), which although relevant are a lot less common.

I would think it should be reliable but for encoding changes (enabling/disabling BOM, choosing another encoding to save the file, etc.), which although relevant are a lot less common.

Those don't affect the buffer, so they shouldn't affect the `changed` flag should they?

They do affect our internal changed flag, as they require saving to take effect;

Ok, if you set them with the `Document` menu they do set `changed`.

as @chrontec didn't reply yet (not a critic, we took enough time to catch up ourselves :confused:), I applied my fixes and committed in 5f386751141a38efc6a64508afec92c9a82b6c06.