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.

Sign up for GitHub

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

Jump to bottom

Minor code improvements #1622

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Minor code improvements #1622

merged 2 commits into from
Aug 21, 2019

Conversation

hanton
Copy link
Contributor

@hanton hanton commented Aug 17, 2019 •
edited
Loading

Minor code improvements, including:

  • Simplify code logic
  • Keep using user-defined type

nguyenhuy
nguyenhuy reviewed Aug 19, 2019
View reviewed changes
Source/Private/TextExperiment/Component/ASTextLine.mm Outdated
@@ -153,7 +153,7 @@ - (NSString *)description {
[desc appendFormat:@"<ASTextLine: %p> row:%ld range:%tu,%tu", self, (long)self.row, range.location, range.length];
[desc appendFormat:@" position:%@",NSStringFromCGPoint(self.position)];
[desc appendFormat:@" bounds:%@",NSStringFromCGRect(self.bounds)];
return desc;
return [desc copy];
Copy link
Member

Choose a reason for hiding this comment

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

While it is absolutely correct to return an immutable copy, it might not matter in this case since desc is going to be thrown away when this method returns anyways (i.e it can be modified by the caller without affecting anything)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. The commit targets for this purpose is deleted from the PR, and the PR message is also updated :)

Source/Private/TextExperiment/Utility/NSAttributedString+ASText.mm Outdated
@@ -440,7 +440,7 @@ - (NSString *)as_plainTextForRange:(NSRange)range {
[result appendString:[string substringWithRange:range]];
}
}];
return result;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, changing result later on doesn't affect anything?

Source/Private/TextExperiment/Utility/NSParagraphStyle+ASText.mm Outdated
@@ -106,7 +106,7 @@ + (NSParagraphStyle *)as_styleWithCTStyle:(CTParagraphStyleRef)CTStyle {
style.defaultTabInterval = defaultTabInterval;
}

return style;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@nguyenhuy
Copy link
Member

Also, can you please rebase with master so this PR will be tested by our new CI? Thanks.

@hanton
Copy link
Contributor Author

hanton commented Aug 20, 2019 •
edited
Loading

Also, can you please rebase with master so this PR will be tested by our new CI? Thanks.

The PR is rebased with the latest master, but it seems the new CI still can not run on it, a bit of weird? 😮

@hanton hanton closed this Aug 20, 2019
@hanton hanton reopened this Aug 20, 2019
@hanton
Copy link
Contributor Author

hanton commented Aug 20, 2019

I guess that is because the PR is from my fork, and I'm not in the Github Action beta program yet, will come back to have a check after I sign up the program.

@nguyenhuy
Copy link
Member

Ah, that's right. I've re-enabled the old CI for PRs like yours. Here is the official doc.

@nguyenhuy
Copy link
Member

nguyenhuy commented Aug 20, 2019 •
edited
Loading

@hanton I think #1627 should work. Please rebase with master again if you want to give it a try. Thank you!

nguyenhuy
nguyenhuy approved these changes Aug 21, 2019
View reviewed changes
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

LGTM!

@hanton
Copy link
Contributor Author

hanton commented Aug 21, 2019

@hanton I think #1627 should work. Please rebase with master again if you want to give it a try. Thank you!

Work like a charm, thanks @nguyenhuy

nguyenhuy
nguyenhuy reviewed Aug 21, 2019
View reviewed changes
Source/Private/_ASPendingState.mm
@@ -100,7 +100,7 @@ @implementation _ASPendingState
@package //Expose all ivars for ASDisplayNode to bypass getters for efficiency

UIViewAutoresizing autoresizingMask;
unsigned int edgeAntialiasingMask;
CAEdgeAntialiasingMask edgeAntialiasingMask;
Copy link
Member

Choose a reason for hiding this comment

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

@hanton not a blocking comment, but I think there are other places in which we also use unsigned int instead of CAEdgeAntialiasingMask? I'm totally down to change them too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nguyenhuy sorry for missing this comment :p

The PR for fixing this issue is ready at #1667

@nguyenhuy
Copy link
Member

Awesome. Let's get this in. Thanks for the contribution as well as helping to test the new CI setup!

@nguyenhuy nguyenhuy merged commit 1467319 into TextureGroup:master Aug 21, 2019
matthewd1234 pushed a commit to matthewd1234/Texture that referenced this pull request Aug 25, 2019
@hanton @matthewd1234
Minor code improvements (TextureGroup#1622)
4c2e4a3 
* Simplify code logic

* Keep using user-defined type
@hanton hanton deleted the minor-code-improvements branch August 28, 2019 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nguyenhuy nguyenhuy nguyenhuy approved these changes

Assignees
No one assigned
Labels
CI approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@hanton @nguyenhuy