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.

Sign up for GitHub

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

Jump to bottom

Upgrade to cosmic-text 0.13.0 #18239

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Upgrade to cosmic-text 0.13.0 #18239

wants to merge 1 commit into from
+8 −7

Conversation

rparrett
Copy link
Contributor

Objective

Upgrade to cosmic-text 0.13

https://github.com/pop-os/cosmic-text/releases

This should include some performance improvements for layout and system font loading.

Solution

Bump version, fix the one changed API.

Testing

Tested some examples locally, will invoke the example runner.

@github-actions github-actions bot mentioned this pull request Mar 10, 2025
Closed
@github-actions GitHub Actions
Copy link
Contributor

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-18239

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Mar 10, 2025
@alice-i-cecile alice-i-cecile added C-Dependencies A change to the crates that Bevy depends on A-Text Rendering and layout for characters S-Needs-Testing Testing must be done before this is safe to merge X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered labels Mar 10, 2025
alice-i-cecile
alice-i-cecile approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I've looked over the Pixel Eagle output and the diff seems totally fine. There are differences, but I can't say that either is better or worse.

@rparrett
Copy link
Contributor Author

I think the minor differences can be attributed to swash 0.2.0, but it's difficult to follow the changes to that library and its dependencies.

@rparrett
Copy link
Contributor Author

I did initiate a full example run just in case, still pending.

@rparrett
Copy link
Contributor Author

rparrett commented Mar 10, 2025 •
edited
Loading

Not seeing anything concerning in there, just the same minor differences noticed by the testbed.

However, it seems that cosmic-text is failing to build for android in the example runner.

@mnmaita
Copy link
Member

mnmaita commented Mar 10, 2025

pop-os/cosmic-text#349 looks like someone had the same issue with the previous version too?

@rparrett
Copy link
Contributor Author

Reverting pop-os/cosmic-text@829a59b seems to fix it. I guess android targets used to take the "no-op" path in other.rs and that's not happening anymore.

I have a branch that works but I'm not 100% confident in. Will open a PR for discussion.

@rparrett
Copy link
Contributor Author

pop-os/cosmic-text#365

@alice-i-cecile alice-i-cecile modified the milestones: 0.16, 0.17 Mar 11, 2025
@alice-i-cecile
Copy link
Member

Bumping from 0.17; we shouldn't wait on a fix IMO. This is not essential to ship immediately.

@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes and removed S-Needs-Testing Testing must be done before this is safe to merge labels Mar 11, 2025
@rparrett
Copy link
Contributor Author

cosmic-text 0.13.2 was released which should fix the Android issue.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Mar 11, 2025
@alice-i-cecile
Copy link
Member

Excellent, thanks @jackpot51 <3

Carter0
Carter0 approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@Carter0 Carter0 left a comment

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@Carter0 Carter0 Carter0 approved these changes

Assignees
No one assigned
Labels
A-Text Rendering and layout for characters C-Dependencies A change to the crates that Bevy depends on D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.
4 participants
@rparrett @mnmaita @alice-i-cecile @Carter0