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

Fix a crash at EditView _blinkInsertionPoint #352

Merged
merged 2 commits into from
Nov 18, 2018
Merged

Conversation

rakaramos
Copy link
Contributor

@rakaramos rakaramos commented Nov 17, 2018

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
Copy link

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%) ⬇️

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
Copy link
Contributor

jansol commented Nov 17, 2018

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

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

We spent a while investigating CVDisplayLink a year or so ago. I forget our exact conclusions, but I recall that we weren't impressed. Maybe @trishume remembers the particulars, but iirc it seemed that the display link didn't necessarily match the draw timing of the display; it was just a periodic timer linked to the refresh rate? Or something like that?

Putting the cursor in a layer might be tricky, right now, because we do all of our drawing with OpenGL. We will need to reconsider this at some point, (see #315) but I'm not sure how well this will work until then.

@rakaramos
Copy link
Contributor Author

@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!

@cmyr cmyr merged commit b0e23d9 into xi-editor:master Nov 18, 2018
@trishume
Copy link

@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.

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.

5 participants