streamlink / streamlink-twitch-gui

A multi platform Twitch.tv browser for Streamlink
https://streamlink.github.io/streamlink-twitch-gui/
MIT License
2.66k stars 201 forks source link

scrolling stops working on windows 10 #196

Closed CosmicToast closed 8 years ago

CosmicToast commented 8 years ago

I've had the application open for ~4 days now (in tray). Scrolling in any view (including settings) became impossible using the scroll wheel. Restarting the application restored functionality.

I have a hunch this is related to smooth scrolling (and it would be nice to be able to disable it in settings), but can't offer any more useful diagnostic, unfortunately.

bastimeyer commented 8 years ago

Yes, this could probably be a smoothscroll issue. The smoothscroll library is listening for DOM mutation events, so maybe there's an issue with that and Ember's rendering engine Glimmer.

Right now, the lib is directly added into the repo because there was no npm/bower package available when I implemented it, but I've just seen that there is one available now since a couple of weeks. Maybe the latest version does have any bugfixes, but first I need to see if it's even compatible with my modified version. I'm not sure, though...

If this issue is not related to the smoothscroll lib, then it must either be NW.js or your OS (Win10). It would be interesting to know if scroll events are still being fired in the DOM when using the mouse wheel.

bastimeyer commented 8 years ago

I don't think I want to use the bower package... Too many bugfixes and app-specific changes have been made. I don't want to replace this with a version that I'm unable to modify without forking the whole project.

So yeah, I can't fix this bug without being able to reproduce it. :unamused:

CosmicToast commented 8 years ago

Maybe consider submitting bugfixes upsteam? App-specific changes-wise, not sure what to do. Still, could it be possible to not-load it in based on some setting?

bastimeyer commented 8 years ago

Maybe consider submitting bugfixes upsteam?

The whole lib is actually written very poorly. The things I fixed have been changed upstream in a different way, so there is no need for me to submit those changes. There's a bunch of other stuff which I haven't touched yet that should be rewritten though.

Still, could it be possible to not-load it in based on some setting?

Sure, with a little bit of extra work, but this is actually more of a design decision and I think that smooth scrolling should be enabled at all times. Disabling this feature because of a bug is not a solution IMO. Like I've said, I need to know how to reproduce it, so I can fix it.

I know this is a critial bug when it occurs, but it hasn't for me in the past two years. And restarting the app is not that big of a deal, especially if you are running it for more than a couple of days. There might even be several memory leaks within NWjs, the used frameworks or the app itself.

CosmicToast commented 8 years ago

I disagree that smooth scrolling should be enabled at all times - personally I find the "feature" (popping up in more and more places) to be fairly annoying. It doesn't matter (much) here, since my follow list rarely overflows to the point that I have to scroll... Is there a particular reason you think it should always be on, regardless of what a user may want?

bastimeyer commented 8 years ago

Is there a particular reason you think it should always be on

I think that the default scrolling (especially on Windows) is terrible and simply not modern. It feels clunky and doesn't look good.

regardless of what a user may want

No, but giving the user control over everything makes it much more complex and harder to maintain. The next step would be a checkbox for disabling animations, the one after that would be about the placement of the window buttons at the top-right corner, and so on...

If there is a huge demand for a setting like this, then I will implement it of course, but I don't think this will be the case.

CosmicToast commented 8 years ago

I think that the default scrolling (especially on Windows) is terrible and simply not modern. It feels clunky and doesn't look good.

How it feels and how it looks is subjective, and viable to vary from person to person.

The next step would be a checkbox for disabling animations, the one after that would be about the placement of the window buttons at the top-right corner, and so on...

Not really. While disabling animations is a possibility, placements of the window buttons at the top right corner is an extreme hypothetical - they are standard per platform, unlike animations, "smooth scrolling" etc.

No, but giving the user control over everything makes it much more complex and harder to maintain.

I'd be fine with a compile-time configuration. It isn't much of a deal to set a function ref to either a stub or the actual smooth-scroller. "Control over everything" isn't really a thing in anything ever, and enabling options for toggling some basic functionality is typically not as complex as some like to pretend.

After a quick look-through, the only line that uses smoothscroll.js is app/components/ApplicationComponent.js :: 48. If that's the case, I doubt it'd be "much more complex and harder to maintain", if properly isolated.

bastimeyer commented 8 years ago

I'd be fine with a compile-time configuration

Yeah that could work. I'll add this tomorrow... I'll leave the thread open though, until I know what is happening with the smoothscroll bug...

CosmicToast commented 8 years ago

:+1: Now if only I didn't have to litter my windows install even more... Still, with a compile-time config, it should be easier for me to later look into it and potentially modify/fork.

bastimeyer commented 8 years ago

I just reworked and also hopefully fixed the smoothscroll module. Could you please try this and tell me if this resolves the issue?

https://github.com/bastimeyer/livestreamer-twitch-gui/compare/master...smoothscroll

CosmicToast commented 8 years ago

Thank you for the config-time option. I'll compile the refactor branch and leave it on for an extended period of time, if it fails within a month I'll report back here. If it does not I'll close the issue (and consider it fixed). If anyone else runs into it might reopen later/eventually.

bastimeyer commented 8 years ago

The scrolling did break while using the arrow keys after scrolling with the mouse wheel. Fixed by a452e8a in master now... Thanks for reporting!

bastimeyer commented 8 years ago

https://github.com/bastimeyer/livestreamer-twitch-gui/issues/204#issuecomment-186192815