-
Notifications
You must be signed in to change notification settings - Fork 594
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
Draft: Armenian Letters (again) #2556
Conversation
Can you try typeset some text for sample? Also when you take screenshot you could have higher resolution :) |
We may need some extra touching for |
Changed |
Nice, this looks a lot better. |
alias 'armn/Oh' 0x555 'O' | ||
alias 'armn/oh' 0x585 'o' | ||
|
||
do "Ayb" # Upper U Group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer remove move the empty "do"s here.
You can leave comments in letter/armenian.ptl
, indicating which letter lives in which file. For example
* U+0531 Ayb => letter/armenian/upper-u-group.ptl
* U+0532 Ben => letter/armenian/hook-group.ptl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file is a leftover to be cleaned up anyway, I'll just move all the actual aliases to their corresponding sources and remove this file.
@@ -217,6 +217,7 @@ glyph-block Letter-Latin-Lower-G : begin | |||
|
|||
select-variant 'cyrl/de.BGR' (shapeFrom -- 'g') (follow -- [conditional-follow SLAB 'g/single/autoSerifed/slab' 'g/single/autoSerifed/sans']) | |||
alias 'cyrl/de.SRB' null 'cyrl/de.BGR' | |||
alias 'armn/co' 0x581 'cyrl/de.BGR' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest putting these into letter/armenian
but create a new file (say, letter/armenian/aliases.ptl
).
Similar for other aliases.
(I shall add this part into my refactor plan too...)
hopefully this doesn't break anything... |
I think they are fine. |
Edited changelog, though I suppose the version would probably be bumped. It's probably ready to be merged, if there are no other problems. |
Right, at least bump the minor version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using include : refer-glyph 'q'
when the original q
character is subject to CV features is not good practice. It should instead select its appropriate variants from within latin/lower-q.ptl
. Otherwise it will appear as if it were locked to whatever q
variant the user compiles their font with by default, which also might not be one you expect here.
create-glyph 'armn/da' 0x564 : glyph-proc | ||
local df : include : DivFrame 1 | ||
include : df.markSet.p | ||
include : refer-glyph 'latn/eta' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to latin/lower-n.ptl
to have more consistent variant behavior. BTW ss17
uses a tailed n
under italics which might not be what you want here.
Alternatively it could be remade from scratch to match the other characters in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if the wiki images for Armenian handwriting is any indication, it's probably fine to have the tail in italic (not that I'm trying to replicate that).
Lower ra is already differentiated by having the n shoulder shift inward.
Well... yeah, I'm kinda aware of the compatibility problem with variants (see top comment in #2552). I reckon it will be hard to reliable "link" Armenian letters to some existing character variants, because most characters that share a shape with Latin letters are not actually related to them etymologically(?), and even if we just link by shape, there will be a few characters where the exact character it should link to is ambiguous. I'd say just merge it as is first (unless there are actual problems where the variant shape would affect the derived character -- I fixed it for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are good enough.
Reopening of #2552. Accidentally discarded when trying to merge the new commits to the branch.
images
Things changed since last push:
other.ptl
, though that only contains aliases now. To be cleaned up.g
(aliased to bulgarian de) for lower Co.For the suggested changes:
I only moved the right part of the bar (forgot to do so for upper Keh) for now since the left part should extrude out of the stem. Should I also move left stem rightward?
For the second point, doesn't seem to be possible for ultraextended heavy slab, which the bar will overlap with the bottom serif. I moved it to the center at baseline for now.
Done (but added overshoot for the hook part so that it looks a little bit better).
Also done (also for Za for consistency).
Barely.
With normal stroke the space left in the middle is very small, so the inner curl would be really thin to fit in.
It still works in the ultracondensed case for now: