fix(render): only apply real effects #1643
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The API change in cursive-core 0.4.0 included the addition of EffectStatus, which was intended to allow composite or additive effects, as well as exclusive effects. The recent update to misapplied this new API and ignored the status, effectively applying all styles, all the time. This change instead resolves the incoming styles against an "empty" style, yielding only the styles that should be applied. As far as I am aware, we don't do any compositing with cursive styles, so this should yield same results as we had with cursive 0.3. Moreover, because we're once again only iterating on the actual effects we need to apply, we can also restore the `bail!()` that was unnecessarily removed durint the upgrade. Closes arxanas#1642 Ref: ed946a9 Ref: f6a7008 Ref: gyscos/cursive@ea2e0be
claytonrcarter
force-pushed
the
fix-1642
branch
from
90c0c30 to
185c2c1
Compare
claytonrcarter
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I didn't review the AI generated changes in ed946a9 and f6a7008 carefully enough. In particular, the AI slapped a bandaid over the Effect/EffectStatus API change in cursive 0.4 that compiled and passed tests, but which was nevertheless incorrect because the rendered ANSI codes were all wrong. As I understand it, the API change passes all effects but each effect has a corresponding status to indicate if it should be applied or not. So the previous version of this code was just applying all effects, all the time. This change reproduces our previous behavior by resolving the effects+statuses against a base "empty" effect, which is what we want: we don't do any style compositing (that I know of) and only apply styles to unstyled text.
Top is current master, bottom is with this change.
