@shiftee pushed 1 commit.

a8fc73c Add auto-file-reload feature to manual

@shiftee pushed 1 commit.

5c85d81 Improve auto-file-reload manual entry

We could probably have the preference defaulted to on, since it's most users would probably expect this behaviour, it doesn't clobber any changes (only happens if the buffer is clean), and it's more inline with the removal of reload confirmation and storing reloads in the undo buffer automatically.

VCS or something changed the file on disk doesn't mean I want to lose the buffer contents

You won't lose it, it's stored in the undo history automagically. Moreover, in this specific case of VCS, I usually would want it to auto-reload since I want to be working on the correct version of the file (though the change notification/infobar would nag me anyway sooner or later).

Currently you will never lose the buffer contents without your explicit approval.

Funny, it happens to me on a daily basis :) I often accidentally press Ctrl+R or hit the Reload toolbar button instead of Save All button, then it takes me a minute to realize what happened, then I remember the change that removed the reload confirmation and then undo the reload. It's more like implicit approval :)

That said, this feature might interoperate poorly with the feature that stores the whole buffer change in the undo history size on reload. Imagine one is viewing a large log file that is being frequently written to, while handy to have it auto-updated in Geany, the feature storing the whole buffer in undo history would cause massive memory footprint (more than just theoretically). It might be nice to have an option to disable the storing of the buffer in undo history on reload, and possibly even make it mutually exclusive with this new option.

Clearly you don't accidentally checkout an old version over your changes as often as I do :(​

It won't, well at least Git won't allow you to change branches with modified files, also Geany wouldn't lose your changes unless the buffer was clean. If it the buffer was clean, just use Undo to restore the previous version. Your changes are as safe as they are at present.

I don't think it should stop this PR since the problem exists even if you manually allow reload a few times

The difference is currently you have to manually (or in my case accidentally manually) reload, whereas if you have a file written to often (like multiple times per second) like the log file example, or even just switching VCS branches while many files are open a bunch of times, the memory will grow out of control much faster. As someone who tends to leave a single Geany instance running for a week or more at a time, this is a real concern.

But PRs are welcome to disable reload undo per document.​

IIRC this was rejected when the confirmation was removed from Reload and we started copying the whole buffer on each reload into the undo history.

So you actually want auto-reload off by default then.

No I want it on, but I don't want each auto-reload to store the whole buffer in the reload history.

Ok, well, unless @shiftee wants to make it so, thats not part of this PR

Why is dealing with bad interactions between features not part of a PR unless the submitter (solely) wants to make the change?

But until its done the default should be off.

I agree, but it can be done as part of this PR if we want to, by anyone, and is far simpler to do than the testing of this feature with all the related features/options will be before merging, so if nobody beats me to it, I might just do it myself while testing.

I have said I don't think it should affect the chances, and you have said you disagree, so its up to somebody else to determine then.

I don't much care if it's done as part of this PR or not, but if it's not, there will be a period where the master branch will contain a potential landmine of misbehaviour, and if it's not done quickly enough, this could include being present in a release, which is why it's nice to get all related stuff together.

The thing I care more about is that I want to use this feature myself (it's an often requested feature and some other editors I use have it), but I will be hesitant to use it because of some existing "bad" behaviour that never got a setting added to disable it (...because it wasn't included in the original PR :).

A nice byproduct of this feature, if enabled by default (or always) is that we could potentially start using proper file change notifications (GFileMonitor and friends which is already in the code but disabled). It needs testing but it seems like it would circumvent the double-notification problem.

Neat, have you solved the problem of it producing multiple notifications which was why it was abandoned last time IIRC?

But people who did NOT have the auto-reload turned on then would get multiple messages, unless you tied the type of monitor to the reload option. But that doesn't sound sensible, you are now supporting two monitor methods for what gain?

That's why in the first sentence you quoted I said "...if enabled by default (or always)...".

It would only be relevant to the "always" case, and always is not acceptable IHNSHO.

The current state of the pull request looks like this. ``` if( buffer.is_clean ) if( auto_reload ) if( file_prefs.keep_edit_history_on_reload ) //default=true possibleHighMemoryUsage(); else possibleDataLoss(); ``` It looks to me like we can remove the auto_reload option and make it automatic without data loss.

Am I correct in assuming you would prefer a finer-grained version of file_prefs.keep_edit_history_on_reload ??

If so, perhaps document_reload_force() and document_open_file_full() could be refactored to take a keep_undo_history parameter??

I do not consider changing the file monitor to be a part of this PR.

It looks to me like we can remove the auto_reload option and make it automatic

without data loss.

@shiftee two ways you can lose data with this scheme:

1) a modified file that has been saved (so the buffer is clean) and is then changed on disk may lose the changes that are still in the buffer if it always reloaded without asking. Think like "Oh, I'll just checkout file from Git to see what it has". Git replaces the modified file. Geany notices, reloads and BANG your changes are lost.

2) when the OOM killer closes geany without saving the files because it consumed too much memory :)

I agree, but for the moment it's probably wise to keep it as is since it may cause high-memory usage.

Agree with @codebrainz that the option is needed, think of a log file that is constantly updating, it will keep reloading and then save each of the previous contents in the undo buffer in memory. For a large logfile it could multiply memory usage very quickly.

The only potential change I can think might be to change the default enabled/disabled state of it, or perhaps making it mutually exclusive with the new auto_reload option.

Would fix the memory usage, but then you would lose the undo protection against accidental reloads any time auto reload was on.

It would be nice if Scintilla offered a circular redo-buffer to limit memory usage.

Older editors used to do that for memory reasons, but its rare to get the size right, even ignoring complete reloads we are talking of here. Normal edits are rarely a problem, so the buffer size doesn't need to be big, but consider refactoring where large parts of one file are moved to others, that can generate large changes and need a big buffer, but of course a big buffer is wasted for normal editing. No size satisfies anybody. But that was with 64kb computers, with current computers keeping all the changes for a session is usually viable. Its just we abuse it by saving reloads. Maybe in the future somebody will save reloads as deltas not the whole file, but thats another issue.

hmmm, not sure, on re-reading the thread it appears that @codebrainz and I have ended up agreeing to have the option off by default, I think.

LGBI

In which case it just needs someone to test it and commit it.

Hurry up, it's getting cold :)

@shiftee from time to time all open source projects that are not corporately supported have periods when available resources have a dip. People can be working on other projects that they didn't think would take anywhere near as long!!! (guilty). People are interrupted by real life (guilty). People are interrupted by that nearly universal scourge, making a living (most people guilty).

From discussions on IRC and elsewhere it seems Geany is currently in such a dip, and what resources are available have recently been involved in two releases in quick succession due to an unfortunate bug in the first release.

Although this PR looks ok (what my LGBI "looks good by inspection" means) it needs to be tested fairly well before committing and thats the resource we lack just now.

If you can get someone else (that its possible to check is reliable, eg by pointing to other projects they contribute to) to test it for unusual cases in a variety of situations, with a variety of filesystems (so it is triggered in a variety of situations, including the dreaded networked file systems like SSHFS) then it should be possible to commit it sooner.

Ah, I was not aware resources were slack. I saw all the activity around the release and thought it would be a good idea to remind the maintainers about this pr.

Its no problem to ping PRs, they do sometimes get forgotten.

If resources continue to be a problem then maybe a change of style is necessary. Perhaps a well-formed pull request could be pushed to master on a weekly or fortnightly basis and reverted if issues are reported

Well, yes and no. Certainly some lower risk things could be committed sooner, but...

Because its a mostly interactive program, Geany relies on testing by usage, not large amounts of automated testing. Because of the resource limitations, to get people to test the latest Git, it has to be stable enough that they are confident to use it for significant periods in real work. There are not enough resources to spend much time doing nothing but testing Geany.

So anything that reduces the reliability of the Git version also reduces the number of people that are willing to test the latest version, becoming a self-defeating exercise. So there is some caution about committing things without testing first.

A second issue (that I believe will affect this PR) is that the testers are using Geany in their normal configurations and work flows and that may not involve the particular change, few of us use remote file systems though many non-testing users of Geany seem to do so, only a few of us have multiple monitor setups, only a few use bleeding edge distros and so latest versions of GTK and Glib. So committing a PR doesn't mean it gets tested.

From the discussions above it seems likely that most Geany testers will either not want this feature turned on as it doesn't fit their workflow, or even if they turn it on, it won't get used much since their workflow won't trigger it. So it won't get tested much, and thats why I suggested you could try to find a tester.

@gdm85 as you will notice on #301, the OP is looking for testers whose workflow will involve this change. Since you are going to merge it into your copy of Geany and therefore presumably use it, maybe you can become the tester to help get this PR committed.

My last 2c on defaults: another way to decide the change is to "observe" what major distros (I am thinking of Debian, Ubuntu, Arch) do: if distros with most users are applying patches to change a default option value, then at some point (it might take one or more years) one can safely decide to incorporate the change based on the assumptions that most users have either already turned off the default or are fine with it.

I do Git checkouts/rebases often, and the need to reload every document is quite a nuisance. In fact, for the past few months I’ve been using a small private plugin that does the equivalent of #1471.

I will be running with this change to see how it works for me. I will also try to test it briefly with remote filesystems and under Windows.

However, I can already see why I will probably prefer a “reload all” feature like #1471 over this “auto reload” option. Geany has operations that depend on the current contents of the documents. Two that I use often are “goto symbol” and “find usage in session”. When I know that many of my open documents have changed on disk (because I just checked out a different commit), I’d rather reload them all eagerly so that those operations work correctly.

What I’m saying is, this change and #1471 are not equivalent, and it may make sense to have *both*.

@vfaronov thanks for testing, since we are only a week away from the next release (if it goes ahead) I'll hold off from committing it just now, OP or others prompt me after release if I forget.

Merged #1246.

Thank you,

Might be a good time to review the following issues: https://github.com/geany/geany/issues/301 https://github.com/geany/geany/issues/1388