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

Compacting nested colors sometimes uses the wrong color #455

Closed
rymiel opened this issue Sep 30, 2021 · 2 comments · Fixed by #463
Closed

Compacting nested colors sometimes uses the wrong color #455

rymiel opened this issue Sep 30, 2021 · 2 comments · Fixed by #463
Milestone

Comments

@rymiel
Copy link
Member

rymiel commented Sep 30, 2021

I don't know the extent of the bug but at the very least: Compacting a component with nested children, where one child-of-a-child uses the color of the outer-most element will incorrectly use the color of the child instead.

Consider the following test case:

  @Test
  void testSomethingIdk() {
    final Component notCompact = text("mew", NamedTextColor.RED)
      .append(text("mow", NamedTextColor.BLUE)
          .append(text("mew", NamedTextColor.RED)));
    final Component expectedCompact = text("mew", NamedTextColor.RED)
      .append(text("mow", NamedTextColor.BLUE))
      .append(text("mew"));

    assertEquals(notCompact.compact(), expectedCompact);
  }

Currently, this test fails because the last "mew" turns blue.
image
The second component is joined into "mowmew" in blue, but those should never be joined as they are different colors

@rymiel
Copy link
Member Author

rymiel commented Oct 1, 2021

I've noticed that swapping the order of childParentStyle and parentStyle here will produce a suboptimal tree, but at least keeps the operation as monotonic. I can't wrap my head around all of the parent and child logic but this is where the mistake is coming from; the style of red color is merged into blue with IF_ABSENT_ON_TARGET, so the blue color "wins"

childParentStyle = parentStyle.merge(childParentStyle, Style.Merge.Strategy.IF_ABSENT_ON_TARGET);

@zml2008 zml2008 added this to the 4.9.2 milestone Oct 3, 2021
@zml2008
Copy link
Member

zml2008 commented Oct 3, 2021

Your change there does appear to be correct. It doesn't look like the compaction algorithm has a way to promote a child to a sibling -- adding that would probably be an interesting feature, but I'd rather prioritize correctness for now.

It seems like all other tests pass with that change though, so I think it's worth applying it for now.

zml2008 added a commit that referenced this issue Oct 4, 2021
zml2008 pushed a commit that referenced this issue Oct 4, 2021
Stealing zml's thunder but it "appears to work for now"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants