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

Editor preview support for external renderers #23333

Merged
merged 9 commits into from
Mar 24, 2023

Conversation

brechtvl
Copy link
Contributor

@brechtvl brechtvl commented Mar 6, 2023

Remove [repository.editor] PREVIEWABLE_FILE_MODES setting that seemed like it was intended to support this but did not work. Instead, whenever viewing a file shows a preview, also have a Preview tab in the file editor.

Add new /markup web and API endpoints with comment, gfm, markdown and new file mode that uses a file path to determine the renderer.

Remove /markdown web endpoint but keep the API for backwards and GitHub compatibility.

⚠️ BREAKING ⚠️

The [repository.editor] PREVIEWABLE_FILE_MODES setting was removed. This setting served no practical purpose and was not working correctly. Instead a preview tab is always shown in the file editor when supported.

@brechtvl
Copy link
Contributor Author

brechtvl commented Mar 6, 2023

Example:
editor_preview_external_renderers

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 6, 2023
@brechtvl
Copy link
Contributor Author

brechtvl commented Mar 6, 2023

The /markdown and MarkdownOption names aren't accurate anymore,. Should I create a new /markup and MarkupOption, and keep the old ones around for backwards compatibility? Rename and break compatibility? Leave it as is?

There is an existing PREVIEWABLE_FILE_MODES setting for this purpose, but it
didn't work for anything except markdown.
@brechtvl brechtvl force-pushed the editor-preview-external-renderers branch from d55dcbc to ea90dd0 Compare March 6, 2023 13:53
@lunny lunny added the type/enhancement An improvement of existing functionality label Mar 6, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 6, 2023
@silverwind
Copy link
Member

Yeah, rename and update docs, and keep the old ones as compat alias. Ideally we should deprecate but I don't think we have a system in place for this.

@brechtvl brechtvl marked this pull request as draft March 6, 2023 15:43
@lunny
Copy link
Member

lunny commented Mar 7, 2023

For internal web routers, I think we can just use /markup replace /markdown and don't care the change of routers.

For API endpoints, we can keep both endpoints at the moment. /markdown will be kept as before but not external renderers. And /markup could include all /markdown

@brechtvl brechtvl marked this pull request as ready for review March 7, 2023 12:16
routers/web/misc/markup.go Outdated Show resolved Hide resolved
@codecov-commenter

This comment was marked as off-topic.

@lunny
Copy link
Member

lunny commented Mar 13, 2023

"file" mode looks like not work. I put the xxxx of [markup.xxxx] in PREVIEWABLE_FILE_MODES, then it works.

@brechtvl
Copy link
Contributor Author

brechtvl commented Mar 13, 2023

I'm not sure what is not working exactly? For me the /markup API in mode file works for all [markup. xxxx].

[repository.editor] PREVIEWABLE_FILE_MODES only affects if the preview tab is displayed in the editor or not. The only thing setting.Repository.Editor.PreviewableFileModes is used for in the code is to tell the editor javascript code which file extensions to show a preview tab for.

@lunny
Copy link
Member

lunny commented Mar 13, 2023

I'm not sure what is not working exactly? For me the /markup API in mode file works for all [markup. xxxx].

[repository.editor] PREVIEWABLE_FILE_MODES only affects if the preview tab is displayed in the editor or not. The only thing setting.Repository.Editor.PreviewableFileModes is used for in the code is to tell the editor javascript code which file extensions to show a preview tab for.

This is my app.ini

[repository.editor]
PREVIEWABLE_FILE_MODES = comment, gfm, markdown, file

[markup.asciidoc]
ENABLED         = true
FILE_EXTENSIONS = .adoc,.asciidoc
RENDER_COMMAND  = asciidoctor -s -a showtitle --out-file=- -
; Input is not a standard input but a file
IS_INPUT_FILE   = false

And when I edit a file end with .asciidoc on the UI, the preview tab will not be displayed.

@brechtvl
Copy link
Contributor Author

Ah, it has to be like this with the current implementation:

PREVIEWABLE_FILE_MODES = markdown, asciidoc

That's how I understood it was meant to work. Though I'm unsure why this setting exists at all really, why the editor doesn't just display a preview for every extension it knows, same as when just viewing the file.

@brechtvl
Copy link
Contributor Author

brechtvl commented Mar 13, 2023

I guess this was meant to map to the mode for /markup? But if so I'm still unsure what the purpose of this is. If we want to control if a file is displayed with markdown or gfm for example (not sure why), an editor setting still seems like the wrong place for it.

@silverwind
Copy link
Member

why the editor doesn't just display a preview for every extension it knows, same as when just viewing the file.

Isn't preview always server-rendered, e.g. backed by a API that is exposed by the configurable markup renderers? At least I don't think we use any kind of preview functionality of Monaco itself, and I'm not aware that it has any.

@lunny
Copy link
Member

lunny commented Mar 14, 2023

Ah, it has to be like this with the current implementation:

PREVIEWABLE_FILE_MODES = markdown, asciidoc

That's how I understood it was meant to work. Though I'm unsure why this setting exists at all really, why the editor doesn't just display a preview for every extension it knows, same as when just viewing the file.

This does work. I made a mistake to let PREVIEWABLE_FILE_MODES = file which should not be supported.
And for the default value, maybe we can have a break change. Defaultly all files markup supported should be allowed to preview except PREVIEWABLE_FILE_MODES given a special value. Currently it's markdown, maybe we can change it to empty.

@brechtvl
Copy link
Contributor Author

brechtvl commented Mar 14, 2023

Isn't preview always server-rendered, e.g. backed by a API that is exposed by the configurable markup renderers? At least I don't think we use any kind of preview functionality of Monaco itself, and I'm not aware that it has any.

It is always server-rendered.

And for the default value, maybe we can have a break change. Defaultly all files markup supported should be allowed to preview except PREVIEWABLE_FILE_MODES given a special value. Currently it's markdown, maybe we can change it to empty.

Could we remove PREVIEWABLE_FILE_MODESentirely? That's barely a breaking change, more of a bugfix in my opinion.

I can't think of a scenario where you want to have a preview when viewing the file, but then when you click edit not have a Preview tab available. It seems useless to me, and that's really the only thing this setting does.

@lunny
Copy link
Member

lunny commented Mar 14, 2023

Isn't preview always server-rendered, e.g. backed by a API that is exposed by the configurable markup renderers? At least I don't think we use any kind of preview functionality of Monaco itself, and I'm not aware that it has any.

It is always server-rendered.

And for the default value, maybe we can have a break change. Defaultly all files markup supported should be allowed to preview except PREVIEWABLE_FILE_MODES given a special value. Currently it's markdown, maybe we can change it to empty.

Could we remove PREVIEWABLE_FILE_MODESentirely? That's barely a breaking change, more of a bugfix in my opinion.

I can't think of a scenario where you want to have a preview when viewing the file, but then when you click edit not have a Preview tab available. It seems useless to me, and that's really the only thing this setting does.

I'm OK to remove the configuration item.

@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Mar 14, 2023
@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 Mar 14, 2023
@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 Mar 19, 2023
@brechtvl
Copy link
Contributor Author

I did implement what was suggested in that comment.

Please see the PR description and other comments for details on what the breaking change is, and why I think it's barely a breaking change at all and more of a bugfix.

@silverwind
Copy link
Member

Removing a broken setting is barely breaking, I agree. But it should be mentioned at least so people can remove it from their configs.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 24, 2023
@lunny lunny merged commit 84daddc into go-gitea:main Mar 24, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 24, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
@brechtvl brechtvl deleted the editor-preview-external-renderers branch May 20, 2023 12:48
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. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants