search

xi-editor / xi-mac

The xi-editor mac frontend.
Apache License 2.0
3.02k stars 147 forks source link

Fix a crash at EditView _blinkInsertionPoint #352

Closed rakaramos closed 5 years ago

rakaramos commented 5 years ago

Addresses: #320

As per @cmyr's comment, this should fix the issue.

Pitch

Something caught my attention on this class, the cursor blink state is managed via NSTimer class (doing UI work), every 1 second.

IMHO, that should be done via CVDisplayLink because it's machinery allows us to run code on every frame refresh.

I would love to create a PR with these changes, but first I would like to run it by you guys. Please note that I'm not an macOS expert, I do iOS for a living and when we have a situation like that, we avoid NSTimer in favour of CADisplayLink. Just porting some knowledge that maybe makes sense on the macOS side.

EDIT Further digging into CVDisplayLink: CVDisplayLink is the recommended way to synchronize your drawing/animation with the refresh of the display on macOS. Many people assume it calls your app just after each display vsync event, unfortunately this isn’t the case at all. CVDisplayLink just fetches the refresh rate of your display, and sets a high resolution timer to call you every 16.6ms (for a 60hz display).

Source

codecov-io commented 5 years ago

Codecov Report

Merging #352 into master will decrease coverage by 0.02%. The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #352      +/-   ##
========================================
- Coverage   44.03%    44%   -0.03%     
========================================
  Files          30     30              
  Lines        3861   3863       +2     
========================================
  Hits         1700   1700              
- Misses       2161   2163       +2
Impacted Files Coverage Δ
Sources/XiEditor/EditView.swift 54.44% <0%> (-0.32%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 59d72e9...e9213f7. Read the comment docs.

jansol commented 5 years ago

My (limited) knowledge about macOS APIs agrees with using CADisplayLink. Maybe this should even have its own layer if it doesn't already.

rakaramos commented 5 years ago

@cmyr fair enough, that's correct! Also #315 is the right place for it, and with opengl dropped in favour of metal, complicates it. thanks!

trishume commented 5 years ago

@cmyr turned out to only be on multi-display when you do it a certain way that it fakes it with a timer. Most of the time it works. See the update to http://thume.ca/2017/12/09/cvdisplaylink-doesnt-link-to-your-display/

I still think an NSTimer is probably what you want here since the frequency is low. If you were doing a Sublime-style smooth fade you'd want display link.