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

scide: adding solarizedLight and solarizedDark themes #3412

Merged

Conversation

Projects
None yet
5 participants
@redFrik
Copy link
Contributor

redFrik commented Jan 12, 2018

i add it here but we can close directly if the dracula theme is preferred. #3410

@gusano

This comment has been minimized.

Copy link
Member

gusano commented Jan 12, 2018

@redFrik we should also add both solarized ones as well :)

@patrickdupuis patrickdupuis added this to the 3.9.1 milestone Jan 12, 2018

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Jan 12, 2018

I would support adding both :)

@redFrik

This comment has been minimized.

Copy link
Contributor Author

redFrik commented Jan 12, 2018

i should've mentioned that this is after https://github.com/helmholtz/sc-solarized

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Jan 12, 2018

Same as with #3410, just want to make sure that licensing has been properly considered.

@snappizz
Copy link
Member

snappizz left a comment

same as #3410 -- let's put these in a separate C++ source file with the license.

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Jan 22, 2018

As was discussed on slack, adding this and other themes via separate C++ files requires a little more work than expected. It was decided to simply add the license text above the function that contains the theme's colour palette.

I've created a PR against @redFrik's branch which adds the license text.

@patrickdupuis patrickdupuis referenced this pull request Jan 24, 2018

Merged

Add license text #2

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Jan 26, 2018

Ping @redFrik. We'd like to get this into 3.9.1 if possible.

@redFrik

This comment has been minimized.

Copy link
Contributor Author

redFrik commented Jan 26, 2018

thanks for taking care of adding the licence. i'm happy to see this go into 3.9.1. is there something i need to do though? like some button click on github?

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Jan 26, 2018

First, you need to merge redFrik#2 so that those changes become part of this PR. There may be some sort of conflict that needs to be resolved before this can be merged into 3.9.

redFrik added some commits Jan 26, 2018

@redFrik

This comment has been minimized.

Copy link
Contributor Author

redFrik commented Jan 26, 2018

ok @patrickdupuis merged and resolved. hope it's good now. thanks for your help

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Jan 26, 2018

@snappizz Good to go?

@@ -271,6 +355,11 @@ Theme::Theme(const QString & _name, Manager * settings)
mLocked = true;
} else if (mName == "dracula") {
fillDracula();
} else if (mName == "solarizedLight") {

This comment has been minimized.

Copy link
@brianlheim

brianlheim Jan 26, 2018

Member

There needs to be a line mLocked = true in the block above.

added missing mLocked = true;
sorry, must have happened when i resolved conflicts
@brianlheim
Copy link
Member

brianlheim left a comment

thanks!

Licensing issues have been addressed.

@brianlheim brianlheim merged commit 216cbc5 into supercollider:develop Jan 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

brianlheim added a commit to brianlheim/supercollider that referenced this pull request Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.