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 support for rendering console output with colors #19497

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Apr 26, 2022

Fixes #19392

Extension .sh-session would be automatically rendered as console output

Screenshot:
screenshot

@lafriks lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Apr 26, 2022
@lafriks lafriks added this to the 1.17.0 milestone Apr 26, 2022
@lafriks lafriks changed the title Add support for rendering terminal output with colors Add support for rendering console output with colors Apr 26, 2022
routers/web/repo/view.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 26, 2022
@lafriks lafriks force-pushed the feat/console_colors branch 3 times, most recently from a771ffd to 4483c5f Compare April 26, 2022 09:16
@wxiaoguang
Copy link
Contributor

I am very curious why the colored logs should be submitted into a git repository .... it violates the git usage.

If someone really needs to use git to manage their logs, they can use an external render IMO.

@lafriks
Copy link
Member Author

lafriks commented Apr 26, 2022

It's not only about logs, it's also about console output from many tools. Git can be used not only for code but also for documentation, it's quite common to store tool report output when doing either software functionality, load/stress or security testing and many tools does colored output for it's reports that in git become quite unreadable, thus this PR :)

Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

It's unfortunate that the package doesn't support io.Reader, but code-wise LGTM.

@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 Apr 26, 2022
@silverwind
Copy link
Member

Do I understand correctly that it would render any file that contains a literal \x1b as console output, even if the file may be a regular source code file? If yes, maybe we should skip rendering as console output if enry detects something else.

Also note that inside markdown ```console or ```bash snippets are pretty common, maybe we could also render them colored, but I would not change the background color to black of such snippets.

@lafriks
Copy link
Member Author

lafriks commented Apr 26, 2022

Do I understand correctly that it would render any file that contains a literal \x1b as console output, even if the file may be a regular source code file? If yes, maybe we should skip rendering as console output if enry detects something else.

True, I have added enry check and only if enry does not detect it as anything (detects as lang other) than it's rendered as console output.

Also note that inside markdown ```console or ```bash snippets are pretty common, maybe we could also render them colored

bash snippet should not be rendered as console output as that bash or sh usually means script code not console output. Only console could be rendered but that is not currently possible by markdown rendering library. At least I could not find how to implement it.

but I would not change the background color to black of such snippets.

Their background color would have to be changed as color codes in console output are usually meant to be displayed on black background thus there is high chance that part of that could be unreadable because of that.

@silverwind
Copy link
Member

Only console could be rendered but that is not currently possible by markdown rendering library. At least I could not find how to implement it.

No big issue, maybe it's something for later.

Their background color would have to be changed as color codes in console output are usually meant to be displayed on black background thus there is high chance that part of that could be unreadable because of that.

Ideally the console should also have a light background on light theme. It'd certainly possible to ensure contrast via different colors for light/dark. But I guess if you're happy with just one theme, we can go with that now.

@lafriks
Copy link
Member Author

lafriks commented Apr 26, 2022

One theme should be ok for now as usually terminal windows still have black background even on light themed desktop environment

@lunny
Copy link
Member

lunny commented Jun 4, 2022

How about allow users setting extensions but not have the detection because the detection maybe wrong?

@lafriks
Copy link
Member Author

lafriks commented Jun 4, 2022

It will try to detect only after language detection is done and would be otherwise rendered as plain text so chance of false rate is low. Also you can always switch to plain text display

routers/web/repo/view.go Show resolved Hide resolved
routers/web/repo/view.go Show resolved Hide resolved
web_src/less/console/console.less Outdated Show resolved Hide resolved
web_src/less/index.less Show resolved Hide resolved
modules/markup/renderer.go Show resolved Hide resolved
modules/markup/console/console.go Show resolved Hide resolved
modules/markup/console/console.go Outdated Show resolved Hide resolved
modules/markup/console/console.go Show resolved Hide resolved
main.go Show resolved Hide resolved
modules/markup/console/console.go Show resolved Hide resolved
@lafriks lafriks force-pushed the feat/console_colors branch 2 times, most recently from f23dd7a to 2f9b50a Compare June 8, 2022 20:27
@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 8, 2022
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jun 8, 2022
@lafriks lafriks merged commit f92b7a6 into go-gitea:main Jun 8, 2022
@lafriks lafriks deleted the feat/console_colors branch June 8, 2022 21:46
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 9, 2022
* giteaofficial/main:
  Prevent NPE whilst migrating if there is a team request review (go-gitea#19855)
  [skip ci] Updated translations via Crowdin
  Add support for rendering terminal output with colors (go-gitea#19497)
  Fix viewed images not loading in a PR (go-gitea#19919)
  Remove out-dated comments (go-gitea#19921)
  Automatically render wiki TOC (go-gitea#19873)
  Improve wording on delete access token modal (go-gitea#19909)
  [skip ci] Updated translations via Crowdin
  Add breaking email restrictions checker in doctor (go-gitea#19903)
  Ensure minimum mirror interval is reported on settings page (go-gitea#19895)
  Improve UX on modal for deleting an access token (go-gitea#19894)
  update discord invite (go-gitea#19907)
  Only log non ErrNotExist errors in git.GetNote  (go-gitea#19884)
  [skip ci] Updated translations via Crowdin
  Update frontend guideline (go-gitea#19901)
  Make AppDataPath absolute against the AppWorkPath if it is not (go-gitea#19815)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for colored console log rendering
7 participants