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

Back references and theme with font, in pure Swift and SPM support #6

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Lessica
Copy link

@Lessica Lessica commented Mar 10, 2021

Fix some caveats mentioned here: soffes#18

Copy link
Owner

@alehed alehed left a comment

Choose a reason for hiding this comment

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

Here are a few questions and comments.

internal struct Result: Equatable {

// MARK: - Properties

let patternIdentifier: String
var range: NSRange
let result: NSTextCheckingResult?
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe rename this so that it is clear that this is the result of the match from a previous regex.

Comment on lines +26 to +28
private var fontName: String = Theme.defaultFontName
private var fontSize: CGFloat = Theme.defaultFontSize
private var fontCallback: FontCallback?
Copy link
Owner

Choose a reason for hiding this comment

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

The theme has nothing to do with font size or font name which should be selected by the application based on other criteria. So the FontCallback should return a font for a given FontStyle which it then uses. Alternatively, instead of a callback a struct with one font for each style could be passed in.

Copy link
Author

@Lessica Lessica Mar 14, 2021

Choose a reason for hiding this comment

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

Yes, thank you for your suggestion. But I am not sure if TextMate theme will control font family and font size: https://github.com/textmate/textmate/blob/e4e54440b509557b79b2bd6f7b54bca279a88358/Frameworks/theme/src/theme.cc#L59

Copy link
Owner

Choose a reason for hiding this comment

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

Even if it is supported by textmate, I think these properties should simply be ignored. The text editor should always control typeface and font size (especially since installed fonts are something that differ from machine to machine, worse the same font might be named differently). I don't know any text editor which only lets the theme control these two properties (either it is fixed, or the user can choose).

@@ -278,9 +281,9 @@ open class Parser {
if range.location == NSNotFound {
continue
}

Copy link
Owner

Choose a reason for hiding this comment

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

Extra whitespace to remove.

Copy link
Author

@Lessica Lessica Mar 14, 2021

Choose a reason for hiding this comment

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

Oh, well, great...


import Foundation

internal extension String {
Copy link
Owner

Choose a reason for hiding this comment

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

Why are we using CStrings here everywhere?

Copy link
Author

@Lessica Lessica Mar 14, 2021

Choose a reason for hiding this comment

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

Maybe it's faster? I have no better idea because I copied these codes from my Objective-C project.

Copy link
Author

@Lessica Lessica Mar 14, 2021

Choose a reason for hiding this comment

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

In soffes#18, you said:
\G is not supported (for performance reasons the parser does not guarantee to match the end of a pattern right after begin, and even if it would, \G behaves wierdly with NSRegularExpression). This might be fixable.

To fix this, I am making my progress to import Oniguruma.

Copy link
Owner

Choose a reason for hiding this comment

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

CStrings should not be used, unless you are interoperating with C.

Copy link
Owner

@alehed alehed Mar 15, 2021

Choose a reason for hiding this comment

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

About the other problem, I would maybe do a test with NSRegularExpression vs Oniguruma to see where they behave differently. Obviously it would be better to keep using NSRegularExpression.

@@ -157,7 +175,7 @@ open class AttributedParsingOperation: Operation {
///
/// - returns: A range in newString that can be safely re-parsed. Or nil if
/// everything has to be reparsed.
class func outdatedRange(in newString: NSString, forChange diff: Diff, updatingPreviousResult previous: inout ScopedString) -> NSRange? {
class func outdatedRange(in newString: NSString, forChange diff: Diff, updatingPreviousResult previous: inout ScopedString) -> NSRange {
Copy link
Author

@Lessica Lessica Mar 14, 2021

Choose a reason for hiding this comment

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

The outdated range calculated here sometimes seems incorrent...
Especially near a new line separator. Any ideas?

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