-
Notifications
You must be signed in to change notification settings - Fork 69
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
Make Scan selected text lazy and add Scan text at selection #915
Conversation
915d3c0
to
cb6f254
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -119,6 +119,7 @@ export class Frontend { | |||
|
|||
this._hotkeyHandler.registerActions([ | |||
['scanSelectedText', this._onActionScanSelectedText.bind(this)], | |||
['scanTextAtSelection', this._onActionScanTextAtSelection.bind(this)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly if renaming is possible I would rather name the current scanSelectedText
to scanTextGreedy
and repurpose scanSelectedText
for this new functionality. I'm not sure about the naming of "lazy" tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about naming them like that but I feel like "greedy" and "lazy" aren't as well known terms to end users.
scanSelectedText just scans the selected text, simple.
scanTextAtSelection scans the text starting at the selection and does whatever it wants inside or outside the selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah these are user facing strings 🤔
Currently, the
Scan selected text
keyboard shortcut ignores the selection if it finds a word bigger than the selection. It makes more sense thatScan selected text
would only scan the selected text and not expand outside of it.To not break existing use cases of the previous
Scan selected text
I've addedScan text at selection
which has the previous behavior and is more in line with what would be expected of that name.Fixes #792