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

C preprocessor colors improvement #18671

Merged
merged 7 commits into from Feb 9, 2022
Merged

C preprocessor colors improvement #18671

merged 7 commits into from Feb 9, 2022

Conversation

braoult
Copy link
Contributor

@braoult braoult commented Feb 8, 2022

Fixes #18670

C comments and preprocessor had nearly-same colors in light theme.
Old values:

.chroma .cm { color: #999988; } /* CommentMultiline */
.chroma .cp { color: #999999; } /* CommentPreproc */
.chroma .cpf { color: #999999; } /* CommentPreprocFile */

Proposed values:

.chroma .cp { color: #109295 } /* CommentPreproc */
.chroma .cpf { color: #4C4DBC; } /* CommentPreprocFile */

@lunny
Copy link
Member

lunny commented Feb 8, 2022

Hi, could you post some screenshots before and after?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 8, 2022
@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Feb 8, 2022
@braoult
Copy link
Contributor Author

braoult commented Feb 8, 2022

Hi, could you post some screenshots before and after?

Screenshots were in #18670.

Before:
Screenshot_2022-02-08_11-50-04

After:
Screenshot_2022-02-08_11-54-40

@silverwind
Copy link
Member

Please also check/show screenshot in dark theme (Settings -> Account -> Select default theme).

@braoult
Copy link
Contributor Author

braoult commented Feb 8, 2022

Please also check/show screenshot in dark theme (Settings -> Account -> Select default theme).

Dark theme has different colors for preprocessor lines, this is why I did not change anything there :
Screenshot_2022-02-08_14-18-36

It does not have a different color for filenames, but this is not really an issue (I did hesitate before doing it to light theme). I mean filenames colors are much less important than full macro parsing (unsupported by chroma).

EDIT: I noticed some other discrepancies between light/dark themes : .s (string literal) and .se (string literal escaped characters) are different on dark theme, not on light theme, for example (and I must admit that I don't like at all the dark theme choice there) :
Screenshot_2022-02-08_14-32-09

Screenshot_2022-02-08_14-33-26

@silverwind
Copy link
Member

Dark theme has a few issues like that, we need to fix them separately. Would appreciate if you could fix the ifdefs and filenames in dark theme as well. I think we could just copy the same colors as from light theme, maybe with slightly increased luminosity.

@braoult
Copy link
Contributor Author

braoult commented Feb 8, 2022

Dark theme has a few issues like that, we need to fix them separately. Would appreciate if you could fix the ifdefs and filenames in dark theme as well. I think we could just copy the same colors as from light theme, maybe with slightly increased luminosity.

I would be happy to do that, but there is something I do not understand in chroma files :-(
I am unsure where background colors are defined (for html & chroma). Is there some "magic" in dark.less so that background colors are correct without being defined there ?
Meantime, with these changes :

.chroma { --color-code-bg: #2a2e3a; }
.chroma .cpf { color: #03dfff; } /* CommentPreprocFile */

I can get something in chroma highlighting (but whole page & line numbers get light background. However, this is not very important here. Would those preprocessor (and include filenames) colors be OK (the only change is the blue color for include filenames) ?

Screenshot_2022-02-08_17-35-01

If yes, I could ament my PR with .cpf value.

@silverwind
Copy link
Member

silverwind commented Feb 8, 2022

Background color comes from --color-code-bg which is defined here:

--color-code-bg: #ffffff;

--color-code-bg: #2a2e3a;

So essentially, the chroma files do not set background because background comes from the base theme (we need to restructure theme variables into separate files later to be more clear).

Colors seem fine.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 8, 2022
@lafriks lafriks added this to the 1.17.0 milestone Feb 8, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 9, 2022
@zeripath
Copy link
Contributor

zeripath commented Feb 9, 2022

make lgtm work

@zeripath zeripath merged commit 439ad34 into go-gitea:main Feb 9, 2022
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 9, 2022
* C preprocessor colors improvement

Fixes go-gitea#18670

* Update web_src/less/chroma/light.less

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

* typo

missing semi

* add color for #include filenames

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
lunny added a commit that referenced this pull request Feb 10, 2022
* C preprocessor colors improvement

Fixes #18670

* Update web_src/less/chroma/light.less

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

* typo

missing semi

* add color for #include filenames

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

Co-authored-by: Bruno Raoult <braoult@users.noreply.github.com>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 10, 2022
* giteaofficial/main:
  Fix issue with docker-rootless shimming script (go-gitea#18690)
  tests: remove redundant comparison in repo dump/restore (go-gitea#18660)
  [skip ci] Updated translations via Crowdin
  Disable unnecessary OpenID/OAuth2 elements (go-gitea#18491)
  Add apply-patch, basic revert and cherry-pick functionality (go-gitea#17902)
  C preprocessor colors improvement (go-gitea#18671)
  Update object repo with the migrated repository (go-gitea#18684)
@wxiaoguang wxiaoguang added the backport/done All backports for this PR have been created label Feb 11, 2022
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* C preprocessor colors improvement

Fixes go-gitea#18670

* Update web_src/less/chroma/light.less

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

* typo

missing semi

* add color for #include filenames

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong CSS for C code highlighting (light theme)
7 participants