Implements shortcut feature (related to #149)

[igorofc]

• Adds an additional field on the 'Search engines' section of the settings page for each engine, which will hold a single character to be used as a shortcut for that engine. This character can either be a letter, number or any other that doesn't require a modifier (e.g. Alt, Ctrl, Shift) to be typed.

• To activate the shortcuts the user has to check the option Use shortcuts for the search engines. Upon enabling this setting, another option named Always enable shortcuts will appear, which, if checked, enables shortcuts even without the popup showing up.

• Shortcuts don't work on editable fields.

• The user can control how the search will be carried out by selecting the opening behaviour under the option When using a shortcut.

• Additionally, the user can select the engine by using the TAB key and hitting Enter. In this case, the same opening behaviour used for shortcuts will apply.

[CanisLupus]

Hey Igor! :) I've looked at your code over the past few days and made small formatting changes (first commit) and minor refactorings (second commit). Hopefully you can see them. Please take a look and check if you see any problems. I can ellaborate on any change if the commits are not clear enough. :) From my testing everything seemed normal after the changes, but I could be mistaken.

I actually wanted to make a few changes that are deeper, but I wanted to get your input first i.e. if you agree, or if you thought about these already and saw possible problems:

1. Remove "Use shortcuts for the search engines" and only keep the option to always allow.

From experience, I feel that users will be confused if they set a shortcut and it doesn't work until (if!) they discover the setting way below in the options menu. I think shortcuts are something that can always be enabled if the popup is visible. This would also simplify the settings page code to show the "Always enable shortcuts" field since it would always be visible with no parent setting. What do you think?

2. Don't create the popup just for always-on shortcuts. Perhaps register a new keydown listener just for shortcuts and inside it check if the popup exists (if it needs to) and if the setting is enabled.

Aside from the optimization of not creating the popup in case the opening behaviour is Off, it would avoid the need for this section: (since we wouldn't depend on the popup being created to use shortcuts)

and allow us to move the line if (settings.popupOpenBehaviour === SSS.PopupOpenBehaviour.HoldAlt && !ev["altKey"]) return; back to the top of the function where it was. This block being down here actually changes some things that may be undesired. The auto copy options, for example, only work when the popup appears, and if the user is using "Alt" to open the popup, it will auto copy and only then check if the popup should appear.

3. (minor one) Make it possible to keep overwriting shortcut if a letter was already inserted. So if you made a mistake inserting the shortcut you just press any other letter while the input field is focused and it keeps writing over the existing one.

I hope I explained this clearly enough. What do you think about these? By the way, if you agree with them I can implement them no problem. It was not my intention to ask you for changes, it's just things that I think make sense. :)

Cheers! Daniel

[CanisLupus]

PS: I'm not sure if I made these commits in the right place, but I followed GitHub's instructions for pulling your PR and committed over that. :p If this wasn't the right place (likely) I can redo the commits somewhere else later, but here are the changes I mean.

[igorofc]

Hi @CanisLupus. Thanks for reviewing this PR.

1 - This makes sense. If the user types a shortcut it's obvious he/she want's that to work right away without the need to further confirming it somewhere else. That checkbox should be removed then.

2 - Actually the only reason for that change was so that we didn't have to include another onkeydown listener and leverage the existing one - which is only created when the popup is created. I wasn't aware that it was breaking other parts of the code, though. Let's revert those changes.

3 - Nice one. That's a great improvement to user experience. Less work is always better :).

The only thing I'd say should be kept is the name property that was added to the default engines in src/swift-selection-search.ts. That's because the confirmation message that appears when setting a shortcut already assigned to one of those engines would read: This shortcut is already assigned to 'undefined'! Override? if that property is missing.

I understood your explanation and agree with your changes. You can go ahead and implement them. I'm enjoying contributing so I'm more than willing to help should you need any assistance.

[CanisLupus]

2 - Actually the only reason for that change was so that we didn't have to include another onkeydown listener and leverage the existing one - which is only created when the popup is created.

I may be able to use the same one but change the time of registration or register in more than one place. I'll have to think about it a bit. :)

The only thing I'd say should be kept is the name property that was added to the default engines in src/swift-selection-search.ts. That's because the confirmation message that appears when setting a shortcut already assigned to one of those engines would read: This shortcut is already assigned to 'undefined'! Override? if that property is missing.

Ah, my mistake! Thanks for the heads-up, I'll add it back. But perhaps I'll change the class declaration to include "name", or leverage the existing "const sssIcons" object, which contains the names.

I understood your explanation and agree with your changes.

Great! Thanks for sharing your opinion. I'm glad you agree!

You can go ahead and implement them. I'm enjoying contributing so I'm more than willing to help should you need any assistance.

The main limitation is my time, but I will gladly implement these. ;) Thanks! I'll let you know of updates here.

