Skip to content

Commit

Permalink
Fix wrong "missing Script" error
Browse files Browse the repository at this point in the history 
This could happen if a shape is not found and substituted
  • Loading branch information
@jspitz
jspitz authored Aug 31, 2024
1 parent 1e5f963 commit 8b51b79
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions tex/polyglossia.sty
View file
Original file line number Diff line number Diff line change
Expand Up @@ -945,9 +945,11 @@
\use:c { #2familylatin }
#5
{
\__xpg_add_font_feature_script:nee { #2 }
{ \prop_item:Nn \l_xpg_langsetup_prop { #1 / scripttag } }
{ \prop_item:Nn \l_xpg_langsetup_prop { #1 / script } }
\fontspec_font_if_exist:nT{\fontname\font}{

This comment has been minimized.

Sign in to view

Copy link
@Udi-Fogiel

Udi-Fogiel Sep 1, 2024 •
edited
Loading

Collaborator

This line produce an error, the macro \fontspec_font_if_exist:nT expects a fontspec syntax, not the engine/luaotfload one. What is this commit trying to fix?

Here is an simple example of the error

\documentclass{article}
\usepackage{fontspec}
\setmainfont{NewCM10}[%
  UprightFont = *-Regular,
  BoldFont = *-Bold,
  Extension = .otf,
  ItalicFont = *-Italic,
  BoldItalicFont = *-BoldItalic]
\begin{document}
\ExplSyntaxOn
\fontspec_font_if_exist:nT{\fontname\font}
\end{document}

This comment has been minimized.

Sign in to view

Copy link
@Udi-Fogiel

Udi-Fogiel Sep 1, 2024

Collaborator

Is the problem is that the Japanese mono font does not have the CJK script? If we don't eant to throw an error in this case, maybe we can switch to a warning instead?

This comment has been minimized.

Sign in to view

Copy link
@jspitz

jspitz Sep 4, 2024

Author Collaborator

No, the problem is that a certain shape of the font is not available, which is substituted, and then polyglossia attempts to apply the CJK script to the missing font and falsely reports that the font is missing the script. If you pass the script directly via option, all is well.

I still think the fix is conceptually right (and needed), and it worked well for me. I would have

This comment has been minimized.

Sign in to view

Copy link
@Udi-Fogiel

Udi-Fogiel Sep 4, 2024 •
edited
Loading

Collaborator

No, the problem is that a certain shape of the font is not available, which is substituted, and then polyglossia attempts to apply the CJK script to the missing font and falsely reports that the font is missing the script.

I don't think this is what happens. the macro \__xpg_add_font_feature_script:nnn that throws the error tries to add the script to the current selected font (at least that is what fontspec documented), it is not aware of the substitution which happens before it is expanded.

The construction you used, \fontname\font, is not aware of the substitution as well.

\documentclass{article}
\usepackage{fontspec}
\setmainfont{David CLM}
\begin{document}
\fontname\font

\scshape ABC

\fontname\font

\itshape ABC

\fontname\font
\end{document}

So I don't see how the test \fontspec_font_if_exist:nT{\fontname\font} is useful (when it works it is true).

If you pass the script directly via option, all is well.

I'm not sure what do you mean by that. Can you check that the font used actually
has the CJK script with the otfinfo tool? Running otfinfo -s path/to/fontfile should list all the available scripts of the font.

I still think the fix is conceptually right (and needed), and it worked well for me. I would have

DId you run the example I gave with LuaLaTeX?

\documentclass{article}
\usepackage{fontspec}
\setmainfont{NewCM10}[%
  UprightFont = *-Regular,
  BoldFont = *-Bold,
  Extension = .otf,
  ItalicFont = *-Italic,
  BoldItalicFont = *-BoldItalic]
\begin{document}
\ExplSyntaxOn
\fontspec_font_if_exist:nT{\fontname\font}
\end{document}

This comment has been minimized.

Sign in to view

Copy link
@Udi-Fogiel

Udi-Fogiel Sep 6, 2024

Collaborator

@jspitz You were partially correct... The macro \fontspec_if_script was testing against the wrong font if there was any substitution and LuaTeX was used. See latex3/fontspec#530.

This comment has been minimized.

Sign in to view

Copy link
@jspitz

jspitz Sep 7, 2024

Author Collaborator

I'll let you deal with this ...

This comment has been minimized.

Sign in to view

Copy link
@jspitz

jspitz Sep 8, 2024

Author Collaborator

@jspitz You were partially correct... The macro \fontspec_if_script was testing against the wrong font if there was any substitution and LuaTeX was used. See latex3/fontspec#530.

Assuming this PR gets accepted over at fontspec, is there anything left to do in polyglossia, or is the fontspec fix sufficient?

This comment has been minimized.

Sign in to view

Copy link
@Udi-Fogiel

Udi-Fogiel Sep 8, 2024

Collaborator

Assuming this PR gets accepted over at fontspec, is there anything left to do in polyglossia, or is the fontspec fix sufficient?

The fontspec fix is sufficient. But, from the look of if fontspec's repository did not had much activity lately.
If you want to do a release soon maybe we can include our own simple version of \fontspec_if_script, at least until it is fixed on the fonspec side. It should be easy enough.

This comment has been minimized.

Sign in to view

Copy link
@Udi-Fogiel

Udi-Fogiel Sep 8, 2024

Collaborator

Although for example luaotfload did not yet release a fix for #650 and it seems fine, so maybe we should not bother with it.

This comment has been minimized.

Sign in to view

Copy link
@jspitz

jspitz Sep 9, 2024

Author Collaborator

The fontspec fix is sufficient. But, from the look of if fontspec's repository did not had much activity lately. If you want to do a release soon maybe we can include our own simple version of \fontspec_if_script, at least until it is fixed on the fonspec side. It should be easy enough.

Could you do that? I think the bug is annoying enough (also considering the LyX side of things)

This comment has been minimized.

Sign in to view

Copy link
@Udi-Fogiel

Udi-Fogiel Sep 9, 2024

Collaborator

Could you do that? I think the bug is annoying enough (also considering the LyX side of things)

Done at c840b78, please test.

This comment has been minimized.

Sign in to view

Copy link
@jspitz

jspitz Sep 9, 2024

Author Collaborator

Could you do that? I think the bug is annoying enough (also considering the LyX side of things)

Done at c840b78, please test.

Works for me with both engines. Thanks!
\__xpg_add_font_feature_script:nee { #2 }
{ \prop_item:Nn \l_xpg_langsetup_prop { #1 / scripttag } }
{ \prop_item:Nn \l_xpg_langsetup_prop { #1 / script } }
}
}
}
}
Expand Down

0 comments on commit 8b51b79

Please sign in to comment.