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

Add bracket highlighter #356

Merged
merged 3 commits into from Sep 22, 2020
Merged

Conversation

pmarnik
Copy link
Contributor

@pmarnik pmarnik commented Sep 18, 2020

Please consider adding this implementation of matching brackets highlighting.

sequel_pro_bracket_hightlighting

@pmarnik pmarnik changed the title add bracket highlighter Add bracket highlighter Sep 18, 2020
@Jason-Morcos
Copy link
Member

Hey there! This looks pretty cool! I don't see an option in the settings to enable/disable this feature though. Would it be possible to include that? I think some users may want to turn this off and I think we want to allow users to do so. Syntax highlighting may provide a good example of how to set up a preference for the query editor!

@Jason-Morcos Jason-Morcos changed the base branch from master to dev September 18, 2020 21:13
@stychos
Copy link
Contributor

stychos commented Sep 19, 2020

I agree about having possibility to disable this from preferences. Sometimes it's needed to work with a huge queries, and syntax highlighting going to be disabled for speed.

@pmarnik
Copy link
Contributor Author

pmarnik commented Sep 20, 2020

I've added option to "Preferences" which will allow to control enabling/disabling this feature.

@Jason-Morcos
Copy link
Member

I've added option to "Preferences" which will allow to control enabling/disabling this feature.

Awesome! Unfortunately, it looks like it's conflicting with the other pref change we just made. Would you be able to resolve?
Additionally, when committing changes to the .xib files, try to commit only the exact lines you modify (so revert changes to other parts of the file). Anytime you open an XIB it updates like the whole thing, so I usually revert all but the specific parts of the file in my client Sourcetree.

@pmarnik
Copy link
Contributor Author

pmarnik commented Sep 21, 2020

I've resolved conflicts. In preference pane I added only "Bracket highlighting" option. When I try to align it, then a lot of content in this file is changing. Should I leave it as is (and you will adjust alignment after merge), or algin on my branch?

Preferences 2020-09-21 22-16-17

@Jason-Morcos
Copy link
Member

I've resolved conflicts. In preference pane I added only "Bracket highlighting" option. When I try to align it, then a lot of content in this file is changing. Should I leave it as is (and you will adjust alignment after merge), or algin on my branch?

Preferences 2020-09-21 22-16-17

Hmmmm go ahead and try to align it! I'd rather have it lined up perfect, even if that means more changes

@Jason-Morcos
Copy link
Member

Want to say, I really love this PR. I think this will be a feature that many of us will use and love.
One comment/request/note - It's a little hard to see the text with highlights in certain backgrounds. Would it be possible for the color to adjust dynamically and/or be customizable in the query editor themes? Definitely not a blocking thing for merging this PR, just a note.

Screen Shot 2020-09-21 at 20 08 01

@pmarnik
Copy link
Contributor Author

pmarnik commented Sep 22, 2020

I fixed the label in settings alignment. Additionally I modified a little bit highlight color, so it looks better on dark theme.

Maybe in some future PR I will try to provide some better solution for color customization.

Copy link
Member

@Jason-Morcos Jason-Morcos left a comment

Choose a reason for hiding this comment

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

Awesome work, @pmarnik!

@Jason-Morcos Jason-Morcos merged commit 7769f3a into Sequel-Ace:dev Sep 22, 2020
This was referenced Sep 22, 2020
@Kaspik
Copy link
Member

Kaspik commented Oct 18, 2020

Seems like there is an issue introduced most probably by this PR - #413

@pmarnik Would you mind taking a look?

@pmarnik
Copy link
Contributor Author

pmarnik commented Oct 18, 2020

I'm not able to reproduce #413 issue.

@Kaspik
Copy link
Member

Kaspik commented Nov 30, 2020

@pmarnik Sooo we have an issue here. We were able to get syntaxHighlight performance to really good state, but bracket highlighter is causing a lot of issues. Specifically:
[self.bracketHighlighter bracketHighlight:caretPosition -1 inRange:currentQueryRange]; called in SPCustomQuery. If you import any SQL file so your query editor has for example 1000 lines, (simple create command with values and some columns), it means your query editor can have 25000 characters. In that case, writing new query and adding (` on line ~100 causes CPU to block main thread because it needs to go through 19 000 characters and find anything to highlight.

Any ideas what to do? We can also simply don't highlight code if the currentQueryRange.length is more than 1000, but ... wanted to first ask you about ideas.

@Kaspik
Copy link
Member

Kaspik commented Nov 30, 2020

Made a hotfix for now: 540b7f6

@pmarnik
Copy link
Contributor Author

pmarnik commented Dec 1, 2020

I have some ideas how to improve performance of bracket highlighting. I need to implement cache of comments locations.

@Kaspik
Copy link
Member

Kaspik commented Dec 1, 2020

@robinkunde had also a comment that for example Sublime limits num of characters it looks forward (https://forum.sublimetext.com/t/matching-bracket-limit/1852/2) - we could do that too. It doesn't make sense to highlight something that is out of screen, so not looking further than 2000 chars makes sense. What do you think?

@pmarnik
Copy link
Contributor Author

pmarnik commented Dec 1, 2020

@Kaspik I think that this limit is a good idea.

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.

None yet

4 participants