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

Add LTR script kerning to DFLT for InDesign default composer #787

Merged
merged 2 commits into from Oct 19, 2023

Conversation

belluzj
Copy link
Collaborator

@belluzj belluzj commented Oct 19, 2023

Hello, this is an Adobe-specific "fix" for the following situation: in InDesign, using the default paragraph composer (not World-ready), and having no language selected, InDesign will not bother detecting the actual script of the text runs, and use the appropriate kerning; instead it sticks to only what is registered in the DFLT script.

With the split by script kerning, there's very little kerning under DFLT and it looks as though the font is unkerned.

In this PR I'm registering all the LTR script lookups under DFLT, so that InDesign applies that kerning even when no language is selected. The idea is that for RTL scripts you need to switch to the "good" composer anyway, so no need to bother.

@anthrotype
Copy link
Member

it looks as though the font is unhinted.

un-kerned you mean?

In the old kernFeatureWriter we had more complex logic as to what and when to use DFLT script for which kern/dist lookup:

scriptGroups = self.context.scriptGroups
if "dist" in self.context.todo:
distScripts = scriptGroups["dist"]
else:
distScripts = {}
kernScripts = scriptGroups.get("kern", {})
ltrScripts = kernScripts.get("LTR", [])
rtlScripts = kernScripts.get("RTL", [])
ltrLookups = lookups.get("LTR")
rtlLookups = lookups.get("RTL")
if ltrLookups and rtlLookups:
if ltrScripts and rtlScripts:
for script, langs in ltrScripts:
ast.addLookupReferences(feature, ltrLookups, script, langs)
for script, langs in rtlScripts:
ast.addLookupReferences(feature, rtlLookups, script, langs)
elif ltrScripts:
ast.addLookupReferences(feature, rtlLookups, script="DFLT")
for script, langs in ltrScripts:
ast.addLookupReferences(feature, ltrLookups, script, langs)
elif rtlScripts:
ast.addLookupReferences(feature, ltrLookups, script="DFLT")
for script, langs in rtlScripts:
ast.addLookupReferences(feature, rtlLookups, script, langs)
else:
if not (distScripts.get("LTR") and distScripts.get("RTL")):
raise ValueError(
"cannot use DFLT script for both LTR and RTL kern "
"lookups; add 'languagesystems' to features for at "
"least one LTR or RTL script using the kern feature"
)
elif ltrLookups:
if not (rtlScripts or distScripts):
ast.addLookupReferences(feature, ltrLookups)
else:
ast.addLookupReferences(feature, ltrLookups, script="DFLT")
for script, langs in ltrScripts:
ast.addLookupReferences(feature, ltrLookups, script, langs)
elif rtlLookups:
if not (ltrScripts or distScripts):
ast.addLookupReferences(feature, rtlLookups)
else:
ast.addLookupReferences(feature, rtlLookups, script="DFLT")
for script, langs in rtlScripts:
ast.addLookupReferences(feature, rtlLookups, script, langs)

whereas here you seem to unilaterally opt for LTR kern in DFLT.. Could we port some of that logic in here?

@anthrotype
Copy link
Member

The idea is that for RTL script you need to switch to the "good" composer anyway, so no need to bother.

I imagine it's not just indesign that uses DFLT when no explicit script/language is requested, that's what DFLT is supposed to be meant for, so the issue that we leave DFLT unkerned is more general than Indesign itself.

@belluzj
Copy link
Collaborator Author

belluzj commented Oct 19, 2023

Yes, un-kerned!

@anthrotype
Copy link
Member

at least we could implement the simple unequivocal cases when there is no doubt what DFLT can contain, if only LTR or RTL are present. I do see what you mean that "for RTL script you need to switch to the "good" composer anyway, so no need to bother", so I'd be inclined to accept that view that LTR be the default (also because we have no way to specify what the user intention is in the source)

@belluzj
Copy link
Collaborator Author

belluzj commented Oct 19, 2023

OK, I'll read through the old feature writer and see how I could apply the same logic to the new one

@anthrotype
Copy link
Member

anthrotype commented Oct 19, 2023

in ambiguous cases the old writer would bail out with an error, arguably too harsh. Besides, having re-read that code, it looks like it is not doing what you want, i.e. ensuring that DFLT contains at least the LTR stuff; when langsyses contain both LTR and RTL, and we produce both LTR and RTL lookups, it is only registering under the respective scripts and not adding anything to DFTL; when langsyses contain one set of scripts and not the other one, and we have two kinds of lookups to register, it adds to DFLT the lookups for which we have no explict langsys definitions (which is kind of the opposite of what you want); finally, when there's only one set of lookups, it always registers them under the relevant langsys (if any) and also always under DFLT. I think only the latter part of the code is worth carrying over to the new writer. In addition, we can keep this proposed behaviour to default to LTR in DFLT when when have mixed LTR and RTL scripts/lookups, with the understanding that RTL being more 'advanced' for "dumb" text applications and requires one to define a RTL language for full OTL support.

@belluzj
Copy link
Collaborator Author

belluzj commented Oct 19, 2023

@anthrotype I'm with you on what the old writer was doing (= not exactly what I want: I want to always have kerning in DFLT, even if the feature file has all the languagesystem declarations)

I think only the latter part of the code is worth carrying over to the new writer.

Is this what you propose:

Currently in my PR:

  • gather LTR lookups and register them under DFLT

Your proposed change:

  • gather LTR lookups
    • if any, register them under DFLT
    • else gather RTL lookups and register them under DFLT

Basically default to registering LTR lookups, but if there are none, register the RTL lookups instead?

Or is it something else?

@anthrotype
Copy link
Member

Yes sound good to me

@belluzj
Copy link
Collaborator Author

belluzj commented Oct 19, 2023

@anthrotype I've applied your feedback

@belluzj
Copy link
Collaborator Author

belluzj commented Oct 19, 2023

Should I leave a day or two before merging to check if anyone else has objections to this? Or does it seem straightforward enough?

@anthrotype anthrotype merged commit efb4658 into main Oct 19, 2023
9 checks passed
@anthrotype anthrotype deleted the fix-indesign-no-language-kerning branch October 19, 2023 14:36
@anthrotype
Copy link
Member

anthrotype commented Oct 19, 2023

nah, it's fine. We can always pin a version or revert as needed

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.

None yet

2 participants