Skip to content

Conversation

@naveen521kk
Copy link
Member

List of Changes

  • Allow users to disable ligatures and also fix Code with it.
  • Revert make Paragraph use CairoText #798
  • number of submobjects = number of chars if ligaratures are disabled.

Explanation for Changes

Better than using CairoText.

Testing Status

Test

Acknowledgement

@naveen521kk naveen521kk requested review from behackl and leotrs December 1, 2020 15:10
@naveen521kk naveen521kk linked an issue Dec 1, 2020 that may be closed by this pull request
@naveen521kk naveen521kk added this to the Release v0.1.1 milestone Dec 1, 2020
@naveen521kk naveen521kk changed the title Allow disabling ligaratures in Text Allow disabling ligatures in Text Dec 1, 2020
Copy link
Member

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

typo

@naveen521kk naveen521kk added the pr:bugfix Bug fix for use in PRs solving a specific issue:bug label Dec 1, 2020
@leotrs
Copy link
Contributor

leotrs commented Dec 1, 2020

After a provisional skimming, this looks good in general. I have to take a closer look. Could you please add a couple of tests?

@behackl
Copy link
Member

behackl commented Dec 1, 2020

This looks good to me, I also seem to be able to use a monospace font without problems:

CodeTest

Testing Code is a bit tedious as long as we are not able to pass code directly, but have to read a corresponding file. I'd be okay with delegating adding tests to a later point in time where the interface of Code has been refactored a bit.

I'm on the fence whether we should use the current implementation using CairoText for the release, or whether we should get this in before. I probably have a slight preference for merging it after the release.

@naveen521kk
Copy link
Member Author

I will only see a simple problem with this. This will not allow passing something like Arabic, or Hindi or some font which heavily depends on ligatures, from within the code and will look worse. This could be a fix for a short time but this definitely needs to be refactored so that Pango takes the responsibility of colouring it (which it does) instead of manipulating the arrays. But this can't be done for now as our SVG parser ignore style element without which it is not possible. One way it can be fixed is to make it not inherit from SVGMobject instead inherit directly from VMobject and provide methods for working directly with Pango which definitely requires a lot of work though. Also, the present bindings don't support Pango.Attributes without which this can't be done. So, that would require pangocffi to support that.

@leotrs
Copy link
Contributor

leotrs commented Dec 2, 2020

This will not allow passing something like Arabic, or Hindi or some font which heavily depends on ligatures, from within the code and will look worse.

This is why I'm fine with using this fix for CodeMobject, but not for Text itself. I agree this should only be a temporary fix.

Point taken on the tests.

Given the absence of tests, I vote we merge this right after the release.

@behackl behackl modified the milestones: Release v0.1.1, Release v0.2.0 Dec 2, 2020
@naveen521kk
Copy link
Member Author

Ready for review.

@leotrs
Copy link
Contributor

leotrs commented Dec 3, 2020

@naveen521kk could you please solve the merge conflict? 🙂

@naveen521kk
Copy link
Member Author

I didn't expect this lol. Will do no worries 😊

@naveen521kk
Copy link
Member Author

@leotrs done. Also, typed(python typing) a few things.

@naveen521kk naveen521kk requested a review from leotrs December 4, 2020 17:58
Copy link
Member

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

nice job

Co-authored-by: Pg Biel <[email protected]>
@naveen521kk naveen521kk merged commit 756a28e into ManimCommunity:master Dec 8, 2020
@naveen521kk naveen521kk deleted the fix-liga branch December 8, 2020 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:bugfix Bug fix for use in PRs solving a specific issue:bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG-General] Text not working properly on master compared to v0.1.0

4 participants