Skip to content

Fix HTML-reserved characters ('&', '<', etc) not rendered correctly in entry preview#10733

Merged
Siedlerchr merged 11 commits into
JabRef:mainfrom
HoussemNasri:some-characters-not-in-rendered-preview
Jan 23, 2024
Merged

Fix HTML-reserved characters ('&', '<', etc) not rendered correctly in entry preview#10733
Siedlerchr merged 11 commits into
JabRef:mainfrom
HoussemNasri:some-characters-not-in-rendered-preview

Conversation

@HoussemNasri

@HoussemNasri HoussemNasri commented Dec 31, 2023

Copy link
Copy Markdown
Member

Fix #10677

TODO

  • Add test case

Screenshots

image

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@HoussemNasri HoussemNasri force-pushed the some-characters-not-in-rendered-preview branch from 94359b2 to 078682b Compare December 31, 2023 18:59
calixtus
calixtus previously approved these changes Jan 1, 2024
@calixtus

calixtus commented Jan 1, 2024

Copy link
Copy Markdown
Member

Looks good so far, how about some simple tests?

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have one minor feedback (also via /review by Codium

Consider using a compiled Pattern for the regex in normalizedFiled method to improve performance.

private static final Pattern HTML_ENTITY_PATTERN = Pattern.compile("&(?!(?:[a-z0-9]+|#[0-9]{1,6}|#x[0-9a-fA-F]{1,6});)");

private String normalizedFiled(String inField) {
    // When we encounter a '&' we check if it starts an HTML entity then we skip it, otherwise
    // we should replace it with '&amp;' to be rendered correctly.
    return HTML_ENTITY_PATTERN.matcher(inField).replaceAll("&amp;") // Replace & with &amp; if it does not begin an HTML entity
                  .replaceAll("\\\\&", "&amp;") // Replace \& with &amp;
                  .replaceAll("[\\n]{2,}", "<p>") // Replace double line breaks with <p>
                  .replace("\n", "<br>") // Replace single line breaks with <br>
                  .replace("\\$", "&dollar;") // Replace \$ with &dollar;
                  .replaceAll("\\$([^$]*)\\$", "\\{$1\\}");
}

Two "major" things:

  1. Refactor HTMLCharsTest to use @ParamterizedTest. I think, with OpenAI, this is just an exercise how to use OpenAI ^^. -- That would also allow "easy" addition of the test covering the changed functionality.
  2. A comment that Apache Common's escapeHtml4 is not a smart escaping and does not handle LaTeX characters adn commands

@HoussemNasri HoussemNasri added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 18, 2024
@Siedlerchr Siedlerchr enabled auto-merge January 23, 2024 20:02
@Siedlerchr

Copy link
Copy Markdown
Member

Thanks for the follow up!

@Siedlerchr Siedlerchr requested a review from koppor January 23, 2024 20:02
@Siedlerchr Siedlerchr disabled auto-merge January 23, 2024 20:02
@Siedlerchr Siedlerchr merged commit 12486da into JabRef:main Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Markdown cannot parse &amp;&lt;&gt; to &<>

4 participants