Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

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

-- 
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/1246#issuecomment-315700060

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

As promised post release.

-- 
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/1246#issuecomment-315687050

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

Merged #1246.

-- 
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/1246#event-1165970784

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Vasiliy Faronov]

@elextr Seeing as the release has been postponed, might be a good time to merge 
this now?

-- 
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/1246#issuecomment-313852021

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

@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.

-- 
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/1246#issuecomment-312571320

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Vasiliy Faronov]

So I promised to test this.

I’m not using this feature on a day-to-day basis for reasons I [explained 
previously](https://github.com/geany/geany/pull/1246#issuecomment-298176055), 
but I did test this PR specifically. I checked things like:

- Windows
- remote files (SFTP mounted via Thunar (GVfs-FUSE))
- checking out a different Git revision, so that many files need reloading
- “tailing” a log file right in Geany
- interaction with unsaved changes
- interaction with deleted files

I found no problems. The feature seems to work as expected.

-- 
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/1246#issuecomment-312567939

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Vasiliy Faronov]

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*.

-- 
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/1246#issuecomment-298176055

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

@gdm85 as far as I know, none of the major distros apply patches to change 
Geany's preferences. Also, due to the way Geany stores its preferences (writes 
them all to user-config dir with no concept of "unset" or "defaulted"), once an 
existing user opens Geany once, they will always have that default preference 
value until they delete their Geany config directory (or if installing fresh on 
a new OS), at which point Geany will write out the new default value.

-- 
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/1246#issuecomment-292722335

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[gdm85]

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.

-- 
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/1246#issuecomment-292709788

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[gdm85]

@elextr sure. I have only tested it with changes to 1 file and multiple files 
so far, no crashes. I will use this from now on so expect me to report back if 
I see some problem.

-- 
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/1246#issuecomment-292709270

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

@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.

-- 
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/1246#issuecomment-292709143

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[gdm85]

I think the defaults should preserve the historical way Geany has worked so far 
(one can make exceptions to this for "major" releases and/or reasons); this is 
just to not break the workflows of all those users that do not come here to 
complain or express their opinion :) Likewise, I would not discuss here 
personal preferences (I am always saving and committing very often, does it 
matter though?).

I was implementing something similar in #1463, this one seems much better 
implemented. I will merge this to my "personal" Geany branch for now, good work!

-- 
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/1246#issuecomment-292708551

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

> 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.

-- 
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/1246#issuecomment-290059319

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

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.

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

-- 
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/1246#issuecomment-290050958

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

@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.

-- 
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/1246#issuecomment-290047712

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

Would anyone be able to test this in the next few months?

-- 
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/1246#issuecomment-290008765

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

Hurry up, it's getting cold :)

-- 
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/1246#issuecomment-288328832

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

Sounds good. Does anyone have the time to test it in that case?

Thanks

-- 
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/1246#issuecomment-286673441

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

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.

-- 
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/1246#issuecomment-284749033

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

So what is the current status of this PR?

-- 
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/1246#issuecomment-284741356

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

> 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.


-- 
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/1246#issuecomment-250721182

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

Yes, both are valid depending on the value of 
file_prefs.keep_edit_history_on_reload.

Keeping it as an option defaulting to false is probably best.

It would be nice if Scintilla offered a circular redo-buffer to limit memory 
usage, I find the default behaviour strange:

> Scintilla has multiple level undo and redo. It will continue to collect 
> undoable actions until memory runs out.

-- 
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/1246#issuecomment-250701455

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

> 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.



-- 
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/1246#issuecomment-250638892

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

> It looks to me like we can remove the auto_reload option and make it automatic
without data loss.

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

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

No, it should be fine if it does what it sounds like. I thought there was only 
"show_keep_edit_history_on_reload_msg". 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.

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

I haven't looked at this code enough to say yet, I will have a look when I test 
this PR out.

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

Agree. I will check how it behaves with the changes in this PR while testing 
it, but it's basically unrelated at this point.

-- 
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/1246#issuecomment-250634630

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

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.

-- 
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/1246#issuecomment-250536557

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

We support like 20 different options and code paths for simply saving a file, I 
don't see an "if" statement to disable monitoring such a big deal in comparison 
:)

-- 
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/1246#issuecomment-249770805

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

> 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.

-- 
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/1246#issuecomment-249770506

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

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

-- 
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/1246#issuecomment-249770103

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

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?

-- 
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/1246#issuecomment-249769486

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

No, but like I said in the last sentence you quoted, it seems like the feature 
in this PR would circumvent it.

If you just saved a file and then get an FS event immediately it doesn't matter 
since the buffer is clean and there won't be any nagging UI. Except for the 
previously mentioned misbehaviour with storing a whole copy of each document on 
each reload in the undo buffer, which would cause additional wasted memory each 
save, it seems like it would make the problems with GFileMonitor go away (at 
least noticeably).

-- 
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/1246#issuecomment-249767273

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

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

-- 
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/1246#issuecomment-249766393

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

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.

-- 
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/1246#issuecomment-249756236

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

> 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 
:).

-- 
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/1246#issuecomment-249755085

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

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

The submitter can always say they don't want to do something, thats always 
their choice.  Not doing something may affect the chances of the PR being 
accepted, but its always the submitter's choice.

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.

> > 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.

Of course you can make another PR with the extra functionality, thats the point 
of open source. :)

-- 
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/1246#issuecomment-249753296

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

> 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.

-- 
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/1246#issuecomment-249751850

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

> 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.  But 
until its done the default should be off.

-- 
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/1246#issuecomment-249748679

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

> 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.

-- 
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/1246#issuecomment-249747467

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

> 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.

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

-- 
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/1246#issuecomment-249747263

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

> 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.

-- 
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/1246#issuecomment-249746594

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

On 27 September 2016 at 11:41, Matthew Brush 
wrote:

> 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).
>

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



> 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 :)
>

​But then I almost never do this :)​



> 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.
>

​Umm, yes, good point.  I don't think it should stop this PR since the
problem exists even if you manually allow reload a few times.  But PRs are
welcome to disable reload undo per document.​



> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> , or mute
> the thread
> 
> .
>


-- 
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/1246#issuecomment-249745451

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

> 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.

-- 
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/1246#issuecomment-249744623

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

Defaulting to on changes the current safe behaviour without any warning.  Just 
because the VCS or something changed the file on disk doesn't mean I want to 
lose the buffer contents.  Currently you will never lose the buffer contents 
without your explicit approval.

-- 
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/1246#issuecomment-249743199

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[Matthew Brush]

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.

-- 
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/1246#issuecomment-249742714

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

elextr approved this pull request.





-- 
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/1246#pullrequestreview-1524571

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

@shiftee pushed 1 commit.

5c85d81  Improve auto-file-reload manual entry


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/geany/geany/pull/1246/files/a8fc73c2477daaeb7eb960b2a74013e3486f1a2c..5c85d81d711a17635fb357d21ded5f78780fe9f6

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

elextr commented on this pull request.



> @@ -2601,6 +2601,10 @@ gio_unsafe_save_backupMake a backup when 
> using GIO unsafe file f
 keep_edit_history_on_reload   Whether to maintain the edit history when
trueimmediately
   reloading a file, and allow the operation
   to be reverted.
+reload_clean_doc_on_file_change   Whether to automatically reload documents
false   immediately
+  which have changed on disk.

Probably should say "...documents that have no changes but which have changed 
on disk...".  It should be spelt out because I don't think the term "clean" is 
used anywhere else, I think we only use "unchanged".

-- 
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/1246#pullrequestreview-1523102

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[shiftee]

@shiftee pushed 1 commit.

a8fc73c  Add auto-file-reload feature to manual


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/geany/geany/pull/1246/files/8c62c98517443ecc9eb6e1634ff72cb9cc3b61a7..a8fc73c2477daaeb7eb960b2a74013e3486f1a2c

Re: [Github-comments] [geany/geany] Added option to auto reload files changed on disk (#1246)

[elextr]

elextr requested changes on this pull request.

Looks ok by inspection, but the new preference needs to be documented in the 
table of various prefs in the manual.



-- 
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/1246#pullrequestreview-1518937