[CanisLupus]

Hey @igorofc!

So, I finished making the changes we talked about. The page-script got the most changes so as to not require a popup to be created. I think it ended up okay. :)

GitHub and pull requests are weird and I'm not too familiar with them, so while I made them to your PR branch, they don't appear here since they went to a new branch with the same name on my repository. To get the desired effect I would probably need to create a PR to your PR, which... sigh... 😫

Besides the changes above, I also edited some of the text that appears in the options menu to the user.

A few things that I found out that I want to do now, outside of this PR (this is all changes to my previous code):

• Simplify the deactivate() function (just deactivate all listeners, regardless of if we registered). It's more trouble than worth to keep it the way it is.
• Rename isInEditableField(). The name is confusing. It checks for the content editable attribute, not if the field is an input field.

There's also a strange behaviour that I haven't checked why is happening, but it's okay and perhaps even desirable anyway, which is that after pressing Tab shortcuts will not close the popup. I think it's because something in the popup is focused, since this also happens for Enter while having the input field focused. Again, don't know why, but it's fine. :p

[CanisLupus]

Commits can be seen here.

[igorofc]

Really nice work, @CanisLupus. It's working great! Thank god we got rid of that ghost popup of mine :)! These changes will help me conform better to the extension's code standards.

GitHub and pull requests are weird and I'm not too familiar with them, so while I made them to your PR branch, they don't appear here since they went to a new branch with the same name on my repository. To get the desired effect I would probably need to create a PR to your PR, which... sigh...

Yeah, I know how you feel. A day shall come when we master this thing once and for all. :crossed_swords:

There's also a strange behaviour that I haven't checked why is happening, but it's okay and perhaps even desirable anyway, which is that after pressing Tab shortcuts will not close the popup. I think it's because something in the popup is focused, since this also happens for Enter while having the input field focused. Again, don't know why, but it's fine. :p

That's weird. Nonetheless, it may not be a big issue as you said. We could check it out later just to be sure of what's causing it.

Thanks for merging this PR. #72 is on the way :)

[CanisLupus]

Thanks! But it's really me that should be thanking you for your work. :)

Yeah, these PRs are hard, but we'll persevere hahah.

I've written down the weird issue. Not important now but hopefully won't be forgotten.

Please take your time. :)

[CanisLupus]

Just letting you know that I (finally) submitted version 3.46.0 for review just now. :)

[igorofc]

Wow, that's great news. Hopefully it will be a well-liked addition to your extension.

[CanisLupus]

It's live now! Hopefully I didn't break anything in SSS with my changes. 😅

Hopefully it will be a well-liked addition to your extension.

Well, I'm using shortcuts already hahah. I'm curious about when we'll receive the first request for shortcuts with modifiers. I think it's fine as it is, though!

[igorofc]

Well, I'm using shortcuts already hahah.

Cool! Shortcuts does makes your life better at times. :) I think the implementation is solid and I've never noticed any bugs.

I'm curious about when we'll receive the first request for shortcuts with modifiers. I think it's fine as it is, though!

Me too. If it is ever requested I'm in for implementing it.

[CanisLupus]

Thanks, Igor! :)