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

properly handle /Lang with mainlanguage #435

Closed
wants to merge 3 commits into from
Closed

properly handle /Lang with mainlanguage #435

wants to merge 3 commits into from

Conversation

pr-apes
Copy link
Contributor

@pr-apes pr-apes commented Sep 14, 2022

No description provided.

@pgundlach
Copy link
Member

Thank you for the patch. I see a few issues here (which can be easily solved)

  • A layout.xml with <Options /> breaks (when no mainlanguage=".." is in the Options command)
  • Not sure if it necessary to introduce another variable sp_mainlanguage
  • The (default) language is written to the document even if it is unset. This is probably a good idea, but it breaks backwards compatibility. So if unset, it should be left out.

I'll have a look

@pr-apes
Copy link
Contributor Author

pr-apes commented Sep 22, 2022

I reply to your issues (after #434 (comment)).

  1. I hadn't checked that. Totally my fault.
  2. I thought it was clearer to have a sp_mainlanguage variable. And I'm not sure I know how to read the value from --mainlanguage for /Lang without it.
  3. Sorry, but I don't understand how adding the main document language to /Lang might break backwards compatibility.

Let me elaborate my last point. /Lang is metadata (in a very similar way as /Creator is metadata for the PDF document).

If I a newer version of Publisher is used with the same layout and data sources, /Creator will be different.

Sorry, but I don't think this breaks backwards compatibilty. Only the output in that given field isn't the same.

The document main language may be not set by the user, but Publisher defaults it to British English.

To the best of my knowledge, text is hyphenated according to the patterns for this language (unless deactivated).

In my opinion, adding the main language to PDF metadata doesn't break existing functionality in an incompatible way.

But of course, this is up to you. Let me know what you think about last two points.

@pgundlach
Copy link
Member

My third point is stupid. It will have byte changes in the documents but you are right that it will not change anything from the user perspective. So we should add it

I will take a look

@pr-apes
Copy link
Contributor Author

pr-apes commented Sep 22, 2022

Just as a comment, :match("%a+") solves the issue with language identifiers involving geographical regions.

if options.mainlanguage ~= nil and options.mainlanguage ~= "" then
    pdfcatalog[#pdfcatalog + 1] = string.format("/Lang (%s)", (language_mapping[options.mainlanguage] or options.mainlanguage):match("%a+"))
elseif sp_mainlanguage then
    pdfcatalog[#pdfcatalog + 1] = string.format("/Lang (%s)", (language_mapping[sp_mainlanguage] or sp_mainlanguage):match("%a+"))
end

@pr-apes
Copy link
Contributor Author

pr-apes commented Sep 22, 2022

Also fixed layout with options but not main language.

The culprit was

if options.mainlanguage ~= "" then

and this fixes it:

if options.mainlanguage ~= nil and options.mainlanguage ~= "" then

I will try to correct my own patches (not sure whether GH will allow me that).

@pr-apes
Copy link
Contributor Author

pr-apes commented Sep 22, 2022

I have to close this pull request to provide a new one.

@pr-apes pr-apes closed this Sep 22, 2022
@pgundlach
Copy link
Member

I believe you think too complicated.

IIRC you just need to put the /Lang field into the catalog with the field 'locale' of the lang object which you get with get_language(defaultlanguage) I think for a start it would be enough to have the first locale part.

No need to have an extra sp_mainlanguage or any other code in the commands.lua file
(I might be wrong though)

@pr-apes pr-apes mentioned this pull request Sep 22, 2022
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.

2 participants