Skip to content

GLKit workaround #trivial#54

Merged
nguyenhuy merged 4 commits into
TextureGroup:masterfrom
stephenkopylov:master
Apr 22, 2017
Merged

GLKit workaround #trivial#54
nguyenhuy merged 4 commits into
TextureGroup:masterfrom
stephenkopylov:master

Conversation

@stephenkopylov

@stephenkopylov stephenkopylov commented Apr 21, 2017

Copy link
Copy Markdown
Contributor

canClearContentsOfLayer now turned off for views with CAEAGLLayer

more:
#53

@stephenkopylov
GLKit workaround:
617bc7d 
canClearContentsOfLayer now turned off for views with CAEAGLLayer
@CLAassistant

CLAassistant commented Apr 21, 2017 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@stephenkopylov stephenkopylov mentioned this pull request Apr 21, 2017
Closed
nguyenhuy
nguyenhuy approved these changes Apr 21, 2017
View reviewed changes

@nguyenhuy nguyenhuy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nits aside, LGTM although I'd prefer another approval from a core maintainer before merging.

Comment thread Source/ASDisplayNode.mm Outdated

// CAEAGLLayer
if([view.layer.class isSubclassOfClass:[CAEAGLLayer class]]){
_flags.canClearContentsOfLayer = NO;

@nguyenhuy nguyenhuy Apr 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Indentation is off

Comment thread Source/ASDisplayNode.mm Outdated
}

// CAEAGLLayer
if([view.layer.class isSubclassOfClass:[CAEAGLLayer class]]){

@nguyenhuy nguyenhuy Apr 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: [view.layer class] for consistency.

@stephenkopylov

stephenkopylov commented Apr 21, 2017

Copy link
Copy Markdown
Contributor Author

Fxd

maicki
maicki approved these changes Apr 21, 2017
View reviewed changes

@maicki maicki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We may should start now thinking about a better structure around specific classes that need special handling as there are quite a couple different cases now. But this looks good to me.

@nguyenhuy

nguyenhuy commented Apr 21, 2017

Copy link
Copy Markdown
Member

@maicki Agreed!

@nguyenhuy

nguyenhuy commented Apr 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@stephenkopylov Would you mind signing the CLA?
@maicki Can you kick off CI build?

@stephenkopylov

stephenkopylov commented Apr 21, 2017

Copy link
Copy Markdown
Contributor Author

@nguyenhuy
I signed it, but notification is still here...

2017-04-21 22 34 19

@nguyenhuy

nguyenhuy commented Apr 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@stephenkopylov Could you try to add the email to your account?

Stepan Kopylov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@stephenkopylov

stephenkopylov commented Apr 21, 2017

Copy link
Copy Markdown
Contributor Author

@nguyenhuy hooray! that was because I pushed PR from office, where I use different email.

@stephenkopylov

stephenkopylov commented Apr 22, 2017

Copy link
Copy Markdown
Contributor Author

Sry guys, I missed one square bracket. Fxd

@ghost

ghost commented Apr 22, 2017

Copy link
Copy Markdown
2 Errors
🚫 Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.
🚫 Please ensure new source files begin with:

//
//  ASDisplayNode.mm
//  Texture
//
//  Copyright (c) 2014-present, Facebook, Inc.  All rights reserved.
//  This source code is licensed under the BSD-style license found in the
//  LICENSE file in the root directory of this source tree. An additional grant
//  of patent rights can be found in the PATENTS file in the same directory.
//  Modifications to this file made after 4/13/2017 are: Copyright (c) 2017-present,
//  Pinterest, Inc.  Licensed under the Apache License, Version 2.0 (the "License");
//  you may not use this file except in compliance with the License.
//  You may obtain a copy of the License at
//  
//      http://www.apache.org/licenses/LICENSE-2.0
//
//  Unless required by applicable law or agreed to in writing, software
//  distributed under the License is distributed on an "AS IS" BASIS,
//  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
//  See the License for the specific language governing permissions and
//  limitations under the License.
//

Generated by 🚫 Danger

@stephenkopylov stephenkopylov changed the title GLKit workaround: GLKit workaround #trivial Apr 22, 2017
nguyenhuy
nguyenhuy approved these changes Apr 22, 2017
View reviewed changes
Comment thread Source/ASDisplayNode.mm
//
// ASDisplayNode.mm
// AsyncDisplayKit
// Texture

@nguyenhuy nguyenhuy Apr 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catch!

@nguyenhuy

nguyenhuy commented Apr 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@stephenkopylov Thanks for being patient and working through all of the hurdles. Hope to see more PRs from you!

@nguyenhuy
nguyenhuy merged commit 1f69274 into TextureGroup:master Apr 22, 2017
@CoderXL CoderXL mentioned this pull request Sep 28, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maicki maicki maicki approved these changes
@nguyenhuy nguyenhuy nguyenhuy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stephenkopylov @CLAassistant @nguyenhuy @maicki