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

Change katex limits #27823

Merged
merged 3 commits into from
Oct 29, 2023
Merged

Change katex limits #27823

merged 3 commits into from
Oct 29, 2023

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Oct 28, 2023

Fixes #27812

Use higher defaults again but limit the input size.

grafik

Add MAX_CHARS.
@KN4CK3R KN4CK3R added the type/enhancement An improvement of existing functionality label Oct 28, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 28, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 28, 2023
@@ -15,18 +15,28 @@ export async function renderMath() {
import(/* webpackChunkName: "katex" */'katex/dist/katex.css'),
]);

const MAX_CHARS = 1000;
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want a setting for this like with Mermaid?

const {mermaidMaxSourceCharacters} = window.config;

Copy link
Member

@delvh delvh Oct 28, 2023

Choose a reason for hiding this comment

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

I'm not a fan of this addition.
Isn't 1000 chars too restrictive?
That's only four tweets, and there are many comments larger than that.
Even more so if they contain latex code which spikes char usage up tremendously.
Especially as that is per comment instead of per recognized latex string.

As I see it, we can either remove it, or we choose the value that GitLab for example uses.

Copy link
Member Author

Choose a reason for hiding this comment

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

GitLab uses 1000. Why do you think it's the whole comment? source is just the <code> block with the math code.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I should have expanded the code a little.
The el.textContent looked to me like it would be the whole comment, not the math block.

@@ -15,18 +15,28 @@ export async function renderMath() {
import(/* webpackChunkName: "katex" */'katex/dist/katex.css'),
]);

const MAX_CHARS = 1000;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I should have expanded the code a little.
The el.textContent looked to me like it would be the whole comment, not the math block.

@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 Oct 28, 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 Oct 29, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 29, 2023
@lunny lunny enabled auto-merge (squash) October 29, 2023 01:44
@lunny lunny merged commit 3c78cb8 into go-gitea:main Oct 29, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Oct 29, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 29, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 30, 2023
* giteaofficial/main:
  List all Debian package versions in `Packages` (go-gitea#27786)
  Fix merge base commit for fast-forwarded GitLab PRs (go-gitea#27825)
  Fix bad method call when deleting user secrets via API (go-gitea#27829)
  Change katex limits (go-gitea#27823)
  Dockerfile small refactor (go-gitea#27757)
  Use GitLab's squash_commit_sha when available (go-gitea#27824)
  [skip ci] Updated translations via Crowdin
  Upgrade xorm to 1.3.4 (go-gitea#27807)
@nschloe
Copy link

nschloe commented Oct 30, 2023

Just noting that there are math expressions out there that are more than a thousand characters long. Just hit

\begin{align*}
S^{\prime} & :=\left\{i\ \colon u_{2}(n^{x_{i}},n^{x_{i-1}})=O(n^{\frac{2}{3}(x_{i}+x_{i-1})})\textrm{ and }u_{2}(n^{x_{i+1}},n^{x_{i}})=O(n^{\frac{2}{3}(x_{i+1}+x_{i})})\right\},\\
S^{\prime}_{+} & :=\Big\{ i:u_{2}(n^{x_{i}},n^{x_{i-1}})=O(n^{\frac{2}{3}(x_{i}+x_{i-1})})\textrm{ and }u_{2}(n^{x_{i+1}},n^{x_{i}})=O(n^{x_{i}}),\textrm{ or }\\
 & \mathrel{\phantom{=}}{}\mathrel{\phantom{=}}{}\mathrel{\phantom{=}}{}\mathrel{\phantom{=}}{}u_{2}(n^{x_{i}},n^{x_{i-1}})=O(n^{x_{i}})\textrm{ and }u_{2}(n^{x_{i+1}},n^{x_{i}})=O(n^{\frac{2}{3}(x_{i+1}+x_{i})})\Big\} ,\\
S^{\prime}_{++} & :=\Big\{ i:u_{2}(n^{x_{i}},n^{x_{i-1}})=O(n^{x_{i}})\textrm{ and }u_{2}(n^{x_{i+1}},n^{x_{i}})=O(n^{x_{i}})\Big\} ,\textrm{ and}\\
S^{\prime}_{-} & :=\Big\{ i:u_{2}(n^{x_{i}},n^{x_{i-1}})=O(n^{\frac{2}{3}(x_{i}+x_{i-1})})\textrm{ and }u_{2}(n^{x_{i+1}},n^{x_{i}})=O(n^{x_{i+1}}),\textrm{ or }\\
 & \mathrel{\phantom{=}}{}\mathrel{\phantom{=}}{}\mathrel{\phantom{=}}{}\mathrel{\phantom{=}}{}u_{2}(n^{x_{i}},n^{x_{i-1}})=O(n^{x_{i-1}})\textrm{ and }u_{2}(n^{x_{i+1}},n^{x_{i}})=O(n^{\frac{2}{3}(x_{i+1}+x_{i})})\Big\} .
\end{align*}

(1108 chars.)

@KN4CK3R KN4CK3R deleted the enhancement-math-limits branch October 30, 2023 16:06
@KN4CK3R
Copy link
Member Author

KN4CK3R commented Oct 30, 2023

I'm sure there are long expressions but I'm not sure if Gitea comments are the correct place for them. In this case you could just split the block into two blocks with two lines.

@nschloe
Copy link

nschloe commented Oct 30, 2023

Those appear in the README, as a translation of a scientific article, not in comments. I have no control over the size of the math blocks.

@silverwind silverwind added the backport/v1.21 This PR should be backported to Gitea 1.21 label Nov 1, 2023
@silverwind
Copy link
Member

silverwind commented Nov 1, 2023

Happy to accept a PR that increases these limits further. As I understand it there should be some limit to prevent performance impact from malicious comments etc, no other reason for those limits.

GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Nov 1, 2023
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Nov 1, 2023
KN4CK3R added a commit that referenced this pull request Nov 1, 2023
Backport #27823 by @KN4CK3R

Fixes #27812

Use higher defaults again but limit the input size.


![grafik](https://github.com/go-gitea/gitea/assets/1666336/23cdf572-de30-4799-b9cf-ef386b1623b9)

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 27, 2024
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 backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

katex: increase maxExpand setting
6 participants