speedata / publisher

speedata Publisher - a professional database Publishing system
https://www.speedata.de/
GNU Affero General Public License v3.0
292 stars 36 forks source link

properly handle `/Lang` with `mainlanguage` #435

Closed pr-apes closed 1 year ago

pgundlach commented 1 year ago

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

I'll have a look

pr-apes commented 1 year ago

I reply to your issues (after https://github.com/speedata/publisher/issues/434#issuecomment-1254693867).

  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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

pgundlach commented 1 year ago

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)