insidegui / WWDC

The unofficial WWDC app for macOS
https://wwdc.io
BSD 2-Clause "Simplified" License
8.63k stars 783 forks source link

Hide controls when the mouse exists or app resigns main #549

Closed yigitcanyurtsever closed 5 years ago

yigitcanyurtsever commented 5 years ago

Fixes two issues with controls staying on screen:

Fixes #538

allenhumphreys commented 5 years ago

This mouse hiding stuff is tricky. Well, multi-window, multi-space, multi-monitor app development is tricky. For context, it looks like this bug was introduced last year while fixing a different bug in these 2 PRs: https://github.com/insidegui/WWDC/pull/437 https://github.com/insidegui/WWDC/pull/477

The bug in question was under a single monitor full screen playback scenario. If you switched from the full screen space, the cursor would incorrectly be hidden. So far I haven't had that issue but I'll test some more tomorrow.

The logic around this stuff is bonkers. I think it'll be hard to say for certain if this fix doesn't break a different usage scenario.

yigitcanyurtsever commented 5 years ago

I definitely see your point. I feel like the problem comes from displaying the controls and making the cursor visible in the same method. There are some cases (like this one 😅) where it feels like the expected behavior should be to have the cursor visible but have the controls hidden.

The app should make sure the cursor is visible every time if it the player view's window resigns main regardless of any guards or checks, i.e.canHideControls checks. Adding NSCursor.unhide() on NSWindow.didResignMain notification handler instead of calling PUIPlayerView.showControls(animated:) would fix the problem but I still think that managing the NSCursor.hide() and NSCursor.unhide() calls manually will always bring new bugs. 😅

My suggestion here is to use NSCursor.setHiddenUntilMouseMoves(true) on PUIPlayerView..mouseIdleTimerAction(_:) instead of NSCursor.hide() and get rid of NSCursor.unhide() on PUIPlayerView.showControls(animated:). It kind of solves all cursor related issues since the cursor will be visible when the mouse moves.

Wydt @allenhumphreys?

allenhumphreys commented 5 years ago

@yigitcanyurtsever I think I had known about that method last year I would've probably investigated using it! But instead of getting rid of NSCursor.unhide() I would think we'd do NSCursor.setHiddenUntilMouseMoves(false). I'd also be worried about causing situations where the controls hide but the curse is somehow still visible in full screen, single monitor viewing, but I might be over-thinking it.

So personally I'm open to these suggestions but would love a cursory review from @insidegui !

insidegui commented 5 years ago

During my testing, I didn't find any issues with this PR applied. We'll see if I regret this decision later 😅