-
Notifications
You must be signed in to change notification settings - Fork 27
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
Shortcut for search engine #149
Comments
You are welcome! ;) I haven't had this suggestion before so let me ask: would this be a shortcut after opening the popup or without opening it at all? Cheers! |
It would be a shortcut defined in the settings tab "search engines " for each engine. When marking for example a word there are two possibilities:
|
Hmm I see. So it would either work with the popup open or closed (or always), depending on preferences. Thanks! |
I'd like to suggest that the user may assign a letter as a shortcut. For example: Assigning the letter "g" for the google search engine. Then the user selects some text and press that letter to do a google search. |
I'm trying to implement my suggestion here. I think it's working fine but I'm still testing it. Do I have to transpile the Javascript back to TypeScript when it's ready? If yes, how do I do that. |
Hi Igor, thanks for checking this issue out. SSS code is written in TypeScript, so this would have to be implemented directly in TypeScript. There is no useful way to transpile JS code back to TypeScript as it loses type information (and formatting), so you should never edit the transpiled JS code (it just exists so the addon can actually run in the browser). I hope this helps! |
Thanks. I'm new to this so I edited the js code instead. I don't know anything about typescript but I'll try to convert what I've done to it. Hopefully it shouldn't be too hard. |
It usually isn't too hard if you don't want the type advantages. :) Most (all?) valid JS code is also valid TypeScript code. If you need help with something let me know. Thanks! |
I pulled it off quite quickly. Fortunately I hadn't written too many lines on the js code before asking. |
Hi @CanisLupus. Nice addon! I'd like to propose what I think #203 was asking for: a way to navigate the elements of the pop-up with the keyboard. The most straightforward way would be to follow the convention of every GUI of using (Combos like I think this would be natural to almost all addon users right away. What do you think? |
Hi @jstetten, I hadn't thought of using Tab (and variants). Could be interesting! It's something that I feel can exist independently of the request for specific shortcuts per engine. As usual I can think of a few possible problems and conflicts with other settings, but overall it feels like it could be added without a setting (first tab selects the text box, next tabs select the engines). I could be wrong, though. I haven't been working much on SSS recently due to other priorities, so I will probably take a while to look at this! (I keep repeating myself, but it's better than giving false expectations of timelines.) By the way, @igorofc, please let me know if you need anything. :) I'll try to find some time for it. I'm not sure how friendly the repository is to "other-than-me programmers", since it was mostly me all the way through and I'm not even a web programmer. :p Cheers! |
Hi, @CanisLupus. I'm making progress here. It would be nice if you could take a look and let me know how you like it. I'm commiting to the 'develop' branch in my fork. I've also been trying to implement the suggestion at issue #175. It would be nice if more people could test it. By the way, currently I've set the tab key to toggle shortcuts when the user is typing in the popup's text field. So it would probably conflict with @jstetten's idea. We can always find alternatives though. |
Hey @igorofc, first of all let me thank you for handling this issue and 175. :) I see you are at 63 commits so it seems like it was a lot of work already! Since there is no pull request yet and you are still working on it, I'm looking at the changes via this link. Normally, we would have talked sooner and I should probably have a "how can I contribute" section in the readme for SSS anyway. Something for me to fix when I have more time. Usually, we'd talk about what the implementation would be and then actually do it, but since we skipped that part, let's see...:
I'm sure there will be more questions later, but for now this is what I was thinking. Lastly, I see that in implementing this you have made yourself familiar with many parts of SSS, and did all of it without asking for any help, which makes me very curious. I'd like to hear from you what parts of SSS's code could use more explaining, what you found more challenging to understand, etc. Feedback in general, if you can spare the time, of course! Thanks! |
Hey. I apologize for not discussing how these features should be implemented in the first place. Even though I've done a lot of coding in the last few years, it's always been just solo projects. This is actually the first time I'm contributing to anything. I'm still getting the hang of GitHub, collaboration and all. So I shouldn't have rushed to do it the way I thought was right without asking for suggestions first. Sorry for that. That being said, I'll try to answer you questions the best way I can:
I didn't ask for help while doing this because I really didn't need to. I'm not the best of judges but your code is very well written and organized. The comments on 'settings.ts' explaining how to add a new setting is pretty straightforward so it's just a matter of researching and working on the new feature. As I've only dealt with a relatively small spectrum of the code I can't give a broader feedback. But for the parts I came in contact with it's all good. You put a lot of effort there. Congrats! |
Don't feel bad for not talking about it. It was not the intention of my previous post. :) I should have dealt with this sooner but unfortunately I haven't had much time for SSS. Although I use Git in my full time job and personal projects, SSS is the only relevant public project I have, and I have had very few pull requests, so I'm definitely not used to GitHub collaboration either! So, in summary, don't worry. We're all learning. ;)
OK, got it. Thanks!
Thanks for explaining this as well. I had the chance to try your code's version of SSS on my Firefox install so I already got a much better idea of how it works. I haven't tried everything yet, nor seen your changes to page-script.ts, but it looks great! Even with some things to solve, such as the original request, this looks like SSS alright. ;)
I believe that the "problem" we face is that whenever the user has focus on a text field (be it SSS's or a webpage's) the shortcuts shouldn't work, and I agree with that. If we implemented "tab" as a "move focus away from the text field and into the first engine" (and then to other engines; i.e. jstetten's suggestion), the text wouldn't be focused anymore, so shortcuts would automatically work again and it would also solve the problem. What I mean is: maybe we don't need special code for tab to "enable shortcuts" but only to look at the current focus to determine if they should work (sorry if you do this already, I haven't looked at the main part of the code yet!) and then in the future implement jstetten's suggestion, which would use tab. I may be missing something, of course.
Yep, the best way to avoid problems is to create a branch (from "master" or "develop") and commit all changes to that branch. This makes it easier to merge it in the end, since ideally all commits relate to the same thing, and also makes it possible to pause work on the feature and start working on some other feature in a separate branch. I tend to NOT do this when I'm working by myself on SSS (I commit to "develop" directly) but that's something I should improve, especially if other people like yourself would like to contribute. :) Finally, I should add to the repository's README some info about SSS's 3 main branches:
Sure! I'm not a Git expert in any way but I'll try. I'm not sure how used to Git you are, but please try the following. You can do this entirely offline and check if everything is fine in the end, before pushing the changes:
I hope this helps!
Thanks so much! It's great to know that the general code is easy to follow. :) I remember adding those instructions for adding a new setting because even I tended to forget a step or two. PS: Sadly, I didn't know the function "confirm" existed in JavaScript and I'm very amused by that fact. XD When I saw it in your code for confirming the shortcut override I was thinking "no, it can't be... :O". Thanks for that hahah. I'm looking at the way I implemented reset buttons (where a second button shows up below the first to confirm, in case the user misclicked) and shaking my head now. I'll simplify that soon(ish). |
Thanks for helping me out! I'll follow your instructions and revert back to match your repo. Then I'll push just one commit with all the changes up to now for each of the issues to their respective newly created branches - keeping them separated. I'm working by myself here so that won't be a problem. Oh, you're completely right about the 'tab' key not having to be bound to toggle shortcuts. There's no need for this at all. I'll fix this when I'm done cleaning up my mess :). Thanks again for sparing the time. |
You're welcome, thank YOU for your collaboration. :) As for the request possibly including mouse buttons for shortcuts, you don't have a way to test mouse buttons and I can definitely take a look at that part afterwards, so perhaps we can keep that aside for now. Just keeping my thoughts here: I guess we can't use mouse buttons 0, 1 and 2 since they have common functions, but we could use the rest. We could, for example, allow the user to click on the shortcut text field with the desired mouse button to set it to that. This is probably very not-obvious for the user without text, though. Back/forward buttons on a mouse will generally not even produce a mouse event (I think) since the browser catches that first, but that's okay. We also can't do anything if the user has changed a mouse button to mimic any other key. So, it would probably only be possible to use totally unassigned mouse buttons in SSS. |
If tests are needed, I can try it with my mouse. But here is an idea for setting the shortcut: |
Hey @CanisLupus. Indeed, there are many details we need to be aware of when implementing mouse buttons as shortcuts. Hopefully it can be done. So I removed the toggling feature so that the I'll explain what I did so far: I was looking and found that to make the icons focusable we add Let me know If you have any ideas for this and whether I should continue or save that for later. |
Hi @igorofc, I think you can add this to the same branch if you'd like to. I said it could be separate because the key-based shortcuts don't depend on this, but it's all still about shortcuts so we can make an exception. ;) I was thinking of using tabIndex as well, if possible. As for changing the name property, that would depend on what information you have when you press enter. The index of the icon could be enough to get the associated engine object before creating the message for the background script, but I'm not aware of the details. You can implement it as best as you can and we'll discuss/change the details in the pull request (even if it doesn't have support for mouse buttons). I'll have a few pointers on code formatting for SSS too (tabs, no trailing whitespace) but it's minor stuff that's expected. :) I have some time today, so I'll try to improve the README if I can. @funnym0nk3y Thanks for being open to testing. I think I'd rather stay away from sequences of inputs to keep this on the simple side, especially for the user who has to create them. ;) Key combinations (ex.: Ctrl+Shift and a key) usually make more sense but those have compatibility problems with browser shortcuts, other extensions, and even your OS input language (which may input characters with Ctrl+Alt). |
Hi @CanisLupus. While testing today I noticed that 'Copy to Clipboard' wasn't working when I focused it with |
@igorofc That's a nice catch, I hadn't noticed this (I basically don't use the copy button :p). I can confirm it happens here for the case where we focus/click the text field and for the keyboard-only behaviour, which also focuses the text field immediately if it exists. However, I use the middle mouse opening behaviour myself and that one seems to work since the selection is not lost. Are we perhaps talking about different things? Or are you seeing something different on your end? |
Oh, I'm using Linux Mint and I just found out that the middle mouse click is used to paste whatever text was just selected. That's certainly why the selection disappears here. Anyway, this is a matter for another issue. By the way, what do we do regarding how the search should be done when hitting |
Ah, mystery solved! I agree. In the worst case someone will come up with a well-justified reason for having separate settings. In that case we could add another setting later, if it makes sense. ;) |
By the way, I made a commit to develop that changes enum declarations to const, since that makes TypeScript copy the actual enum string values to every place where they are used in JS, instead of accessing an object that WebExtensions will "break" because of script sandboxing. This made it possible to delete all duplicated enum declarations in the page script and settings script, which is something that has always bothered me. :) All scripts now simply reference the enums in namespace SSS (declared in the swift-selection-search script). I'm warning you because it might lead to some conflicts and changes in your code when merging to/from develop. In general, after merging just declare new enum values in |
Alright. Thanks for the heads up! |
Hi @CanisLupus. There's this unexpected behaviour I'm not sure how we should handle. The problem is that alpha/numeric shortcuts (A-Z 0-9) fires when combined with modifiers (Alt, Ctrl, Shift). So if I set the letter 'C' as a shortcut, then select some text and press Ctrl + C to copy it, the search would take place. I think disabling modifiers altogether when setting a shortcut would not be good since it would limit the options (no special characters would be allowed). My idea is that when a shortcut is pressed we could check if it is alpha/numeric and, if yes, do nothing in case it was pressed in conjunction with a modifier. Any ideas? |
Never mind. This approach would fail because in my mind I was thinking only about roman characters, but there's a whole range of different languages to check. Maybe an easier solution would be to do nothing when any modifier except 'Shift' was pressed along with the shortcut. Since you can't set a shortcut with Ctr, Alt or Meta anyway that wouldn't be a problem. The next step would be to save when 'Shift' is used to set the shortcut. So if the user (for whatever reason) presses 'Shift' when assigning the letter 'C' as a shortcut, this shortcut would only work if pressing 'Shift + C', even though only 'C' appears in the shortcut text field. I hope you understood that :) |
Hi @igorofc, I think you're on the right track. :) I also think the best way is to check if any modifier key is pressed and, if so, ignore the key. Although I would also ignore it if shift is pressed, at least until (if) we support other combinations. I can see some possible user confusion arriving from allowing only shift. ;) But let me know what you think:
So, some things to consider. I'm not sure if you wanted to support shift right now simply because users might use it when inserting the shortcut, or if you had other reasons as well. |
By the way, a heads up: I'll be away next week, so I probably won't be able to reply until September begins. :) |
I think you're right about not supporting modifiers right now. I didn't count, but when we think about it there are a lot of different shortcuts available with only letters, numbers and the other characters that don't require modifiers (do users really need more than that?). So no modifiers then. I suggest that shortcuts should be case insensitive. They always appear uppercase in the shortcut field, but that's just for stylistic purposes. This way we keep the field shorter and the settings page clean. |
Back! Alright, if you agree then I think that's the best for now. :) The upper case style seems fine as well. In the worst case, supporting mouse button shortcuts could mean that we'd have to move the text field to the "expandable" settings that open when an engine is selected in the table, since if the field says "Mouse 4" it doesn't fit in a single letter. |
Hey @CanisLupus. I just read what you said on another issue about some behaviours not working with the new imported engines. That's a bummer. Terrible move by Mozilla. Hopefully we can sort it out soon. Hiding the text field in the expandable area is a great idea. The settings page would become even cleaner. If we want to keep it there, though, maybe we could apply a different style to it to indicate it's a mouse shortcut and include just the number of the button. I think it's ready for the PR. Anything you'd like to add? |
Sadly, WebExtensions APIs are generally much more restrictive than the old Firefox-specific APIs. This is one of those cases where we'll have to make do with not even knowing the URL of the search engine. :(
That's also a possibility! :) We can always change this later if needed. The most important thing is that settings are saved in a format that doesn't limit future changes too much.
Thanks, Igor. I really appreciate your collaboration. :) I don't have anything else to add. I assume I'll have more to say after the PR, but that's expected! |
Hey folks, just letting you know that the single-key shortcuts feature was implemented by Igor for the just released version 3.46.0. :) (Although if anything breaks please blame me because I made changes on top of that.) @funnym0nk3y I should note that your original request for mouse buttons is still not solved, so this issue will remain open. I'm unsure of the best solution for that right now, but it's not forgotten. @jstetten As per your suggestion, Igor also implemented Tab/Shift+Tab to navigate the icons (followed by Enter to search). ;) It doesn't do anything different when using shift or ctrl with enter since it respects the same opening behaviour that is used for the single-key shortcuts. |
First of all: Great extension! Thank you!
I have a suggestion for a better usability:
Allow to assign a shortcut for each search engine. I'm using a gaming mouse with a few spare buttons. Would be nice to assign "search with abc for xyz" to one of those buttons.
The text was updated successfully, but these errors were encountered: