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

Improved conversion from LaTeX to HTML/Unicode #841

Merged
merged 14 commits into from
Feb 26, 2016

Conversation

oscargus
Copy link
Contributor

Now the huge list of conversion symbols is used when converting from LaTeX to HTML and Unicode. I also added a field right-click menu item to convert from LaTeX to Unicode. This sort of works except that it doesn't handle text within $$ in a good way (remove the $$ and the symbol conversion works).

The huge diff is caused by reformatting the huge list, as the newly added entries was aligned differently compared to earlier on saving...

@oscargus oscargus added type: enhancement status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Feb 20, 2016
Assert.assertEquals("ı", layout.format("\\i"));
Assert.assertEquals("ı", layout.format("\\{i}"));
Assert.assertEquals("ıı", layout.format("\\i\\i"));
Assert.assertEquals("ı ı", layout.format("\\i \\i"));
Copy link
Member

Choose a reason for hiding this comment

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

Why does a HTMLChars() formatter format latex to unicode and not to HTML?
Is there something wrong with the implementation or is the naming just suboptimal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Historical reasons. Earlier the huge conversion list resided in that file, so adding a method "formatUnicode" was simple. Indeed there should (very soon) be a UnicodeToLatexConverter extracted... Doing this is steps...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, wrong answer. :-) The answer above holds for HTMLConverter...

I think HTMLChars format to HTML? Now, since there are named entities these are used, if not, the numerical are used.

Copy link
Member

Choose a reason for hiding this comment

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

Ah my mistake! Sorry....

@oscargus oscargus force-pushed the bigconversionlist branch 2 times, most recently from 1783e9d to aa1ad68 Compare February 20, 2016 12:24
@oscargus
Copy link
Contributor Author

I fixed the equation issue and added support for some more LaTeX text styles. For example, \textsuperscript and \textsubscript are supported for both HTML and OO.

This also means that the preview looks quite a bit better now.

@simonharrer
Copy link
Contributor

Please fix the failing tests. Then this can be merged in as well.

@simonharrer
Copy link
Contributor

Converters could be added to the Save Actions under the Formatter Interface as well.

@oscargus
Copy link
Contributor Author

oscargus commented Feb 22, 2016 via email

@oscargus
Copy link
Contributor Author

oscargus commented Feb 22, 2016 via email

@simonharrer simonharrer removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 22, 2016
@simonharrer
Copy link
Contributor

Ok, please add the label ready-for-review again when you are done with the other changes.

@oscargus oscargus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Feb 22, 2016
@oscargus
Copy link
Contributor Author

It turned out that it also made sense to move the layout formatters from export.layout to logic.layout, so the final(?) commit is handling that. Some minor refactoring had to be done, but nothing controversial.

@oscargus oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 22, 2016
@oscargus oscargus force-pushed the bigconversionlist branch 2 times, most recently from bf49de3 to c3526b8 Compare February 22, 2016 22:50
@oscargus
Copy link
Contributor Author

I'm giving up on this PR for now... Doesn't really make sense to add a JournalAbbreviationRepository argument to PdfImporter or MoveFileAction, although that is required if it is propagated (and now I've only gone two steps back...). Will be back after a week or so of holidays (true story, nothing to do with this)...

Accessing a global variable is a single formatter or passing an argument to hundreds of non-related classes to possibly be used in a single formatter? I'm not convinced it is worth it...

@oscargus oscargus removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 23, 2016
@simonharrer
Copy link
Contributor

The idea is that the GUI part can have globals, whereas the logic does not. With this in mind, the PdfImporter or MoveFileAction do not need to have this class, but can merely pass in the global variable if necessary. Does this make sense?

@oscargus
Copy link
Contributor Author

It makes sense from the perspective that I understand where to start/stop. :-)

@oscargus oscargus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 26, 2016
@simonharrer
Copy link
Contributor

👍 LGTM (but can you remove the unused imports, please?)

@oscargus
Copy link
Contributor Author

All(?) imports are now removed and I managed to sort out the PdfCleanup thing as well.

Feel free to merge. :-)

simonharrer added a commit that referenced this pull request Feb 26, 2016
Improved conversion from LaTeX to HTML/Unicode
@simonharrer simonharrer merged commit 69e4fe2 into JabRef:master Feb 26, 2016
@tobiasdiez
Copy link
Member

Sorry for being such a pain in the ass with this abbreviation thing...but one last remark: did you tried out the Pdf rename cleanup as a user? I think the dialog always passes a null repository to the cleanup worker which in the end would result in a NPE. Not sure through...

@oscargus oscargus deleted the bigconversionlist branch March 16, 2016 09:51
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 type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants