search

mathieudutour / scroll-through-time

:hourglass: :tophat: :rabbit2: Two fingers scroll moves through time instead of space
https://xkcd.com/1806/
MIT License
721 stars 54 forks source link

Add initial support for keyboard modifier (and desktop support) #13

Closed mallardduck closed 6 years ago

mallardduck commented 6 years ago

So I had read about this package for Atom somewhere and was really excited to give it a shot. However I mostly use a desktop workstation and realized it won't work here. So when I saw #4 and #11 I realized that adding support for that could probably solve my problem too.

With this patch it's now possible to use keyboard modifies with the mouse scroll to scroll thru time.

Currently this only supports using Ctrl+Alt+Scroll, however it can likely be adapted to use any other key modifier via config. Just my first time dabbling in Atom packages so I'm not 100% on how to do that part yet. :smile:

mallardduck commented 6 years ago

Welp, actually after further testing I just realized that my toggle for keyboard modifiers doesn't seem to work at all. I had a really rough version of everything worked out a bit ago that didn't have this bug, but after refactoring somethings to use more of the original code I ended up with this.

Guess I'll give it another go and report back!

mallardduck commented 6 years ago

Ok, now it should, in theory, play nice! No promises that it won't eat your cat, but I've got things as ironed out as I can now. After some testing I realized that anything involving shift scroll is gonna just do horizontal; and anything control scroll is just gonna adjust zoom.

I've updated the key to be just Alt. While this does still allow the page to be scrolled as you scroll through time, atom will usually keep you around the same spot. As mentioned in the second commit message, this does work now however there still feels like room for improvement.

This is complete speculation and spit-balling but...maybe provide an option for operation mode; Keyboard+Mouse, or TrackPad(w/o or w/ hotkey). Then when operating in keyboard mode the listener will be attached to the keydown and keyup events; that way you can prevent the default key actions? (taking over the zooming, or side-scroll the keys cause now?) Then when those events fire you set and unset, respectively, the scroll listener.

mallardduck commented 6 years ago

Aye. Found a way to fix #5.

Also didn't realize it but I totally fixed #6 too as I was having a similar issue with the toggle I added. So in the process of doing this up I also fixed that and updated the name/description of that option. So it looks like in total this PR could help get rid of a bunch of these open issues. :tada:

4, #5, #6, #9, #10 (partially!), and #11 (since it's a repeat).

mallardduck commented 6 years ago

Ok, so what I could whip up doesn't seem to play nice if we go the direction you suggested.

It's all very complex and I only have enough of a grasp to know it won't work. (at least the way I've tried) So bear with me while I try to put it into words. So we'll start with the scroll handler being used. Like what was pointed out in #5 the window still technically scrolls when you use this as is. So I adjusted the eventlistener call and invoked stopImmediatePropagation to prevent that.

While that seemed to have fixed the scrolling in the window as you scroll thru time, this didn't actually solve for the issue of preventing the default actions of Ctrl(zoom) and Alt(side scroll) when used with scroll. So to take that on we will want to throw in a preventDefault; this effectively prevents the stock scroll modifiers from kicking in.

In doing this though you end up breaking scroll everywhere in atom. So if you have scroll thru time on for your editor, you can't use scroll in your settings, etc. Now why's that though? Well as issue #9 indicates it's cause the package is active everywhere in atom and that's cause events are bound to the document instead of the specific editor element. While I claimed this PR will fix #9 what I realize is that it only fixes the symptom of it since having a modifier key would allow scroll to work in tree view without enabling time scroll.

So next I looked into doing just that, attaching the eventlistener to the editor only. At first I found success and thought it could get things on the right track, but then I realized you really only want this active in the current editor window. So then that requires even more logic track the window the user is on; remove listeners from the previous window and then add them to the new one.

So to summarize my points:

  1. Any scroll listener that will use a modifier key requires to preventDevault and stopImmediatePropagation. (otherwise you're gonna be zooming or scrolling the page)
  2. Doing this with the current (omnipresent, doc level listener) will prevent scroll in every area of atom.
  3. Scoping the listener to just editor windows is complex; requires moving listeners as users move, or just applying it to all editor tabs (and then as more are opened. Even if it fixes some issues it clearly adds more. (also seems to make it impossible to reliably remove listeners)
mallardduck commented 6 years ago

Ok. So my last comment can be, more or less, ignored.

I still don't exactly like that the scroll handler applies to the whole Atom window, but now I'm just checking for the scroll context by making sure it has a parent of the atom-editor. Meaning this PR does still fix #5 and #9. (and the others I mentioned still seem fixed too!)

mallardduck commented 6 years ago

No problem at all! Glad to help out with such a neat package. It was also a great experience learning about how atom packages work. Plus helping squash bugs/issues is always fun!

:tada: