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

Rainbow parentheses feature #7027

Merged
merged 14 commits into from Jun 18, 2020
Merged

Rainbow parentheses feature #7027

merged 14 commits into from Jun 18, 2020

Conversation

adamconroy
Copy link
Contributor

@adamconroy adamconroy commented Jun 2, 2020

Colorize matching parentheses, braces, and brackets in a series of colors. This is customizable in the theme CSS. It can also be turned on/off dynamically per document through the Code -> Rainbow parentheses option.

Example in light and dark themes:
Oth1C9K

Closes #1888

Copy link
Member

@gtritchie gtritchie left a comment

Choose a reason for hiding this comment

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

Very minor tweaks requested, and a few additional generic questions:

  • guessing you already did, but make sure the themes unit-tests pass
  • have you sanity tested in IE11?
  • what happens if someone has previously imported a custom theme, then upgrades and turns on rainbow parentheses; just curious how that works in general when we add new css rules to the themes

src/cpp/session/include/session/prefs/UserPrefValues.hpp Outdated Show resolved Hide resolved
src/cpp/session/prefs/UserPrefValues.cpp Outdated Show resolved Hide resolved
src/gwt/acesupport/acemode/c_cpp.js Show resolved Hide resolved
/*
* rainbow_paren_highlight_rules.js
*
* Copyright (C) 2009-20 by RStudio, PBC
Copy link
Member

Choose a reason for hiding this comment

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

Switch to our new "current-year only" format.

@@ -77,6 +77,7 @@ public EditingPreferencesPane(UserPrefs prefs,
"When enabled, the indentation for documents not part of an RStudio project " +
"will be automatically detected."));
editingPanel.add(checkboxPref("Insert matching parens/quotes", prefs_.insertMatching()));
editingPanel.add(checkboxPref("Rainbow parentheses", prefs_.rainbowParentheses()));
Copy link
Member

Choose a reason for hiding this comment

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

Does everything in this pane still fit (vertically)? Noticed this pane is getting very full. I ran into a problem adding a checkbox to the Code / Display pane in #7030 and had to remove some other spacing or it was slightly clipped at the bottom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, there's still room for like...one more checkbox. It's definitely cramped though. We are in the situation where we want to start thinking about a different format for our options panel.

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

Nicely done! Overall this LGTM -- the one thing that I think is missing is one bit of code where we check this explicit type for parentheses; e.g. here:

https://github.com/rstudio/rstudio/blob/master/src/gwt/acesupport/acemode/token_cursor.js#L1102-L1130

I think that code needs to be changed to accommodate the new type.

@kevinushey
Copy link
Contributor

Also echoing Gary's question here:

what happens if someone has previously imported a custom theme, then upgrades and turns on rainbow parentheses; just curious how that works in general when we add new css rules to the themes

I think we need to still include the default paren.keyword.operator type so that "default" highlight rules can be selected for parentheses, in case a custom theme doesn't define these colors yet. (But we want the rainbow rules to have a higher precedence than the "default" rules.)

@adamconroy
Copy link
Contributor Author

adamconroy commented Jun 10, 2020

Unfortunately in the themes paren.keyword.operator is defined with !important. I can add important to all the styles that I just added but that's a pretty bad escalation that leads to more trouble down the line. I think if we can just communicate that custom themes will need to adapt as we add styles that will be fine, not everything can be backwards compatible. I can be thorough about this in the blog post about the feature. Rainbow parens defaults to off and still uses the old style so they don't lose any functionality in that case.

Copy link
Member

@gtritchie gtritchie left a comment

Choose a reason for hiding this comment

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

LGTM! Excited to see this in action.

Copy link
Member

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

I do think it's going to be a problem that this feature won't work with old custom themes. Here is a proposal for resolving the issue:

  1. Make the theme CSS rules for the colors more specific; e.g., rather than .ace_paren_color_0, the theme could specify div .ace_editor .ace_paren_color_0.
  2. Define a default set of dark/light CSS colors for parens and include them in our root style sheet, themes.css (i.e. .ace_paren_color_0, .editor_dark .ace_paren_color_0).

This way the themes can customize the colors as needed, but if the theme fails to provide the colors we will still get nice defaults. What do you think?

@@ -515,6 +515,11 @@
"title": "Highlight R function calls",
"description": "Whether to highlight R function calls in the code editor."
},
"rainbow_parentheses": {
"type": "boolean",
"default": false,
Copy link
Member

Choose a reason for hiding this comment

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

Add a title to this (and regenerate prefs) so it can be set from the Command palette.

Copy link
Member

Choose a reason for hiding this comment

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

(note you'll probably have to merge master into your branch to get this capability)

@adamconroy
Copy link
Contributor Author

That solution sounds good. I wish we could just remove !important from most/all of the styles but there are so many of them who knows what will happen, especially with custom themes.

@jmcphers
Copy link
Member

If you go the route of adding default styles, it looks like you could even forgo the addition of adding CSS rules to individual themes, since we just have one rainbow set for dark themes and another for light themes (easily scoped with .editor_dark). That would make the change much simpler w/o closing the door to custom rainbow colors in custom themes.

@adamconroy
Copy link
Contributor Author

Ok this should be good to go now!

Copy link
Member

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

LGTM!

@adamconroy
Copy link
Contributor Author

@kevinushey it this looks good to you it's ready to go!

Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

LGTM!

@kevinushey kevinushey merged commit 1d578c6 into master Jun 18, 2020
@jmcphers
Copy link
Member

🎉

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.

Enhancement: Colour matched parenthesis/braces
4 participants