Closed wiiiiilllllll closed 5 years ago
Ditto for Linux, having the same exact issue.
you're right, i havent written the detection code to dynamically specify the commands per OS..
tasks:
cmd || ctrl
Since it does offer key support, can you let us know what the alternatives are for cmd on windows/linux are?
I've tried all the hotkeys you seem to listen for here but none of them seem to do what cmd suggests, and I don't see you listening for anything besides cmd here.
Guessing I'm misunderstanding something, but would love to actually try out all the features.
Doesn't KeyboardEvent Key values already solve that?
this is what i'm using for hotkeys, i thought it normalized the cmd and windows keys to be the same, but i dont see it in the docs anymore https://github.com/jaywcjlove/hotkeys/
this is what i'm using for hotkeys, i thought it normalized the cmd and windows keys to be the same, but i dont see it in the docs anymore https://github.com/jaywcjlove/hotkeys/
I can verify in the demo https://wangchujiang.com/hotkeys/ that it is, I think the issue is that many windows key combinations are captured by the the os. Windows key + arrow keys is used to move the active window.
It gets mapped to the super key on my Linux install, most of which comminations I use for desktop environment level functions. It should, of course, map to ctrl instead.
ctrl was a key i tried not to use because it conflicted with many system level commands that had to do with arrow keys. sounds like we'll need to hit a sweet spot per platform, and that sweetspot is whether or not to use the system key or the control key. sounds like on linux and windows we should be using control and on mac we keep it command? can a windows/linux user specify the current and then desired set of keys they'd like to see? we can start mapping expectations back into a pattern we can solve this ux request with?
Letting users configure their "cmd" key gets my vote. For a predominantly keyboard driven tool making this a config instead of just coming up with one default per system make a lot of sense.
On Mon, Nov 19, 2018, 11:31 PM Adam Argyle <notifications@github.com wrote:
ctrl was a key i tried not to use because it conflicted with many system level commands that had to do with arrow keys. sounds like we'll need to hit a sweet spot per platform, and that sweetspot is whether or not to use the system key or the control key. sounds like on linux and windows we should be using control and on mac we keep it command? can a windows/linux user specify the current and then desired set of keys they'd like to see? we can start mapping expectations back into a pattern we can solve this ux request with?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/GoogleChromeLabs/ProjectVisBug/issues/194#issuecomment-439986125, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG5Ej-LG6LqyVm5bRHEIUJf_0whWAD1ks5uwvH-gaJpZM4Ydv7y .
great idea, would go well with #70 👍
I think the idea of making the hotkeys user configurable is a good one; yet I feel the U/X is better if the default keybindings per system are sensible defaults. Users would expect to be able utilize all the features of the tool without needing to configure their settings first. Maybe swap out cmd for ctrl on non-mac first and then later back in to letting the user customize it?
To that end I started working on a patch to detect o/s and change the keybindings. Unfortunately I could only put about 6-7 hours into it last weekend and spent about half of that trying to get the code to build on Windows 🤣. It's still very WIP but what I've got so far is here https://github.com/dgeare/ProjectVisBug/tree/user_key_prefs
If someone wanted to look at what I've got so far I'd appreciate any feedback. Be brutal, I'm new to open source and I'm sure I've a lot to learn. Honestly, I'm not too hot on the content script message passing; I feel as though there should be a more formalized/reusable structure to it but that felt out of scope.
@dgeare there's a lot of good work and thinking/testing in that branch, thanks for sharing! i agree that sensible defaults are most desirable, and customization should be an escape hatch. i also agree that how we store the user preference, retrieve it, and get the values to the various functions that need it is a larger scope task that we need to tackle before allowing customization. task #70 is quickly becoming the top priority task, it unblocks many other customization tasks/features/desires folks are looking for right now. i'll be focusing some efforts there once the marketing website and firefox extensions are good.
i'm on linux and windows today debugging, checking out branches/forks, and trying to figure out how to best serve linux/windows users on their keyboard.
def looking like using the metaKey (windows/linux home keys) wont work. osx uses the control key for 1 unique case, to hide the handles/guides/labels temporarily so you can see your work without the overlays. we'll want to change that to another key, and then reserve control as the substitute on windows/linux.
was really hoping i could fork hotkeys-js and just teach it to the do the swap for me, but i dont think that's the best path anymore (though a fork of hotkeys is imminent).
abstracting cmd vs ctrl is a good excuse to refactor some of this work as well, and move it to a utility with a nice API https://github.com/GoogleChromeLabs/ProjectVisBug/blob/master/app/features/margin.js#L13
I personally wouldn't mind setting up my own cmd + opt key on Linux. It would be first we click on Visbug icon, then OS detected, a window jumps out saying: You're using Linux, would you like to set up your cmd and opt key? Maybe can write some recommendation of the key?
Otherwise perhaps what you said above could be good. So far it's a nice tool and I admire your great work. :)
@argyleink I also had the same thought when I saw line 13 and felt it could use an abstraction. Using string interpolation felt very dissatisfying. I thought about it briefly but nothing exciting came to mind; Did you have any thoughts on the pattern for the API? I think I should have some time this weekend to play with it either way.
@dgeare a pattern never really leapt out at me.. there's only a little consistency, so the api of the abstraction would need config options, and at that point i'm not sure how valuable it is. tho, being a testable function would be good. so maybe a model driven api, where we could pass a clear config and get strings back? thoughts?
also all, i've updated the tasks comment above with a more helpful list. let me know thoughts or questions!
A few thoughts:
@argyleink after playing a bit what I came up with was a chai-esque chainable api that looks something like:
keys(opts = {}).shift.plus.up.string()//the initial keys call taking an options object so it can be flexible in the future
It uses a map behind the scenes to map between the app language and the eventual environment/user preference keybindings. Idea being when the app initializes it can override the defaults in the keyMap and that becomes transparent to the rest of the code. You can see what I've got so far at
https://github.com/dgeare/ProjectVisBug/tree/refactor_keys_api
I only refactored the margin.js file so far just in case you hate it, but to still show what it might look like. This pattern felt readable and comfortable to use for me, but that might be my own lack of experience and a true understanding of what a proper model-driven api looks and feels like 🤣.
Some notes: I'm reconsidering renaming "plus" and "comma" since those are being used in a literal sense and not representing their key-equivalent. Maybe "and" and "join". And I'm not sure if I like the keys().shift.string()
or `${keys().shift}`
syntax better in the refactored code (both work).
If you hate it let me know, I'm fine with that. I'd be happy to learn a better alternative. If you don't hate it, I can try to refactor the rest over the next week or so.
@dgeare I certainly don't hate it! i think it's clever to use some chai syntax, it's a fun interface! i'm glad you challenged the api and made something. please never stop!👍 all projects need many perspectives imo, i want your influence.
there is a looming feeling for me though that your suggested interface isn't that much easier or better (struggling to find the wording here, better is a terrible word). it feels like a different way to do it, that's not necessarily empowering the workflow or reducing complexity. that fair to say? what specifically brought you joy/ease in this refactor?
there's other code you've written in there with package.json and the zip npm command. are you open to making a PR of just that work and explaining why/what you did?
@argyleink No, I definitely understand what you mean, and I think it's fair to say. How I arrived at this solution:
If the end goal is to allow the keys to be different for different environments (and possibly user preference, someday) then the actual key being bound to needed to be abstracted from the features, etc. I personally prefer to avoid using strings when what is meant is a semantic element (when using "cmd" the code doesn't want to think about it as a string but as the cmd key). So my inclination was to abstract the keys into a map (which is essentially the keyMap used behind keys) but that would leave the rest of the code reading const command_events = `${keyMap.cmd}+${keyMap.up},${keyMap.cmd}+${keyMap.shift}+${keyMap.up}....`
which I felt was a little verbose and unpleasant to use.
The only other options I saw for abstracting the keybindings were keys("cmd")
which looks very much like previous in usage, but still has all those strings running about. Or keys("cmd+up,cmd+shift+up")
which would return a string with the proper substitutions, but would need to rely on some regular expression magic or string parsing that would probably not account for all possible use cases, leaving it open to bugs or general lack of flexibility in the future. I briefly considered trying to abstract the entire sequences of keys... but the features seemed too opinionated for that to be a viable approach.
The proposed solution I felt was more comfortable because it gained a bit in brevity, while retaining flexibility and mostly maintaining readability. Plus the ability to return a string or array felt nice for things like keys().up.down.left.right.array().forEach(side => {...
So those were my thoughts, and how I arrived where I did. If you have any ideas on a different pattern I would be excited to hear it because that would mean I get to learn something new! 😁 Or if you have thoughts on why one of the others listed would actually be preferable that's fine too, I'm not tied to the one I've got. The big win is just in being able to replace one key for any other (right now we're mostly concerned with cmd <=> crtl, but there was interest expressed in further customization with user preferences) and have that be transparent to the rest of the code.
And yeah, I can certainly put in a PR for the changes to the NPM scripts. Mostly just what I had to do to get the build to work on a Windows box. Even using bash on Windows there were cross-platform issues with npm vars ($ vs %) and things. Something about npm using cmd.exe under the hood ¯\_(ツ)_/¯
.
yo @dgeare thanks for that PR! love it! you're helping create great conversations, thank you.
i just made a PR that fixes Linux (and i assume windows), but doesnt refactor how the strings are made for hotkeys. that refactor task will still be open for anyone who wants to take a stab at cleaning up the event string creation for design features.
how's it workin for y'all? Linux and Windows should now be able to use ctrl where cmd was, and all features/tools use ctrl now instead too!
Hi again. I just tested Margin and Padding.
@AnnieTaylorCHEN Are you testing the actual extension and/or master branch? If so it hasn't been merged yet. The PR is here https://github.com/GoogleChromeLabs/ProjectVisBug/pull/250 so you can see when it comes in.
@AnnieTaylorCHEN thanks for testing! i found that pinning bug too, logged it here #251 I'm curious like dgeare is, did you pull the PR down and then test? the solution isn't deployed via chrome extensions yet 👍
@AnnieTaylorCHEN Are you testing the actual extension and/or master branch? If so it hasn't been merged yet. The PR is here #250 so you can see when it comes in.
Ah sorry guys, I thought it was merged. I didn't test the PR but the app (guess still the old verison. ) :">
np! easy peasy thing to do!
i'm also going to do the work to show properly alt or opt in this PR/ticket
@argyleink Thanks for getting this underway. I appreciate it. I'm excited to be able to show it off to my team soon. I'm still game to refactor the key events strings into an API if we can come up with an amiable pattern. I'll have to reach out to you on gitter sometime later to get your thoughts.
sounds good dgeare! i can tell from your code you're most certainly capable! totally down to chat!
also curious what you think of the solution i made in that PR!
I think it looks solid! I think we agree a pattern for the command_keys would be more desirable, but I think the efficiency of this is keen; don't want to hold up adoption of the extension for it.
I honestly didn't know that navigator.platform was a thing ;>.>, for some reason I'd figured that information would be protected by the browser and only divulged given some indication of user trust (installing the extension). This seems to be a much better approach than I was playing with since it can be retrieved synchronously and stored in a constant. It's good work!
(P.S. I appreciate the vote of confidence ^_^)
Some of the UI tooltips mention a "cmd" key, which doesn't exist in Windows. Commands which use the cmd key don't seem to work when using any of the common Windows modifier keys. I tried ctrl, alt, fn, and the Windows key. Ctrl is usually the one.
If VisBug does have alternative Windows bindings, the UI tooltips should display them for Windows users. If the alternatives don't exist, then VisBug is partially broken on Windows!
Apologies if I've missed something here.