Skip to content

Conversation

@behackl
Copy link
Member

@behackl behackl commented Oct 24, 2020

List of Changes

This changes our default text renderer from the Cairo toy API to Pango. A logger.info message is added that points to the former implementation in the form of CairoText.

Testing Status

Tests pass (at least locally).

Acknowledgement

@behackl
Copy link
Member Author

behackl commented Oct 24, 2020

Note: I put up this PR as a suggestion. I think it makes sense to switch to rendering with Pango by default. If there are people against doing so already, feel free to speak up; I don't mind closing this PR again (or leaving it open until we feel that it is a better time to switch).

@naveen521kk
Copy link
Member

Thank you for picking this up✌.

@behackl behackl added the enhancement Additions and improvements in general label Oct 24, 2020
@behackl
Copy link
Member Author

behackl commented Oct 24, 2020

If someone wonders about the RTD build time: the first time I pushed it we have 506 seconds (which is about right), and the second time (after removing an additional newline) 765 seconds.

I think the latter build is just an outlier, no need to worry about it.

Copy link
Member

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

LGTM!

@eulertour
Copy link
Member

I'm a little uncomfortable doing this because I still don't understand the new problem with Cairo installation on Windows.

Am I understanding correctly that pycairo and pangocairocffi both contain Cairo wheels, and that the two are incompatible with each other? And if that is the case, should we be working towards replacing pycairo entirely?

@behackl
Copy link
Member Author

behackl commented Oct 25, 2020

I'm a little uncomfortable doing this because I still don't understand the new problem with Cairo installation on Windows.

Are you referring to the problems where the import of pangocairocffi._generated.ffi fails? I might have missed other reports of installation failures.

Anyways, if so I think this is a problem with how pangocffi and pangocairocffi are installed and not necessarily an interaction problem between different cairo wheels -- but @naveen521kk has investigated this closer, I think?

Am I understanding correctly that pycairo and pangocairocffi both contain Cairo wheels, and that the two are incompatible with each other? And if that is the case, should we be working towards replacing pycairo entirely?

The description of cairocffi mentions that it is a drop-in replacement for pycairo; I don't know whether it makes sense to actually replace it.

@naveen521kk
Copy link
Member

naveen521kk commented Oct 25, 2020

pycairo is different from all other things and not related to this.

Now, here I was forced to use cffi binding for pango as I couldn't find a CPython binding for it. So, I had to make it compatible as it is not possible to have connections between pycairo and cffi binding of Pango aka, pangocairocffi and pangocffi. So I had to cairocffi here. All these cffi binding requires interaction with binaries.

But the problem is users doesn't download the correct binaries, choco helps here. I had been try to help those projects to have self included wheels just like pycairo.

@leotrs leotrs mentioned this pull request Oct 25, 2020
1 task
@eulertour
Copy link
Member

In that case can we open an issue to get chocolatey working again?

@naveen521kk
Copy link
Member

In that case can we open an issue to get chocolatey working again?

I did my best there. Just waiting for their mods to come up soon. Maybe a week?

@naveen521kk
Copy link
Member

@behackl can you fix this function?

def text2hash(self):
"""Internally used function.
Generates ``sha256`` hash for file name.
"""
settings = (
"PANGO" + self.font + self.slant + self.weight
) # to differentiate Text and PangoText
settings += str(self.t2f) + str(self.t2s) + str(self.t2w)
settings += str(self.line_spacing) + str(self.size)
id_str = self.text + settings
hasher = hashlib.sha256()
hasher.update(id_str.encode())
return hasher.hexdigest()[:16]

Remove that "PANGO" from the hash.

@behackl
Copy link
Member Author

behackl commented Oct 26, 2020

Remove that "PANGO" from the hash.

Wouldn't this cause CairoText and PangoText to produce the same hashes? I think it should be kept around as long as we keep CairoText around as a fallback.

Copy link
Member

@naveen521kk naveen521kk left a comment

Choose a reason for hiding this comment

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

ok then it can be merged.

@naveen521kk naveen521kk merged commit f437525 into ManimCommunity:master Oct 26, 2020
@behackl behackl deleted the pango-default branch May 16, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Additions and improvements in general

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants