search

MaddTheSane / Simple-Comic

macOS comic viewer
MIT License
260 stars 22 forks source link

Hide cursor in fullscreen #94

Closed dousherve closed 1 year ago

dousherve commented 1 year ago

Hello,

When using the app (which is very good), I found that it lacked the standard "Hide cursor when in fullscreen" feature.

I then quickly browsed through the code, and noticed that a NSTimer mouseMovedTimer object and a hideCursor function existed, but were never used/called.

So I just added a handleFullscreenCursorHiding function which is called when the mouseMoved event is triggered. It takes care of initializing and invalidating the timer when necessary. After a bit of testing, I set the timeout to 1 second, which feels natural (at least to me 😅). I don't check whether the timer is nil when trying to invalidate it since after testing, nothing yells at me, either at runtime or at compile time (even though it is explicitly set to nil in the init and hideCursor functions). I guess that's an Objective-C thing (well I just checked and it is 😅).

I also moved the if-statement that checks if the app is in fullscreen to this new function, so that the 'convenience function' hideCursor called when the timer fires just does that (as well as setting the timer back to nil).

I'd like to apologize in advance if I violated some 'Pull request' or Objective-C 'convention', since it's literally my first time looking at code written in this language (it's also my first pull request ever).

Thank you for your work, and for taking the time to take a look at this really simple fix (if by any chance you do)!

DavidPhillipOster commented 1 year ago

A quick search for mouseMovedTimer = nil; shows that some of them are NOT preceded by [mouseMovedTimer invalidate]; you have to manually invalidate any previous value before you nil the instance variable. Please fix.

nickv2002 commented 1 year ago

Thanks for the PR @dousherve - please follow up on @DavidPhillipOster suggestion and then we can merge.

We're due for an update so I can work on building a signed version & pushing to the app store soon.

dousherve commented 1 year ago

@DavidPhillipOster Thanks, I indeed overlooked that. I only found one occurence of this in the hideCursor function. It should be fixed now.

@nickv2002 Great! Thank you for your time.

nickv2002 commented 1 year ago

Now in TestFlight if you want to try see: https://github.com/MaddTheSane/Simple-Comic/releases/tag/App-Store-1.9.6

Update: the same build has been released to as a regular release now too