Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bookmark: Add bookmark support #17700

Closed

Conversation

CharlesChen0823
Copy link
Contributor

Currently bookmark can work, and had something not done.
Want to get some feedback and suggestion.

Release Notes:

  • Add bookmark support

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Sep 11, 2024
@ConradIrwin
Copy link
Member

Thanks for this!

I think bookmarks should be based on Anchors, not on row/column, so that as the code is edited the bookmarks stay in the "same place". In order to make this work reliably, we'd either have to ensure that creating a bookmark keeps the file open, so the Bookmark might end up being struct Bookmark{ buffer: Model<Buffer>, anchor: text::Anchor }.

We also need to think about multi-buffers. If you create a book mark from a project search, and then go to that bookmark, should it take you to that position in the underlying buffer, or in the multibuffer?

@ConradIrwin ConradIrwin self-assigned this Sep 12, 2024
@CharlesChen0823
Copy link
Contributor Author

CharlesChen0823 commented Sep 13, 2024

I think bookmarks should be based on Anchors, not on row/column, so that as the code is edited the bookmarks stay in the "same place".

I think there is an use case, we cannot easy toggle the bookmark remove, if we based on Anchors.

In order to make this work reliably, we'd either have to ensure that creating a bookmark keeps the file open

I had check some other implementation, IMO, bookmarks should exist even if the current buffer is closed, if using Modal<Buffer> to represent bookmark, we should keep buffer always opened?

@ConradIrwin
Copy link
Member

I think removing Anchors can still work, you just need to convert the anchor back to a point when checking?

If we retain the buffer in the bookmark store as a Model<Buffer> it will stay in memory even if the editor is closed. The alternative would be to convert between anchors and row/column when the buffer is closed - which you may want to do if you want bookmarks to persist when the workspace is closed and reopened.

@CharlesChen0823
Copy link
Contributor Author

CharlesChen0823 commented Sep 14, 2024

I think removing Anchors can still work, you just need to convert the anchor back to a point when checking?

I don't mean user cannot can remove bookmark, I thought when user toggle remove a bookmark, he should first find the correct point, then can remove it. It's not better UX.

If we retain the buffer in the bookmark store as a Model<Buffer> it will stay in memory even if the editor is closed. The alternative would be to convert between anchors and row/column when the buffer is closed - which you may want to do if you want bookmarks to persist when the workspace is closed and reopened.

Ahh, thanks, i'm misunderstood.

I will convert using Modal<Buffer>

@SomeoneToIgnore
Copy link
Contributor

This looks quite stale, so I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants