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

Refactoring needed #12

Open
raspu opened this issue Jan 16, 2017 · 7 comments
Open

Refactoring needed #12

raspu opened this issue Jan 16, 2017 · 7 comments

Comments

@raspu
Copy link
Owner

raspu commented Jan 16, 2017

The code right now is a little messy and could use a big refactoring to make it easier to follow and more scalable.

@raspu raspu self-assigned this Jan 16, 2017
@raspu raspu removed their assignment Jan 16, 2017
@cyanzhong
Copy link

Totally agree with that, this project is awesome.
I'd like to see a delightful structure and flexible code.
It would be a great project! 👍

@raspu
Copy link
Owner Author

raspu commented Jan 17, 2017

Hi @cyanzhong thanks for the support, I am not sure when I will able to work on this, but it's certainly needed.

@raspu raspu changed the title Refactorig needed Refactoring needed Jan 17, 2017
@brunophilipe
Copy link

I'm working on some improvements to this code, you can check my fork repo.

Are you welcome to breaking changes? I have replaced a nullable property for an enum in CodeAttributedString, for example. If these changes are merged back, they should probably be put in a new major version, I guess.

@raspu
Copy link
Owner Author

raspu commented Aug 3, 2017

Hi @brunophilipe! That's awesome, thank you!

About the language detection for the CodeAttributedString, I wasn't able to do this before because the way the library works (and is the same concern I have with #17), it will highlight only the line that the user is currently editing, and the detection system sometimes is not able to detect the language with just one line of code. Maybe to make it more reliable we could use the previously detected language or give the highlighter more context (when available) to avoid issues like this (even if this will impact performance).

And about the refactoring, I had in mind a more complex restructuring of the library, I would like to separate the highlighting in phases, in order to allow the creation plugins for things like indentation and attributes customization, also I would like to move all the regex that makes the code so difficult to follow. So, even if you send me a PR I will keep this issue open.

@brunophilipe
Copy link

brunophilipe commented Dec 3, 2017

@raspu I believe I have figured out a neat way to fix the highlight during editing.

Unfortunately I don't believe it is possible to avoid re-highlighting the entire string sometimes, as if, for example, you add a /* for a block comment, you'd need to know where afterwards in the document to stop highlighting. To do this reasonably well would mean to basically reimplement highlight.js...

However, in my own testing, this seems to work well enough.

I'll push my changes after I've done some tests. Basically it logs the language <span> blocks reported by highlight.js as attributes and then uses them as hints of the language to request highlighting.

Edit:

Also, in my testing, rewriting CodeTextStorage in Objective-C yielded a massive performance improvement. I guess this will be the way to go until the expensive NSString-String Swift bridging is improved 😞

@raspu
Copy link
Owner Author

raspu commented Dec 4, 2017

@brunophilipe that sounds awesome, I am looking forward to your PR. I thought about doing something like that before, but I never had the time to test it :).

Is really so much faster to use Objc? If that's so I can easily port the library to Objc...

@bpisano
Copy link

bpisano commented Jul 2, 2019

I've tried some code refactoring with some improvements in #69. Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants