Open funnym0nk3y opened 5 years ago
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! Daniel
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:
The popup shows as usual according to the settings. But instead of pressing a button on the popup I press the key on my mouse.
The popup does not show according to the settings. You can make it show with the associated shortcut but that is optional. Either way when pressing the mouse button the word is search with associated search engine.
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'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 Tab
& Shift-Tab
to cycle between elements and Enter
to "click" them.
(Combos like Shift-Enter
or Ctrl-Enter
, to simulate middle- and right-clicks could be considered as well.)
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! Daniel
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...:
Your comments are clear. Good job. :) Despite that, it's still hard for me to review such a large amount of code just out of the sheer number of changes, especially without knowing if it is final from your perspective. Can you tell me in broad terms what you have and what is missing? I'd really appreciate it.
From the user's point of view, what can they do that's different from before? I understand that you are implementing single-letter shortcuts for each engine, but that wouldn't necessarily cover the original request for mouse buttons, for example, unless you have software that maps buttons to keyboard letters (which you usually don't want because of other apps).
You mentioned the "tab" key and I saw it is used to toggle shortcut functionality on/off while the popup is open. Was this because of the auto focus on the input field when using keyboard shortcuts to open the popup? I originally thought that shortcuts would be automatically disabled if the text box was focused, but yeah, there's the problem of the auto focus in that situation. I think what @jstetten said would probably be a better use for the tab key, and a relatively simple implementation of intuitive "shortcuts", but it can definitely be a separate issue for later.
Is there any way you can separate the changes for #175 from the changes for #149? Or is there a reason that made them be in need of fixing together? Since the two issues look different I would expect two different pull requests (please remember that I will have to look at the changes mostly as a global change, not necessarily individual commits, so it greatly helps to keep different features separate).
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! Daniel
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:
From my perspective, most of the work is done in terms of functionality. Maybe the descriptions of the new settings could be improved, as well as how they should be called. I can't think of anything else that is missing. It's not final - as in eligible to close this issue - because it does not solve the original request (although I would gladly try to if I had a mouse with multiple buttons at hand). As you mentioned, there were many changes, more than I had anticipated (in part because I decided to address issue #175 along with this). In general, it just needs more testing.
The idea is that the user will now be able to assign a single character (not just letters, but numbers and others special characters like @,#,*, etc.) as a shortcut to an engine. There's a checkbox to enable/disable them, as well as another one to enable them even if the popup is not visible (e.g the behaviour is set to anything other than 'Auto'). There's also the option to choose how the search will be done, just like when clicking an icon. The shortcuts are always disabled when the selection occurs in an editable field and are also disabled by default when typing in the popup's text field (although they can be toggled here). I'll describe what I did with issue #175 on that post.
When the popup is open, shortcuts are disabled automatically only if the user types something on the text field as it could become messy when typing characters that are also shortcuts. The 'tab' key is only used to toggle shortcuts in this case. That's because after editing the selected text or typing the new search query after opening an empty popup (#175), it would be useful to be able to use shortcuts again just by pressing one special key. It doesn't have to be the 'tab' key though. Maybe we can come up with another idea for this?
Again, my lack of experience with GitHub is to blame here. Now I understand that I should've created another branch to tackle issue #175. Since I've been working on both features together, the commits and the code itself got mixed so I don't know exactly how I should go about separating the changes. Can you help on this one?
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!
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.
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. ;)
- From my perspective, most of the work is done in terms of functionality. Maybe the descriptions of the new settings could be improved, as well as how they should be called. I can't think of anything else that is missing. It's not final - as in eligible to close this issue - because it does not solve the original request (although I would gladly try to if I had a mouse with multiple buttons at hand). As you mentioned, there were many changes, more than I had anticipated (in part because I decided to address issue #175 along with this). In general, it just needs more testing.
OK, got it. Thanks!
- The idea is that the user will now be able to assign a single character (not just letters, but numbers and others special characters like @,#,*, etc.) as a shortcut to an engine. There's a checkbox to enable/disable them, as well as another one to enable them even if the popup is not visible (e.g the behaviour is set to anything other than 'Auto'). There's also the option to choose how the search will be done, just like when clicking an icon. The shortcuts are always disabled when the selection occurs in an editable field and are also disabled by default when typing in the popup's text field (although they can be toggled here). I'll describe what I did with issue #175 on that post.
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. ;)
- When the popup is open, shortcuts are disabled automatically only if the user types something on the text field as it could become messy when typing characters that are also shortcuts. The 'tab' key is only used to toggle shortcuts in this case. That's because after editing the selected text or typing the new search query after opening an empty popup (#175), it would be useful to be able to use shortcuts again just by pressing one special key. It doesn't have to be the 'tab' key though. Maybe we can come up with another idea for this?
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.
- Again, my lack of experience with GitHub is to blame here. Now I understand that I should've created another branch to tackle issue #175.
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:
Since I've been working on both features together, the commits and the code itself got mixed so I don't know exactly how I should go about separating the changes. Can you help on this one?
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:
First, to make things clearer, create a new branch called "issue-149-shortcuts" (or some other name you'd like) from your "develop" branch. It will point to the the latest commit, just like develop, and we'll leave it there. Further commits related to this feature would go to this new branch.
This is optional, but I think you should be able to reset your "develop" and "master" branches to the same commit as mine so that they contains no changes; only the newly created "issue-149-shortcuts" should have those. My latest commit (both develop and master point there) is still the one with text "v3.44.0;".
I don't use command line Git, but in SmartGit, for example, this is done by being on the develop/master branch, right clicking the commit to reset to, and using "Reset". Normally you'd never do this, since you pushed the commits already and if someone forked your repository it would cause havok when merging their changes (since you are removing commits from a branch), but if you are the only person working on your repo this is okay.
At this point we haven't solved the mixup problem yet, just made an easier-to-manage repository. :) You should have this layout ("origin" branches will still be wrong until these changes are pushed):
The next step would be to actually separate the commits. I would start by creating a new branch from develop or master (so also pointing at my lastest commit) and call it something like "issue-175-empty-selection" (any name is fine ;)).
Now, this part greatly depends on how the changes were made. It may or may not be possible to ask Git to pick the commits that implement feature "X". You can try being on branch "issue-175-empty-selection" and using the "cherry pick" feature to add commits related to the 175 feature, one by one, to the branch. However, I'm guessing that this will be hard and confusing, with conflicts, so I have a different -- and unfortunately more manual -- suggestion:
5.1. In branch issue-149-shortcuts, simply search and delete all code related to 175 and commit that change. 5.2. Checkout issue-175-empty-selection and manually copy the code that you deleted to the right files here.
I apologize if there's an easier way, but this is how I'd do the whole thing, honestly. :p This doesn't "erase" the fact that code from another feature was in issue-149-shortcuts since it's still in the commits, but I'm fine with that if you are too!
Push the changes if every branch is correct. If something goes wrong at any point you can re-download the repository (or keep a copy of the original .git folder and return to that).
I hope this helps!
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!
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: Something like a record button, pressed with the left mouse button, which then records all the buttons pressed afterwards. That way, key combinations can also be created.
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 tab
key can now be used to cycle through the icons. I was actually playing around with this and, as you said, @jstetten's idea is simple to implement. I can try to implement it before pushing the new branch for this issue, but since you said it could be a separate issue for later, I wanted to hear what you think.
I'll explain what I did so far:
I was looking and found that to make the icons focusable we add tabIndex = 0;
to each one of them. To associate the icon with the engine, I set the name
property of the img
tag to hold the searchUrl of the engine. When hitting 'Enter' on an icon we can then retrieve that property and look for the engine that matches that url. It works as expected.
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 tab
and hit Enter
, Turns out it depends on the text being selected and the selection disappears when pressing a key or clicking anywhere, which makes it also unusable if clicking on the popup's text field or when using the 'middle mouse' and 'keyboard-only' opening behaviours.
@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 Enter
on an icon. My opinion is that we could make use of the same setting used for shortcuts. What you think?
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 swift-selection-search.ts
and make page-script
and settings
reference enums like SSS.PopupOpenBehaviour.Auto
instead of just PopupOpenBehaviour.Auto
or Types.PopupOpenBehaviour.Auto
.
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?
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:
A
be different from a
? If so, the shortcut field would probably have to display Shift+A
for upper case and A
for lower case / single key (just A
vs a
is probably not clear enough for the user).Shift + number keys
results in punctuation and special symbols (at least on our keyboards, I don't know about non-latin alphabets). Would it show up as the shift+number
combination or the symbol itself (like %
)? Again, based on Caps Lock, I assume it would show up as Shift+5
instead of %
(on my keyboard) to avoid problems.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?
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.
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. :(
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.
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.
I think it's ready for the PR. Anything you'd like to add?
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.