bastibe / annotate.el

Annotate.el
Other
384 stars 20 forks source link

Fix closing indirect buffer #135

Closed cage2 closed 1 year ago

cage2 commented 2 years ago

When an annotated indirect buffer was closed the procedure was processing a database entry where the filename was nil. This type is invalid and an error was signaled, preventing the buffer to be closed.

This patch supposed to fixes #134

Bye! C.

bastibe commented 2 years ago

I wonder if it might be preferable to export the annotations in some other way instead of discarding them. I imagine it would be annoying to have annotated a file, then close Emacs and all your annotations are gone.

cage2 commented 2 years ago

On Mon, Aug 15, 2022 at 12:12:08AM -0700, you wrote:

Hi!

I wonder if it might be preferable to export the annotations in some other way instead of discarding them. I imagine it would be annoying to have annotated a file, then close Emacs and all your annotations are gone.

I agree! And that is why i asked JuanManuelM if this kind of "ephemeral" annotations was OK for them. In a sense the behaviour of annotation of indirect buffer seems to me that matches the one of the direct buffer. But i feel we can do a bit more. :) I also would like to annotation of indirect buffer persists someway like you wrote. My idea was to modify the database format to indicate that some annotations should be applied to an indirect buffer pointing to a base buffer and not to a buffer visiting the actual file.

We have now a format for a single annotated file like that:

(FILENAME ANNOTATIONS CHECKSUM)

where `FILENAME' is a string representing the path to the annotated file

for an indirect buffer `FILENAME' could become a list that helps the program to deal with indirect buffer:

(:indirect FILENAME BUFFERNAME)

With a format similar to the one above we could instruct the program to create an indirect buffer named BUFFERNAME' using a base buffer visitingFILENAME'.

As, unfortunately, i can not find an hook fired when an indirect buffer is created the best i could do was to manage indirect buffer inside: `annotate-load-annotations'.

The one above is just my best idea (and i would like to know your opinion about that) so far, i am not sure if this procedure can be actually implemented let alone the implementation details! :)

So said i think we supposed to decide what to do with this patch; even if not optimal is going to fix an annoying error. Do you think we should put that on hold until the above idea (or a better one!) is going to be implemented or merge this and starting working on a proper solution?

Bye! C.

cage2 commented 2 years ago

Hi!

I more or less implemented what i wrote in my previous message but the results are a terrible mess! :(

The two buffer seems to share the strings properties and this means that any annotation text in a the indirect buffer leaks in the base buffer and vice versa. Honestly so far i have no clue how to fix that issue. :(

Bye! C.

bastibe commented 2 years ago

Hi @cage2, that sounds tricky indeed. Perhaps we should display a big fat warning when trying to annotate an indirect buffer, saying that we won't be able to save annotations.

When closing an indirect buffer, perhaps we could dump the annotations to a new buffer and have the user deal with the new buffer manually. That way at least the annotations are not gone immediately. And if the user indeed does want to delete them, just kill the buffer.

So said i think we supposed to decide what to do with this patch; even if not optimal is going to fix an annoying error.

I agree with you. This at least fixes an error, and that's a good thing.

cage2 commented 2 years ago

Hi @bastibe !

When closing an indirect buffer, perhaps we could dump the annotations to a new buffer and have the user deal with the new buffer manually. That way at least the annotations are not gone immediately. And if the user indeed does want to delete them, just kill the buffer.

I think I implemented what you suggested; there are some sharp edge to smooth but i think the patch is more or less here.

I used a popup-window for the warning, it is very intrusive but i added a button to the window that allow to dismiss it permanently (when pressed, a customizable variable is set and saved in the user's configuration file).

Bye! C.

bastibe commented 2 years ago

Wow, great work! It's intrusive, but that's better than losing a user's work.

cage2 commented 2 years ago

Hi @bastibe !!

Wow, great work!

Thanks!

It's intrusive, but that's better than losing a user's work.

I agree! I just need to clean the code a bit and add missing docstring. Then i think i am going to merge this patch. I am bit busy lately that's because i am progressing slowly with that fix.

Bye! C.

cage2 commented 1 year ago

Hi!!

I think the patch is ready now! :)

Bye! C.

cage2 commented 1 year ago

Hi @bastibe !

I think i am going to merge this patch, not only because i think it is finished but i would like to finish working on #136 as it seems fun! :)

Bye! C.