-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support selecting multiple ranges #27
Comments
Good catch @petr-tik ! |
Judging from this code in the extension, https://github.com/karlicoss/grasp/blob/master/src/js/background.js#L90-L100 you call toString() on the selection, which bundles them together. If I understand it correctly, we can remove the toString() call, iterate over all selections and wrap them inside #+BEGIN_QUOTE #+END_QUOTE blocks. Do we need any changes in the server though? It looks like we can just send a bigger selection string with quote blocks inside. |
Yep, you're right about iterating, but extension doesn't know anything about BEGIN_QUOTE/END_QUOTE; this is handled by backend based on the template. (e.g. I don't use begin_quote/end_quote). |
Would you prefer not to use begin/end quotes or add support to a custom template? |
Discussed with @petr-tik and it seems that it'd be reasonable to pass multiple selections (as an array) from the extension and accept proper f-string as a template; that way it'd be easy to format the list to user's liking. |
Actually, f-strings are syntax, so wouldn't be possible to pass them as a commandline argument. But this brings us closer to supporting multiple seleciton.. |
since you merged the PR, is there anything else that needs to be done for multiple selections apart from updating client-side javascript to pass the full selections vector? I guess the server should offer a graceful fallback, if the template doesn't specifiy how to deal with multiple selections by gluing them together into 1 block (for example) |
Yep! Just saying it should be easier now. Extension changes: Backend changes:
The tricky bit is: the extension will be updated automatically; whereas the backend requires a manual intervention. That unfortunately means that if we start sending multiple selections to the backend, it will break it for the existing users :( So there are three options here:
Note to self: in the future, perhaps always need to expose a backend version throught an endpoint. So the frontend can act accordingly... Ugh. How hard can it be to to write a clipping extension :) |
If we can confirm that the failure was due to format discrepancy and nothing else, trying new way and retrying with the old format on failure should work, no? Why "meh"?
We can start with that now - who knows what other features you will extend grasp with in the future. Adding a "/version" endpoint to backend and client extension is purely additive. What happens if a new client sends a GET to the non-existing "/version" on the old backend. I tried googling about python's stdlib http.server and couldn't find anything. If the error doesn't crash the server and returns a consistent error code, we should be able to use that error code as an indicator of an old version eg. enum Version {
Prehistoric, // introduced when the backend still didn't have a /version endpoint
Tracked(u64), // returned by the backend with a version endpoint
// incremented when new features are added or breaking changes are introduced
} excuse the rust snippet - enums are too nice |
I found one bug though. If I make several text selections on one page, press Ctrl-Alt-Y to capture them, the selections end up concatenated without a newline character.
If it's not too heavy duty, it would make sense to me to create a
#+BEGIN_QUOTE
#+END_QUOTE
block for every selection. I don't think interleaving my comments with selections/quotes is necessary, but I think it makes sense to save selections as quotes.
Can I do this with a different org-mode capture template or does it require changing the extension?
Originally posted by @petr-tik in #7 (comment)
The text was updated successfully, but these errors were encountered: