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

Fixes #27605: inline math blocks can't be preceeded/followed by alphanumerical characters #30175

Conversation

jmlt2002
Copy link
Contributor

@jmlt2002 jmlt2002 commented Mar 29, 2024

  • Inline math blocks couldn't be preceeded or succeeded by alphanumerical characters due to changes introduced in PR Fix slight bug in katex #21171. Removed the condition that caused this (precedingCharacter condition) and added a new exit condition of the for-loop that checks if a specific '$' was escaped using '' so that the math expression can be rendered as intended.
  • Additionally this PR fixes another bug where math blocks of the type '$xyz$abc$' where the dollar sign was not escaped by the user, generated an error (shown in the screenshots below)
  • Altered the tests to accomodate for the changes

Former behaviour (from try.gitea.io):
image

Fixed behaviour (from my local build):
image

(Edit) Source code for the README.md file:

$x$ -$x$ $x$-

a$xa$ $xa$a 1$xb$ $xb$1

$a a$b b$

a$b $a a$b b$

$a a\$b b$

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 29, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 29, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 29, 2024
… alphanumerical characters

Inline math blocks couldn't be preceeded or succeeded by alphanumerical characters due to changes introduced in PR go-gitea#21171. Removed the condition that caused this (precedingCharacter) and added a new if statement that checks if a specific '$' was escaped using '\'.
Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
@jmlt2002 jmlt2002 force-pushed the fix-inline-math-blocks-preceeded-followed-alphanumeric-chars branch from c75fa56 to 1edf43a Compare March 29, 2024 14:28
@silverwind silverwind added the backport/v1.22 This PR should be backported to Gitea 1.22 label Mar 29, 2024
@silverwind
Copy link
Member

I tested what GitHub renders for your example and only the first one works. Would you say those are bugs in GitHub's implementation?

image

@silverwind
Copy link
Member

silverwind commented Mar 29, 2024

Also, could you add a few more test cases, instead of only modifying the existing ones.

Added one test case to check that math blocks render correctly when preceeded by an alphanumerical character, and another one for when it is succeeded by an alphanumerical character.

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
@jmlt2002
Copy link
Contributor Author

I tested what GitHub renders for your example and only the first one works. Would you say those are bugs in GitHub's implementation?
image

I think so since I don't see a particular reason why it wouldn't be allowed for characters to come directly before or after the math expressions.

@silverwind
Copy link
Member

silverwind commented Mar 29, 2024

Seems like GitLab rendering does agree with your solution, so 👍 from me on the rendering part.

image

@silverwind silverwind added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Mar 29, 2024
@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 29, 2024
@jmlt2002
Copy link
Contributor Author

Seems like GitLab rendering does agree with your solution, so 👍 from me on the rendering part.
image

We missed it but it seems that GitLab does not agree with my solution after all, the last expression of the second line is not rendered, and the rendering in the fourth line does not match

…ical characters

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
@jmlt2002 jmlt2002 force-pushed the fix-inline-math-blocks-preceeded-followed-alphanumeric-chars branch from fff63a7 to 88f41f3 Compare March 31, 2024 16:19
@wxiaoguang
Copy link
Contributor

TBH, I still have question about why it needs this change.

Why it needs to handle $a a$b b$ / a$x$ that way? Why "inline math blocks should be preceeded/followed by alphanumerical characters"?

By reading #27605, I didn't get the answer, and I haven't seen a real world example yet ....

Could you elaborate the use cases by real examples?

@jmlt2002
Copy link
Contributor Author

jmlt2002 commented Apr 1, 2024

For the a$x$ expression you could have for example something of the type a$x²$ where 'a' is a constant. If 'a' was inside the math expression it would have the same formatting as 'x²' making it less readable.

As for the $a a$b b$, it's an extra, it's a side effect of changing the render as mentioned above. Before, this expression would generate an error, as shown in the first comment of the conversation, and now it no longer does that.

@wxiaoguang
Copy link
Contributor

For the a$x$ expression you could have for example something of the type a$x²$ where 'a' is a constant. If 'a' was inside the math expression it would have the same formatting as 'x²' making it less readable.

