Skip to content
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

Scroll patch with mouse wheel or trackpad #1412

Merged
merged 7 commits into from
Aug 29, 2018

Conversation

brusherru
Copy link
Contributor

It will close #1388

@brusherru brusherru self-assigned this Aug 17, 2018
@brusherru brusherru requested a review from a team August 17, 2018 14:13
@brusherru brusherru force-pushed the feat-1388-pan-patch-on-scroll branch from 159d306 to 6ee7419 Compare August 17, 2018 14:52
@brusherru brusherru added the wip label Aug 17, 2018
@brusherru brusherru force-pushed the feat-1388-pan-patch-on-scroll branch 2 times, most recently from 4234fda to fc6367b Compare August 20, 2018 17:27
@brusherru
Copy link
Contributor Author

@nkrkv fixed these issues:

  • invert scrolling
  • shift key for horizontal scrolling (Windows has the same behavior as a Linux, so I made an exception only for macosx)
  • Fix twitching of Patch on doing something before PATCH_UPDATE_OFFSET was dispatched
  • normalized scroll speed for all browsers/platforms

I did all these things in separate commits, you can check it out separately. Then I'll squash it all in the one commit.

@brusherru brusherru removed the wip label Aug 20, 2018
// Only MacOS works fine. So we have to do a hack for proper horizontal
// scrolling with pressed Shift key on all other OS: just switch X and Y.
const wheel =
detector.os.name !== 'macosx' && event.shiftKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one feels fragile nevertheless. Can we go on without relying on OS name? For example, the following predicate looks pretty equivalent to me:

event.shiftKey && normalizedWheel.pixelX === 0

In contrast to the current conditional, this will trigger a swap on macOS when a user hold down the Shift key and somehow scrolled strictly horizontally?! Is it ever possible? If so, what’s in the head of this xoder who holds the Shift key while scrolling with a gesture?

Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙄 now it’s totally broken

My desktop version no longer scrolls at all. The welcoming comment is not centered on startup.

Scrolling in Firefox browser is several times faster than native. Feels very counter-intuitive.

@brusherru brusherru force-pushed the feat-1388-pan-patch-on-scroll branch from fc6367b to aca8caf Compare August 21, 2018 12:14
@brusherru
Copy link
Contributor Author

@nkrkv check it out, please

@nkrkv nkrkv mentioned this pull request Aug 22, 2018
3 tasks
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it’s pretty usable 👍

However, on Firefox I get more coarse scrolling than on Desktop and Chrome (~1.9x faster). Not a big deal and we can leave it as is I think.

The feature should be confirmed to work fine on Windows and ThinkPad. Would you check Windows? And I forgot my ThinkPad, again, today 🙈

@nkrkv
Copy link
Member

nkrkv commented Aug 23, 2018

Tested it on ThinkPad with a touchpad and trackpoint. Both work fine 👍

Let’s wait for feedback from @joepie91 if he has time for testing until the end of this week. If not, let’s merge and open a new issue if something is wrong.

@joepie91
Copy link

I've been working with it a bit on my desktop today (ie. with a regular mouse), and there it worked fine. I'll probably have time to test it on my laptop tomorrow, and I'll report back then.

I do keep running into weird intermittent scrolling issues with the project explorer and inspector; I'm up to three different mystery issues now. The specific issues I've been running into:

  1. No scrolling at all in the Inspector pane (Inspector scrolling is broken #1423)
  2. When scrolling rapidly and nearing the end of the content in the project explorer, it suddenly jumps to the end, skipping a part of the content. Same for scrolling towards the start.
  3. When clicking an item when the content exceeds the project browser pane size, the pane jumps to a different scroll position.

Annoyingly, none of these issues are reliably reproducible, and I have no idea whether they're a consequence of the patch panning or a totally unrelated issue (eg. a wonky pane scrolling implementation). I don't think they should be a blocker, unless you have a reason to believe that it might be related to patch panning.

@brusherru
Copy link
Contributor Author

@joepie91 thanks for the reports and test on the desktop. Will you test it on your Thinkpad?

About project browser and inspector — we'll inspect it and try to fix with another pull request. Looks like the problem is in the "custom scroll" component 🤔

@nkrkv
Copy link
Member

nkrkv commented Aug 28, 2018

@brusherru I think we should rebase & merge this PR. In case of any flaws, a new issue might be open.

@joepie91
Copy link

Apologies for the delay :)

I've tested it on my laptop for a bit, and the scrolling works fine. It does get a bit laggy/jittery when I scroll quickly; I'm not sure whether that's due to the scrolling code, or whether it's just a general performance issue. Looking at my process manager, it starts eating a core when that happens.

@joepie91
Copy link

Hmm, another issue: if you pan too far away from the contents, it can be difficult to find the contents again, given that it's an infinite canvas and there doesn't seem to be a 'return to center' button or anything. I guess that's a broader panning UX issue though, and not specific to this PR.

@nkrkv
Copy link
Member

nkrkv commented Aug 28, 2018

Thanks for testing!

To find the origin press HOME or Ctrl+HOME. These commands are also available via the View menu.

@brusherru brusherru force-pushed the feat-1388-pan-patch-on-scroll branch from aca8caf to 342a07f Compare August 29, 2018 10:08
@brusherru brusherru merged commit d184ae6 into master Aug 29, 2018
@brusherru brusherru deleted the feat-1388-pan-patch-on-scroll branch August 29, 2018 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pan patch on scroll events
3 participants