bastibe / annotate.el

Annotate.el
Other
384 stars 20 forks source link

Buffers marked as modified #74

Closed articuluxe closed 2 years ago

articuluxe commented 4 years ago

Hello and thank you for the package. It's neat.

As soon as I add an annotation, the buffer is marked as modified. I wonder if there is a way to get around this. The actual buffer contents to be written to disk have not changed; and only the annotation db needs to be updated. Is there a way to inhibit the modified status?

Thank you.

cage2 commented 4 years ago

On Tue, Jun 30, 2020 at 08:31:23AM -0700, articuluxe wrote:

Hi articuluxe!

Hello and thank you for the package. It's neat.

Thank you! :)

As soon as I add an annotation, the buffer is marked as modified. I wonder if there is a way to get around this. The actual buffer contents to be written to disk have not changed; and only the annotation db needs to be updated. Is there a way to inhibit the modified status?

Yes, i think there is a way to get what you asked for (but there are caveat, please read below):

The places where the code explicitly set the 'modified' bit "on" are two:

when all the annotations are removed from buffer:

https://github.com/bastibe/annotate.el/blob/master/annotate.el#L1322

and when a single annotation is created/modified/erased:

https://github.com/bastibe/annotate.el/blob/master/annotate.el#L402

If you comment out the form highlighted you will get something you want; but, unfortunately, this is not enough! :)

If you take a look at:

https://github.com/bastibe/annotate.el/blob/master/annotate.el#L1595

in this local function (delete) the buffer is actually modified by 'annotate--remove-annotation-property'. So the best way to deal with this (probably unintended, implicit) modification is to make something like what was done here before:

https://github.com/bastibe/annotate.el/blob/master/annotate.el#L1098

That is, saving the 'modified' bit before computation and restoring after (probably a macro could help here).

I think the latter could be very likely a bug (there is no need to set modification bit in the local function as 'annotate-annotate' will do anyway) thank you for helped me spotting this! But for the first i have mixed feeling.

If you does not mark the buffer as modified when a change -only in the annotations- occurs the hook 'after-save-hook' will not be fired and you will actually loose the annotations if you do not explicitly call 'annotate-save-annotations' and close the buffer. This is not something i would like happening to me! ;)

Anyway i have to investigate the bug above for potential regression (i guess there are not going to be regressions but some test or at least code inspection is needed) and, when i am going to fix the bug, if you just comment out the forms i mentioned in the links should do the job for you, but i do not recommend to do so, so please be careful, you could lose data!

Bye! C.

EDIT: fixed typo

bastibe commented 4 years ago

I agree with @cage2. Adding an annotation may not change the file content itself, but there are unsaved changes in the buffer, hence the modification flag. Think of it like adding and deleting a whitespace somewhere. Technically, the buffer content and the file content are the same, but it's still a modified buffer to Emacs.

I think that's a necessary inconsistency for annotate.el. Or do you have a better solution in mind?

articuluxe commented 4 years ago

Thanks for your response. It seems to me that adding an annotation changes the annotation database, and the buffer has not really changed, just has an additional decoration, like a linter warning or something. But I understand if that viewpoint is difficult to reconcile with implementation details.

Cheers.

On Jul 1, 2020, at 2:18 AM, Bastian Bechtold notifications@github.com wrote:

I agree with @cage2 https://github.com/cage2. Adding an annotation may not change the file content itself, but there are unsaved changes in the buffer, hence the modification flag. Think of it like adding and deleting a whitespace somewhere. Technically, the buffer content and the file content are the same, but it's still a modified buffer to Emacs.

I think that's a necessary inconsistency for annotate.el. Or do you have a better solution in mind?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bastibe/annotate.el/issues/74#issuecomment-652239505, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYO2SN6EIDR6DD643WFYV3RZLPKVANCNFSM4OMLMTBA.

bastibe commented 4 years ago

The thing is that the annotation database is only saved to disk if the buffer is saved. Thus if we didn't set the modified flag, your annotations might be lost when you close the buffer.

Do you know if there is a hook for when a buffer is closed? If that exists, we might be able to hook into that if there are unsaved annotation changes. So your feature request might be possible to implement.

Before we look into that however, I would ask you to explain your use case for it. What is it about the current modification flag behavior that you don't like? I am wondering if it is merely a "this feels wrong", or instead a "this leads me to wrong behavior" kind of thing.

cage2 commented 4 years ago

Hi everyone!

After #75 I believe would not be too difficult to add a switch, via a customizable variable, to prevent the buffer to be marked as modified. Anyway i would prefer that the standard behaviour (marks the buffer as modified when a changed in annotations occurs) was not changed.

Bye! C.

cage2 commented 4 years ago

Hi!

Now i realized that my last message could be misunderstood: i think that, if the aforementioned variable was implemented, its default value should be the one that maintains the current software behaviour (marks the buffer as modified when a changed in annotations occurs).

Bye! C.

indigoviolet commented 3 years ago

@cage2 Thanks for your work in #75. Was the variable you mentioned in https://github.com/bastibe/annotate.el/issues/74#issuecomment-653583487 implemented?

I also noticed that (perhaps related to #75), I cannot clear an annotation from a buffer marked read-only (buffer-read-only is t). That is unfortunate because I often like to annotate read-only buffers (to ensure I'm not changing them in any other way).

cage2 commented 3 years ago

Hi!

@cage2 Thanks for your work in #75.

You're welcome!

Was the variable you mentioned in #74 (comment) implemented?

Actually no, there was no interest to add this feature and i am not even convinced this could be a good idea.

What is your opinion about this feature? Bye! C.

indigoviolet commented 3 years ago

What is your opinion about this feature?

My opinion is not very strong, but I have a mild preference towards the opinion in https://github.com/bastibe/annotate.el/issues/74#issuecomment-653025118 . One way to handle the issue about unsaved annotations could be that if this potential flag ("don't mark buffer as modified") is turned on, we save the annotations after each annotate-annotate invocation.

hyOzd commented 2 years ago

Do you know if there is a hook for when a buffer is closed? If that exists, we might be able to hook into that if there are unsaved annotation changes. So your feature request might be possible to implement.

There is kill-buffer-hook. Can this be used?

cage2 commented 2 years ago

On Sat, Jun 11, 2022 at 03:57:34AM -0700, you wrote:

Hi!

Do you know if there is a hook for when a buffer is closed? If that exists, we might be able to hook into that if there are unsaved annotation changes. So your feature request might be possible to implement.

There is kill-buffer-hook. Can this be used?

Thanks for reviving this issue, honestly i forgot about it before your message! 😰

Reading the docs seems this is the right hook, thanks! I just have to check that the hooks is called when em's is closed (using C-x C-c), but I guess it is. I am even start to think that using this hook we can get rid of the "dancing" of the modified bit in the code completely (but likely I am wrong) and, this way, having a cleaner code!

Bye! C.

cage2 commented 2 years ago

Hi @hyOzd and everyone!

I have prepared a PR (#132 ) that uses the hook you suggested; turned out that my belief was wrong, though. 'kill-buffer-hook' is not called when closing emacs but, fortunately, 'kill-emacs-hooks' is. The patch will remove any changes to the modified status of the buffer so only changing the annotation of a buffer will not mark the buffer as modified.

Thanks for your suggestion! C.

cage2 commented 2 years ago

Hi everyone!

With #132 merged, I think this issue can be closed. Feel free to reopen if i am wrong.

Bye! C.