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

Stop treating document as an optional #6

Merged
merged 2 commits into from
Mar 25, 2017

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Mar 24, 2017

This is something that had bit me last week when I was playing around with some other drawing changes. By treating the document as an optional we were failing silently if we tried to send an RPC while it was nil. I think it's better if we crash in that situation.

It should be expected that a given view / viewcontroller always has a
corresponding backing document. If it doesn't, we currently fail
silently, which makes debugging more difficult.

The EditView also no longer needs to have its own reference to the
document. This can instead be exposed via the datasource protocol (which
may be poorly named).
This is more in line with Cocoa naming conventions, and makes using
Xcode quick-open easier (ViewContainer and ViewController are too
similar.)
@cmyr
Copy link
Member Author

cmyr commented Mar 24, 2017

I also added a little commit renaming the EditViewContainer to EditContainerView; the earlier name's similarity to EditViewController made using Xcode's quick-open panel frustrating.

@raphlinus
Copy link
Member

Last I played around with this, it was making the sending of scroll not update its "last" if document was nil. It would only send new scroll parameters if the were different than the "last" ones, so when document was nil it would drop it on the floor. So my only concern is, could there be a race condition where document is nil? If there's a good reason why not, I'm happy to merge this.

@raphlinus raphlinus merged commit 813f536 into xi-editor:master Mar 25, 2017
@cmyr cmyr deleted the non-optional-document branch March 25, 2017 03:28
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.

2 participants