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

Replace <p> in localization by \n #7279

Merged
merged 39 commits into from
Jun 7, 2021
Merged

Replace <p> in localization by \n #7279

merged 39 commits into from
Jun 7, 2021

Conversation

koppor
Copy link
Member

@koppor koppor commented Dec 31, 2020

Triggered by #7232 (comment), I saw that we did not cover the RuntimeExceptions by test cases. This PR adds them.

This PR also adds a check for \n in Localization strings. I do not now anything about handling <br> and \n in JavaFX. It seems, that we cannot have line breaks in either way. Maybe, I should document that somewhere.

  • Change in CHANGELOG.md described (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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@Siedlerchr
Copy link
Member

I think only in the search the formatting html tool tip are replaced with javafx formatting for bold etc
Line breaks using \n must be outside the Localization.lang calls
e.g Localization.lang("first line") +\n + Localization.lang("second line")

@Siedlerchr
Copy link
Member

But according to this thread https://stackoverflow.com/a/10285574/3450689

They should also work in the keys

@koppor
Copy link
Member Author

koppor commented Jan 1, 2021

I think only in the search the formatting html tool tip are replaced with javafx formatting for bold etc
Line breaks using \n must be outside the Localization.lang calls
e.g Localization.lang("first line") +\n + Localization.lang("second line")

We should never split up lines of localizations into different keys. Thank you for the \n pointer in the other message. I'll update the PR accordingly.

@koppor
Copy link
Member Author

koppor commented Jan 1, 2021

Intermediate report: We had <p> hardcorded and replaced it by \n at

genericDescription = genericDescription.replace("<p>", "\n");
.

I am trying to dig deeper there.

@koppor koppor changed the title Add tests for RuntimeException (and rewrite to ParameterizedTests) Replace <p> in localization by \n Jan 1, 2021
@Siedlerchr
Copy link
Member

Siedlerchr commented Jan 1, 2021

We should only keep the HTML tags for bold, italic and monospaced (See ToolTipTextUtil) because otherwise we can't indicate if the text is bold or not. As Javafx does not have HTML support, we must then use a TextFlow element with the CSS.

@koppor koppor marked this pull request as draft January 2, 2021 00:43
- Remove obsolete translation keys
- Make use of <tt> feature of ToolTipTextUtil
- Introduce special constructors for the .java and the .properties case
- Fix escaping
- Remove nested sub class
@tobiasdiez
Copy link
Member

It may also worthwhile to explore if the language keys still need to be escaped at all. If I remember correctly, that's actually not the case any longer.

@koppor koppor assigned koppor and unassigned calixtus Jun 6, 2021
@koppor
Copy link
Member Author

koppor commented Jun 7, 2021

Current result:

grafik

@koppor koppor added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels Jun 7, 2021
@koppor koppor marked this pull request as ready for review June 7, 2021 00:45
@koppor koppor requested a review from calixtus June 7, 2021 00:46
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Two questions.
Everything else looks good enough to me. ;-)

src/main/java/org/jabref/gui/util/TooltipTextUtil.java Outdated Show resolved Hide resolved
@calixtus
Copy link
Member

calixtus commented Jun 7, 2021

Btw.: Error: eckstyle] [ERROR] /home/runner/work/jabref/jabref/src/test/java/org/jabref/gui/search/ContainsAndRegexBasedSearchRuleDescriberTest.java:3:8: Unused import - java.util.Arrays. [UnusedImports]

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 7, 2021
@calixtus calixtus merged commit ffd623d into main Jun 7, 2021
@calixtus calixtus deleted the add-exception-test branch June 7, 2021 21:10
Siedlerchr added a commit that referenced this pull request Jun 8, 2021
* upstream/main:
  Refactoring existing unit tests into parametrized tests (#7700)
  Replace <p> in localization by \n (#7279)
  Bump byte-buddy-parent from 1.11.0 to 1.11.1 (#7800)
  Bump org.javamodularity.moduleplugin from 1.8.6 to 1.8.7 (#7799)
  Bump mockito-core from 3.10.0 to 3.11.0 (#7801)
  Bump classgraph from 4.8.106 to 4.8.108 (#7802)
  Update .markdownlint.yml
  Ignore slant.co links
  GitBook: [main] 3 pages and 17 assets modified
  GitBook: [main] one page modified
  GitBook: [main] one page modified
  GitBook: [main] 6 pages and 42 assets modified

# Conflicts:
#	build.gradle
@calixtus
Copy link
Member

grafik

ManageStudyDefinitionView.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants