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

Goto within list #50

Merged
merged 4 commits into from
Dec 28, 2024
Merged

Goto within list #50

merged 4 commits into from
Dec 28, 2024

Conversation

nfginola
Copy link
Contributor

@nfginola nfginola commented Dec 27, 2024

Resolves #48

Implements wrap-around supported goto next/previous within the same active BookmarkList by order id.

This feature requires keeping track of some transient working state (last visited bookmark within the same list) which I've kept in a context table in Service. I assume here that Repo is used for data that would be stored persistently, which is I thought it is appropriate to put in in Service. If there's a better place, let me know.

@nfginola nfginola force-pushed the goto_within_list branch 2 times, most recently from d034f20 to efc7f83 Compare December 27, 2024 21:41
More complicated to support being able to disable it for other traversal methods (such as BookmarkList traversal). If requested, this support can be added later.
@nfginola nfginola force-pushed the goto_within_list branch 2 times, most recently from 40cdd85 to a4d3ece Compare December 27, 2024 22:27
@nfginola
Copy link
Contributor Author

nfginola commented Dec 28, 2024

Made a modification to store away the per-list last visited bookmark to storage so its persistent.
This forced me to delete my database due to incompatible schema.

What's the strategy here when we want to update the database schema more in the future to potentially hold more state?

@LintaoAmons
Copy link
Owner

Made a modification to store away the per-list last visited bookmark to storage so its persistent.

Maybe just get all bookmarks in the active list from db then do the comparison with the last visited date field in the memory?

@LintaoAmons
Copy link
Owner

What's the strategy here when we want to update the database schema more in the future to potentially hold more state?

Yeah... Currently only make a breaking change, so I prefer keep the schema stable unless we have very strong reason to add a new state to db...

@nfginola
Copy link
Contributor Author

nfginola commented Dec 28, 2024

Maybe just get all bookmarks in the active list from db then do the comparison with the last visited date field in the memory?

Haha yeah, I completely missed I can find the latest visited by the already exposed visited date 💀
I made a change at it works as expected locally, I'll remove all the database changes.

@nfginola
Copy link
Contributor Author

Implementation fixed, no breaking changes anymore, verified locally traversing as expected.

Text could be misinterpreted as ordering by ID, and not by 'order id'.
Clean up documentation, and use 'visited at' to find the last visited in
the bookmark list.
Visited at is not sufficient for fast goto since it is being updated on
seconds frequency at the minimum.
@nfginola
Copy link
Contributor Author

nfginola commented Dec 28, 2024

I noticed when working on some other project that the goto was.. intermittently laggy. It wasn't as snappy as the previous goto implementation.

Turns out last visited uses os.time() which increases at per-second rate and this messed up the last visited selection.
Since this occurs when you are performing goto very fast, I added a fallback to use the bookmark under the cursor if there are more than one bookmarks in the list with the same 'last visited'.

This works fixed the issue and now we are snappy again ☺️

@LintaoAmons
Copy link
Owner

Wonderful!
Thanks for your help!

@LintaoAmons LintaoAmons merged commit 4648756 into LintaoAmons:main Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Next/Prev mark in the active list (order by order id)
2 participants