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

Support selecting multiple ranges #27

Open
karlicoss opened this issue Dec 15, 2019 · 9 comments
Open

Support selecting multiple ranges #27

karlicoss opened this issue Dec 15, 2019 · 9 comments
Labels
bug Something isn't working

Comments

@karlicoss
Copy link
Owner

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)

@karlicoss
Copy link
Owner Author

Good catch @petr-tik !
I've totally forgotten about it, so it's gonna require both changes in extension to support getRangeAt and also changes in the backend script to split selection parts.
The backend bit is trickly one, need to think what would be a good way to specify such a template.

@karlicoss karlicoss added the bug Something isn't working label Dec 15, 2019
@petr-tik
Copy link

petr-tik commented Dec 15, 2019

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.

@karlicoss
Copy link
Owner Author

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).

@petr-tik
Copy link

Would you prefer not to use begin/end quotes or add support to a custom template?

@karlicoss
Copy link
Owner Author

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.

@karlicoss
Copy link
Owner Author

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..

@petr-tik
Copy link

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)

@karlicoss
Copy link
Owner Author

Yep! Just saying it should be easier now.

Extension changes:

Backend changes:

  • change the signatures of as_org and make Config/DefaultConfig join the selected strings by default

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:

  1. If the user selected a single range, send it as a string (this will still break for people who did multiselection). Very easy, but half-assed.
  2. Make it defensive: try sending as a list, and if it failed, do it the old way. Easy, reliable, but sort of meh.
  3. Make it configurable: send it as a single string for the old addon users + add an option to switch to sending a list. For the new installs, always send as a list. Requires more settings etc, but the most robust way.

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 :)

@petr-tik
Copy link

Make it defensive: try sending as a list, and if it failed, do it the old way. Easy, reliable, but sort of meh.

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"?

Make it configurable: send it as a single string for the old addon users + add an option to switch to sending a list. For the new installs, always send as a list. Requires more settings etc, but the most robust way.

Note to self: in the future, perhaps always need to expose a backend version throught an endpoint. So the frontend can act accordingly...

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants