[Github-comments] [geany/geany] Fix 'changed' flag being altered when all documents are closing. (#1857)

Sat May 19 06:10:58 UTC 2018

>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 at 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?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1857#issuecomment-390382781
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20180518/9f1640d5/attachment-0001.html>