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 resolving key path for matte layers #2544

Merged

Conversation

alex-dorokhov
Copy link
Contributor

This PR adds the check for matte layer name before going deeper during key path resolution.

Without the fix the key path matches children inside the matte layer even though it should not, for example:

'layer_1.group_1.child_1' could match 'layer_1.matte_1.group_1.child_1'

With the fix it is not possible anymore.

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Could you add a snapshot test for this? You can follow other test cases here as an example.

It may be difficult to fully run the tests locally because it requires API keys but you can comment out other test cases and then put a breakpoint here to view the bitmap snapshot.

Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@gpeal
Copy link
Collaborator

gpeal commented Aug 30, 2024

Could you add a snapshot test for this? You can follow other test cases here as an example.

It may be difficult to fully run the tests locally because it requires API keys but you can comment out other test cases and then put a breakpoint here to view the bitmap snapshot.

Alternatively, if you push a test, it'll run on CI and post a comment back. It just won't run until I click approve so the turnaround time will be longer.

@gpeal
Copy link
Collaborator

gpeal commented Sep 29, 2024

@alex-dorokhov would you like to update this so we can get this PR landed?

@alex-dorokhov
Copy link
Contributor Author

alex-dorokhov commented Oct 4, 2024

@alex-dorokhov would you like to update this so we can get this PR landed?

Sorry, that it got so long for me to reply.

There is already a test for dynamic properties of matte layers. I believe it had wrong results before the fix and now it is good, so should be accepted. Please, look at the second report here #2544 (comment)

I've debugged the test to be sure.

Before the fix

This line has been called twice when callback is added (so two matte layers have been matched).

Screenshot 2024-10-04 at 19 13 23

Please, notice that "Shape Layer 3" should not be matched:

Screenshot 2024-10-04 at 19 13 08

After the fix

This line has been called only with "Shape Layer 1":

Screenshot 2024-10-04 at 19 14 09

I've also attached before and after videos for convenience:

before.mov
after.mov

@gpeal gpeal merged commit e320c99 into airbnb:master Oct 4, 2024
7 checks passed
@gpeal
Copy link
Collaborator

gpeal commented Oct 4, 2024

@alex-dorokhov Thanks for clarifying!

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.

2 participants