Why people would write such expr a$x²$? I think it is still an over-abstracted example, but not a real world example. I used some examples above like The price is between $1 and $10, because people really write that way. But for a$x²$ , could you show some real world documents/comments that why it is written?

As for the $a a$b b$, it's an extra, it's a side effect of changing the render as mentioned above. Before, this expression would generate an error, as shown in the first comment of the conversation, and now it no longer does that.

Yup, I agree that it is just a side-effect of this change. The root question is still the first one.

@jmlt2002
Copy link
Contributor Author

jmlt2002 commented Apr 1, 2024

But for a$x²$ , could you show some real world documents/comments that why it is written?

I looked at some websites and math research papers and it seems that the math expressions are never directly preceeded by a character, they are either a whole mathematical block or have a space in between like a $x$.

I will change the code to not allow either preceeding or suceeding characters, and solely fix the bug that was fixed by as a side-effect.

@jmlt2002
Copy link
Contributor Author

jmlt2002 commented Apr 1, 2024

Should the math expressions not allowed to be followed by alphanumerical characters or also not allowed to be followed by any character besides ' ' or a newline?

@wxiaoguang
Copy link
Contributor

IMO following by a punctuation or newline is fine, because that's also how a sentence is written. GitHub:

  • It is $1$, or maybe $2$. => It is $1$, or maybe $2$.

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 1, 2024
{
`$a a$b b$`,
`<p>$a a<code class="language-math is-loading">b b</code></p>` + nl,
`<p>$a a$b b$</p>` + nl,
Copy link
Contributor

@wxiaoguang wxiaoguang Apr 2, 2024

Choose a reason for hiding this comment

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

Could there be a case for something like:

  • $a a\\$b b$ => $a a\$b b$

?

In case the users would like to escape and use $

I have this question because I guess the old code might work with $a a\\$b b$ (although it reports errors for $a a$b b$, it is still acceptable)

So I'd like to confirm the new code's behavior.

Copy link
Contributor Author

@jmlt2002 jmlt2002 Apr 2, 2024

Choose a reason for hiding this comment

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

The old code does what you described with $a a\$b b$, with only one backslash. With two, or any even number of backslashes it would generate an error.

With the current code, the expression $a a\\$b b$ generates the expression below.
image

If the user wanted to get the output that you described he would need to use the following expression
$a a \$ b b$
image

@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 Apr 2, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 2, 2024
@silverwind silverwind enabled auto-merge (squash) April 2, 2024 18:14
@silverwind silverwind merged commit e006451 into go-gitea:main Apr 2, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Apr 2, 2024
@silverwind silverwind removed the backport/v1.21 This PR should be backported to Gitea 1.21 label Apr 2, 2024
GiteaBot added a commit to GiteaBot/gitea that referenced this pull request Apr 2, 2024
…by alphanumerical characters (go-gitea#30175)

- Inline math blocks couldn't be preceeded or succeeded by
alphanumerical characters due to changes introduced in PR go-gitea#21171.
Removed the condition that caused this (precedingCharacter condition)
and added a new exit condition of the for-loop that checks if a specific
'$' was escaped using '\' so that the math expression can be rendered as
intended.
- Additionally this PR fixes another bug where math blocks of the type
'$xyz$abc$' where the dollar sign was not escaped by the user, generated
an error (shown in the screenshots below)
- Altered the tests to accomodate for the changes

Former behaviour (from try.gitea.io):

![image](https://github.com/go-gitea/gitea/assets/114936010/8f0cbb21-321d-451c-b871-c67a8e1e9235)

Fixed behaviour (from my local build):

![image](https://github.com/go-gitea/gitea/assets/114936010/5c22687c-6f11-4407-b5e7-c14b838bc20d)

(Edit) Source code for the README.md file:
```
$x$ -$x$ $x$-

a$xa$ $xa$a 1$xb$ $xb$1

$a a$b b$

a$b $a a$b b$

$a a\$b b$
```

---------

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
Co-authored-by: Giteabot <teabot@gitea.io>
GiteaBot added a commit to GiteaBot/gitea that referenced this pull request Apr 2, 2024
…by alphanumerical characters (go-gitea#30175)

- Inline math blocks couldn't be preceeded or succeeded by
alphanumerical characters due to changes introduced in PR go-gitea#21171.
Removed the condition that caused this (precedingCharacter condition)
and added a new exit condition of the for-loop that checks if a specific
'$' was escaped using '\' so that the math expression can be rendered as
intended.
- Additionally this PR fixes another bug where math blocks of the type
'$xyz$abc$' where the dollar sign was not escaped by the user, generated
an error (shown in the screenshots below)
- Altered the tests to accomodate for the changes

Former behaviour (from try.gitea.io):

![image](https://github.com/go-gitea/gitea/assets/114936010/8f0cbb21-321d-451c-b871-c67a8e1e9235)

Fixed behaviour (from my local build):

![image](https://github.com/go-gitea/gitea/assets/114936010/5c22687c-6f11-4407-b5e7-c14b838bc20d)

(Edit) Source code for the README.md file:
```
$x$ -$x$ $x$-

a$xa$ $xa$a 1$xb$ $xb$1

$a a$b b$

a$b $a a$b b$

$a a\$b b$
```

---------

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
Co-authored-by: Giteabot <teabot@gitea.io>
@silverwind
Copy link
Member

silverwind commented Apr 2, 2024

Guess 1.22 backport is enough. Edit: Nevermind, GiteaBot was faster.

@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Apr 2, 2024
@silverwind silverwind removed the backport/v1.22 This PR should be backported to Gitea 1.22 label Apr 2, 2024
HenriquerPimentel pushed a commit to HenriquerPimentel/gitea that referenced this pull request Apr 2, 2024
…by alphanumerical characters (go-gitea#30175)

- Inline math blocks couldn't be preceeded or succeeded by
alphanumerical characters due to changes introduced in PR go-gitea#21171.
Removed the condition that caused this (precedingCharacter condition)
and added a new exit condition of the for-loop that checks if a specific
'$' was escaped using '\' so that the math expression can be rendered as
intended.
- Additionally this PR fixes another bug where math blocks of the type
'$xyz$abc$' where the dollar sign was not escaped by the user, generated
an error (shown in the screenshots below)
- Altered the tests to accomodate for the changes

Former behaviour (from try.gitea.io):

![image](https://github.com/go-gitea/gitea/assets/114936010/8f0cbb21-321d-451c-b871-c67a8e1e9235)

Fixed behaviour (from my local build):

![image](https://github.com/go-gitea/gitea/assets/114936010/5c22687c-6f11-4407-b5e7-c14b838bc20d)

(Edit) Source code for the README.md file:
```
$x$ -$x$ $x$-

a$xa$ $xa$a 1$xb$ $xb$1

$a a$b b$

a$b $a a$b b$

$a a\$b b$
```

---------

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
Co-authored-by: Giteabot <teabot@gitea.io>
techknowlogick pushed a commit that referenced this pull request Apr 3, 2024
…numerical characters (#30175) (#30250)

Backport #30175 by @jmlt2002

- Inline math blocks couldn't be preceeded or succeeded by
alphanumerical characters due to changes introduced in PR #21171.
Removed the condition that caused this (precedingCharacter condition)
and added a new exit condition of the for-loop that checks if a specific
'$' was escaped using '\' so that the math expression can be rendered as
intended.
- Additionally this PR fixes another bug where math blocks of the type
'$xyz$abc$' where the dollar sign was not escaped by the user, generated
an error (shown in the screenshots below)
- Altered the tests to accomodate for the changes

Former behaviour (from try.gitea.io):

![image](https://github.com/go-gitea/gitea/assets/114936010/8f0cbb21-321d-451c-b871-c67a8e1e9235)

Fixed behaviour (from my local build):

![image](https://github.com/go-gitea/gitea/assets/114936010/5c22687c-6f11-4407-b5e7-c14b838bc20d)

(Edit) Source code for the README.md file:
```
$x$ -$x$ $x$-

a$xa$ $xa$a 1$xb$ $xb$1

$a a$b b$

a$b $a a$b b$

$a a\$b b$
```

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
Co-authored-by: João Tiago <114936010+jmlt2002@users.noreply.github.com>
techknowlogick pushed a commit that referenced this pull request Apr 3, 2024
…numerical characters (#30175) (#30251)

Backport #30175 by @jmlt2002

- Inline math blocks couldn't be preceeded or succeeded by
alphanumerical characters due to changes introduced in PR #21171.
Removed the condition that caused this (precedingCharacter condition)
and added a new exit condition of the for-loop that checks if a specific
'$' was escaped using '\' so that the math expression can be rendered as
intended.
- Additionally this PR fixes another bug where math blocks of the type
'$xyz$abc$' where the dollar sign was not escaped by the user, generated
an error (shown in the screenshots below)
- Altered the tests to accomodate for the changes

Former behaviour (from try.gitea.io):

![image](https://github.com/go-gitea/gitea/assets/114936010/8f0cbb21-321d-451c-b871-c67a8e1e9235)

Fixed behaviour (from my local build):

![image](https://github.com/go-gitea/gitea/assets/114936010/5c22687c-6f11-4407-b5e7-c14b838bc20d)

(Edit) Source code for the README.md file:
```
$x$ -$x$ $x$-

a$xa$ $xa$a 1$xb$ $xb$1

$a a$b b$

a$b $a a$b b$

$a a\$b b$
```

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
Co-authored-by: João Tiago <114936010+jmlt2002@users.noreply.github.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 3, 2024
* giteaofficial/main:
  Refactor "dump" sub-command (go-gitea#30240)
  Add -u git to docs when using docker exec with root installation (go-gitea#29314)
  Show 12 lines in markup code preview (go-gitea#30255)
  Fixes go-gitea#27605: inline math blocks can't be preceeded/followed by alphanumerical characters (go-gitea#30175)
  Render embedded code preview by permlink in markdown (go-gitea#30234)
  Fix missing 0 prefix of GPG key id (go-gitea#30245)
  Fix spacing in issue navbar (go-gitea#30238)
  Add unique index for project_issue to prevent duplicate data (go-gitea#30190)
  [skip ci] Updated translations via Crowdin
  Refactor commit signature parser (go-gitea#30228)
  Refactor dropzone (go-gitea#30232)
  Remove scheduled action tasks if the repo is archived (go-gitea#30224)
  Refactor file view & render (go-gitea#30227)
  Refactor DeleteInactiveUsers, fix bug and add tests (go-gitea#30206)
  [skip ci] Updated licenses and gitignores
  Add `/options/license` and `/options/gitignore` to `.ignore` (go-gitea#30219)
AvengerMoJo pushed a commit to AvengerMoJo/gitea that referenced this pull request Apr 8, 2024
…by alphanumerical characters (go-gitea#30175)

- Inline math blocks couldn't be preceeded or succeeded by
alphanumerical characters due to changes introduced in PR go-gitea#21171.
Removed the condition that caused this (precedingCharacter condition)
and added a new exit condition of the for-loop that checks if a specific
'$' was escaped using '\' so that the math expression can be rendered as
intended.
- Additionally this PR fixes another bug where math blocks of the type
'$xyz$abc$' where the dollar sign was not escaped by the user, generated
an error (shown in the screenshots below)
- Altered the tests to accomodate for the changes

Former behaviour (from try.gitea.io):

![image](https://github.com/go-gitea/gitea/assets/114936010/8f0cbb21-321d-451c-b871-c67a8e1e9235)

Fixed behaviour (from my local build):

![image](https://github.com/go-gitea/gitea/assets/114936010/5c22687c-6f11-4407-b5e7-c14b838bc20d)

(Edit) Source code for the README.md file:
```
$x$ -$x$ $x$-

a$xa$ $xa$a 1$xb$ $xb$1

$a a$b b$

a$b $a a$b b$

$a a\$b b$
```

---------

Signed-off-by: João Tiago <joao.leal.tintas@tecnico.ulisboa.pt>
Co-authored-by: Giteabot <teabot@gitea.io>
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants