[bookmarks] Closing tab deletes bookmarks

[sparklost]

Thanks in advance for your bug report!

• [x] Have you reproduced issue in safe mode?
• [x] Have you used the debugging guide to try to resolve the issue?
• [x] Have you checked our FAQs to make sure your question isn't answered there?
• [x] Have you checked to make sure your issue does not already exist?
• [x] Have you checked you are on the latest release of Pulsar?

What happened?

Bookmarks from file are deleted when its tab is closed. Also, all bookmarks are sometimes deleted when restarting Pulsar.

Pulsar version

1.128.0

Which OS does this happen on?

🐧 Arch based (Manjaro, Garuda, etc.)

OS details

Arch Linux, kernel 6.15.2-arch1-1, Gnome desktop, Wayland

Which CPU architecture are you running this on?

x86_64/AMD64

What steps are needed to reproduce this?

1. Add bookmark in current tab
2. Close this tab
3. Reopen same file
4. Bookmark is gone

Additional Information:

Happens in PulsarNext too.

[savetheclocktower]

Let me brain-dump from the discussion we had on Discord before I forget any further details:

1. I was surprised that the bookmarks package didn't do any of its own serialization of bookmark ranges. All it does is remember the IDs of markers, not the actual buffer ranges they cover. (We use display markers to track logical ranges of bookmarks in the buffer; this allows them to move around as needed in response to buffer edits above the bookmarked range.)
2. I suddenly realized that it does this because it relies on the existing serialization scheme of editors in general. When you close Pulsar and reopen it, the projects you already had open will be restored, and as much as possible about the previous state will be deserialized and put back the way it was, including DisplayMarker instances and their IDs. So bookmarks can survive this relaunch scenario…
3. …but they don't survive a much simpler scenario in which you simply close your editor tab and open it again. When you do that, Pulsar considers it a brand-new TextEditor and doesn't try to restore any state. Even though the file path is the same, the TextEditor instance itself is different.
4. You could argue that this is prudent on bookmarks’s part because, if it captures actual buffer ranges, it can't be certain that those ranges won't have changed by the next time the file is opened. (Like if the file is modified by some other process.) Except…
5. …the scenario described in item 2 above (deserialization of existing markers in the buffer upon relaunch) is just as likely to produce stale ranges, and we still plow ahead as though they're valid! (When restoring project and editor state, we don't seem to throw away existing state if a buffer was modified while we were closed.) If it's good enough for TextEditor deserialization, it's good enough for bookmarks deserialization.
6. It's fair to ask why Pulsar doesn't try to restore editor state when you close a tab and reopen it. Perhaps it's because it doesn't have a good way of knowing when an old TextEditor state should be reused. After all, closing and relaunching is about preserving and restoring the TextEditor instances that were already there. You can use the instances' own IDs (which start at 1 and increment over time, I think) to serialize and deserialize. But closing a tab and reopening it has to involve the destruction of the instance. We could still serialize closed tabs and try to restore them again later (with a cache limit so we don't keep a snapshot of every tab we've ever opened!) but then we'd need to key them on the buffer's file path or something that's arguably brittle. I don't see any huge problems with doing that, but it's a big task to undertake just to fix this bug. So…
7. The best solution for this specific problem is probably to capture explicit ranges of bookmarks outside of Pulsar's serialization lifecycle. We can then associate those ranges with specific paths in the project so that they can be restored if you close an editor tab and re-open it later at the same file path. It's possible that this will do the “wrong” thing sometimes, but no more likely than in the scenario from item 2 above; and the stakes are low because it's just bookmarks!