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

Fix an error that prevents any text layers from being imported #105

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

rodionovd
Copy link
Contributor

Steps to Reproduce

  1. Take a .fig document with at least one text layer and try to convert it to .sketch

Expected Results

The generated .sketch file contains all text layers from the original Figma document.

Actual Results

  1. The tool reports the following error:
ERROR:root:An unexpected error occurred when converting TEXT: another ((3, 58)). It will be skipped
Traceback (most recent call last):
  File "converter/tree.py", line 74, in convert_node
  File "converter/tree.py", line 62, in convert_node
  File "converter/text.py", line 81, in convert
  File "converter/text.py", line 201, in override_characters_style
KeyError: 'styleID'
  1. There're no text layers in the generated .sketch document.

Solution

The culprit is emoji detection logic when text layer attributes are prepared: instead of a special fill style override for emojis there's now a glyph-level "emojiCodePoints" field that we should use to detect them.

Testing

I've updated tests/data/structure.fig to the latest Figma file format version so running

pytest tests/integration/test_structure.py::test_page

will now trigger the error.

Note

There're other test cases in test_structure.py that fail for a different reason – see #104


Now, I'm not 100% happy with this PR:

  1. Do we need to be backwards-compatible with the previous versions of .fig file format? I've left the old code branch intact but the more I think about it the less I'm inclined to keep it around. So what's fig2sketch's official stance on maintaining backward compatibility?

  2. What does the process of updating .fig files used in tests look like? I've imported tests/data/structure.fig to Figma and immediately downloaded it back – is this enough? I guess the same trick will work for tests/data/vector.fig but tests/data/broken_images.fig seemingly requires an additional step to corrupt one of the bundled images – and I couldn't find a script that does it anywhere in the repo.

@tmdvs
Copy link
Member

tmdvs commented Feb 29, 2024

@rodionovd You should be able to rebase main into your branches and 🤞 the GitHub actions will pass 🚥

@rodionovd
Copy link
Contributor Author

@tmdvs all green now!

@tmdvs tmdvs self-requested a review February 29, 2024 16:15
@tmdvs
Copy link
Member

tmdvs commented Feb 29, 2024

Do we need to be backwards-compatible with the previous versions of .fig file format? I've left the old code branch intact but the more I think about it the less I'm inclined to keep it around. So what's fig2sketch's official stance on maintaining backward compatibility?

I've checked in with some of the team on this one. Given we keep older fig2sketch releases available here on GitHub and the ease with which you can migrate older fig documents to newer schemas using Figma, there is no need to keep the code backwards-compatible.

This is a good example where we wouldn't need to worry too much. So feel free to drop that older code path, and we can get this merged in :shipit:

Instead of a special fill style override for emojis there's now a glyph-level "emojiCodePoints" field we can use to detect them

Signed-off-by: Dmitry Rodionov <[email protected]>
@rodionovd
Copy link
Contributor Author

This is a good example where we wouldn't need to worry too much. So feel free to drop that older code path, and we can get this merged in :shipit:

The old code path is no more 👋

@tmdvs tmdvs merged commit e747668 into sketch-hq:main Mar 1, 2024
4 checks passed
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