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

Group names can now be rendered using LaTeXs commands #1684

Merged
merged 2 commits into from
Aug 8, 2016

Conversation

oscargus
Copy link
Contributor

@oscargus oscargus commented Aug 5, 2016

Inspired by #1681

  • Change in CHANGELOG.md described
  • Manually tested changed features in running JabRef

@oscargus oscargus added [outdated] type: enhancement component: ui status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers component: groups labels Aug 5, 2016
@tobiasdiez
Copy link
Member

tobiasdiez commented Aug 5, 2016

Mhh, I think this is the wrong place to add this functionality. If I name my group \relax (or \beta) then I probably want to keep this name.
But I think it is a good idea to add the Latex-to-Unicode-formatter to determine the name when groups are automatically created.

@oscargus
Copy link
Contributor Author

oscargus commented Aug 5, 2016 via email

@stefan-kolb
Copy link
Member

Do we really need to allow latex code for group names? Why is normal text not enough? I'd rather not have this option and keep it simple.

@oscargus
Copy link
Contributor Author

oscargus commented Aug 7, 2016

See #1681. Names containing accents is the main reason. Already now, the names can contain LaTeX code (except that the reading is flawed, again see #1681). It's just that they are properly rendered now. I do not really see the problem.

@tobiasdiez
Copy link
Member

tobiasdiez commented Aug 8, 2016

For me the problem is that the Latex -> Unicode converter is not good enough to handle arbitrary text and I think we may run into problems where names of groups are not properly rendered (i.e. "A or B" in the form "A\B", or similar things with backslash). As a user, I kind of expect that JabRef takes what I give it as a name (especially since I can provide unicode strings, I don't see the reason to support latex accents). Moreover, we might run into big performance problems with groups again.

The idea is nonetheless good. Just put the conversion code in the automatic generation of groups (since there the user has no control over the names of the groups).

@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 8, 2016
@oscargus
Copy link
Contributor Author

oscargus commented Aug 8, 2016

I see your point, although I'm not fully agreeing with it (I'd rather write $\Delta\Sigma$ than look up the Unicode key and writing \ in anything related to LaTeX expecting it to stay a backslash seems questionable... ;-)), but you solution is fair enough and leads to more or less the same end result. Only question is if BibTeX is bothered by Unicode characters in fields that are ignored anyway? (Yes, there are good reasons to use BibTeX for many people.)

Could you have a look at why the backslashes are removed when reading back the group definitions? (Since you have better insight into the group package.)

@oscargus
Copy link
Contributor Author

oscargus commented Aug 8, 2016

I changed it to do the conversion upon generation of automatic groups (and correspondingly changed the CHANGELOG entry).

@@ -73,6 +74,8 @@
private final JabRefFrame frame;
private final BasePanel panel;

private static final LatexToUnicodeFormatter FORMATTER = new LatexToUnicodeFormatter();
Copy link
Member

Choose a reason for hiding this comment

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

I would make the formatter a normal variable (initialized directly before the for loop below) instead of static final.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it like that first. ;-)

@tobiasdiez
Copy link
Member

Ok, just make the fomatter a normal variable and then this can be merged. I will have a look at the serialization / parsing problem as soon as I'm back in Germany (i.e. in a month)

@oscargus oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 8, 2016
@oscargus oscargus merged commit 086aa37 into JabRef:master Aug 8, 2016
@oscargus oscargus deleted the propergroupnames branch August 8, 2016 12:29
Siedlerchr added a commit to Siedlerchr/jabref that referenced this pull request Aug 9, 2016
* master: (22 commits)
  Do not cite entries without a key in OpenOffice/LibreOffice (JabRef#1682) (JabRef#1683)
  Got rid of unused preferences (JabRef#1672)
  Move labelpattern (JabRef#1626)
  Implementation of shared database support (full system). (JabRef#1451)
  Removed bst from architecture tests JabRef#1699
  Update PULL_REQUEST_TEMPLATE.md
  French localization: Menu: Translation of an empty string
  French localization: Jabref_fr: empty strings translated
  Updated string similarity version (JabRef#1698)
  Minify description of release process
  Announce switch from GPL to MIT in CONTRIBUTING.md and README.md
  Some cleanups to initialize empty MetaData at fewer positions (JabRef#1696)
  Automatic group names are converted from LaTeX to Unicode (JabRef#1684)
  Update ISSUE_TEMPLATE.md (JabRef#1686)
  Removed (false) NPE issue reported by FindBugs
  More Swedish translations (JabRef#1691)
  Updated wiremock version (JabRef#1690)
  Some minor code cleanups (JabRef#1689)
  Removed dependencies of Globals.prefs in some tests (JabRef#1688)
  Lookup BibEntry from ISBN and merge information (JabRef#1621)
  ...

# Conflicts:
#	src/main/java/net/sf/jabref/gui/BasePanel.java
#	src/main/java/net/sf/jabref/importer/ImportMenuItem.java
#	src/main/resources/l10n/JabRef_da.properties
#	src/main/resources/l10n/JabRef_de.properties
#	src/main/resources/l10n/JabRef_en.properties
#	src/main/resources/l10n/JabRef_es.properties
#	src/main/resources/l10n/JabRef_fa.properties
#	src/main/resources/l10n/JabRef_fr.properties
#	src/main/resources/l10n/JabRef_in.properties
#	src/main/resources/l10n/JabRef_it.properties
#	src/main/resources/l10n/JabRef_ja.properties
#	src/main/resources/l10n/JabRef_nl.properties
#	src/main/resources/l10n/JabRef_no.properties
#	src/main/resources/l10n/JabRef_pt_BR.properties
#	src/main/resources/l10n/JabRef_ru.properties
#	src/main/resources/l10n/JabRef_sv.properties
#	src/main/resources/l10n/JabRef_tr.properties
#	src/main/resources/l10n/JabRef_vi.properties
#	src/main/resources/l10n/JabRef_zh.properties
ayanai1 pushed a commit to ayanai1/jabref that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: groups component: ui [outdated] type: enhancement 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.

3 participants