Skip to content

Update mm.c#5386

Merged
skef merged 5 commits intofontforge:masterfrom
slichtzzz:master
Nov 11, 2024
Merged

Update mm.c#5386
skef merged 5 commits intofontforge:masterfrom
slichtzzz:master

Conversation

@slichtzzz
Copy link
Copy Markdown
Contributor

Corrected the situation, when substituted part of the string remains untranslated.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Non-breaking change

Comment thread fontforge/mm.c Outdated
FVChangeGID( sf->fv,i);
ff_post_error(_("Bad Multiple Master Font"),_("The %1$s hints in glyph \"%2$.30s\" in font %3$.30s do not match those in %4$.30s (different number or different overlap criteria)"),
"horizontal", sf->glyphs[i]->name,sf->fontname, mm->instances[j]->fontname);
_("horizontal"), sf->glyphs[i]->name,sf->fontname, mm->instances[j]->fontname);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting localizable strings is a bad practice, as they cannot be reliably translated due to syntax differences. This string should be as follows:
_("The horizontal hints in glyph \"%1$.30s\" in font %2$.30s do not match those in %3$.30s (different number or different overlap criteria)"), sf->glyphs[i]->name,sf->fontname, mm->instances[j]->fontname

The other string below should be changed accordingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes! Especially in this case substitution makes no sense.

Comment thread fontforge/mm.c Outdated
FVChangeGID( sf->fv,i);
ff_post_error(_("Bad Multiple Master Font"),_("The glyph %1$.30s in font %2$.30s has a different set of kern pairs than in %3$.30s"),
"vertical", sf->glyphs[i]->name,sf->fontname, mm->instances[j]->fontname);
ff_post_error(_("Bad Multiple Master Font"),_("The glyph vertical in font %2$.30s has a different set of kern pairs than in %3$.30s"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string looks buggy at the first place - note that it used to have 3 parameters and 4 arguments. I think "vertical" should be just removed, the parameter %1$.30s is about glyph name, and it should be kept.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! Didn't notice that.

@iorsh
Copy link
Copy Markdown
Contributor

iorsh commented Mar 6, 2024

Looks good to me now

Copy link
Copy Markdown
Contributor

@skef skef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@skef
Copy link
Copy Markdown
Contributor

skef commented Apr 17, 2024

The checks aren't starting on this for some reason

@skef
Copy link
Copy Markdown
Contributor

skef commented Apr 20, 2024

@slichtzzz can you push some trivial change to this branch? The checks are stuck and probably won't trigger without a commit. (You could just add two commits that make a change and then reverse it, for example.)

@jtanx jtanx closed this Jul 16, 2024
@jtanx jtanx reopened this Jul 16, 2024
@iorsh
Copy link
Copy Markdown
Contributor

iorsh commented Jul 16, 2024

Comment thread fontforge/mm.c Outdated
jtanx
jtanx previously requested changes Jul 16, 2024
Copy link
Copy Markdown
Contributor

@jtanx jtanx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix format strings

@skef skef self-requested a review October 6, 2024 22:23
Copy link
Copy Markdown
Contributor

@skef skef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving

@skef skef dismissed jtanx’s stale review October 6, 2024 22:25

@jtanx resolved his own concern with an additional push

Corrected the situation, when substituted part of the string remains untranslated.
Corrected mistakes as per the reviewer's request
@skef skef merged commit e9f3d3d into fontforge:master Nov 11, 2024
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.

4 participants