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

Ready for Review: Fix font-config crash #168

Merged
merged 22 commits into from
Dec 22, 2020

Conversation

brightly-salty
Copy link
Contributor

@brightly-salty brightly-salty commented Dec 19, 2020

Allowing because following clippy's suggestion results in an error in Rust 1.41.0 (was an unstable feature then)
src/window/concept_frame.rs Outdated Show resolved Hide resolved
}
}
let font_bytes: Option<Vec<u8>> = fontconfig::FontConfig::new()
.and_then(|font_config: fontconfig::FontConfig| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the type annotations in these and_then calls necessary? I'd be surprised if they are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they aren't. I just included them to aid my understanding through the implementation. Hopefully this next commit will compile, and then it will be ready (but maybe need to squash b/c so many were failing)

@brightly-salty
Copy link
Contributor Author

I am unable to compile on my own before each commit because my build of calloop keeps failing.

@brightly-salty brightly-salty changed the title Fix font-config crash WIP: Fix font-config crash Dec 19, 2020
@brightly-salty brightly-salty changed the title WIP: Fix font-config crash Ready for Review: Fix font-config crash Dec 19, 2020
@brightly-salty
Copy link
Contributor Author

@chrisduerr It's ready now for your review. It will definitely need to be squashed when it's merged because 17 commits for a couple lines is excessive.

Copy link

@catleeball catleeball left a comment

Choose a reason for hiding this comment

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

Small nit: It looks like you may have accidentally added your .DS_Store file. I might be good to add this to the .gitignore for this repository.

@chrisduerr
Copy link
Contributor

I might be good to add this to the .gitignore for this repository.

Files that are specific to your development process should always be added to your global gitignore file, not project specific gitignores.

@elinorbgr
Copy link
Member

@brightly-salty : if you are unable to compile calloop, you can disable it in SCTK by building it with

cargo build --no-default-features --features "frames"

However, the build failure of calloop is definitely a problem, could you report it to calloop's repository ?

Copy link
Member

@elinorbgr elinorbgr left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!

This looks pretty good overall, There is just one main thing I left in a review comment about self.font_data that I think should be part of the PR as well. Once that is addressed this should be mergeable. 👍

src/window/concept_frame.rs Outdated Show resolved Hide resolved
src/window/concept_frame.rs Outdated Show resolved Hide resolved
@brightly-salty
Copy link
Contributor Author

@brightly-salty : if you are unable to compile calloop, you can disable it in SCTK by building it with

cargo build --no-default-features --features "frames"

However, the build failure of calloop is definitely a problem, could you report it to calloop's repository ?

I have reported it here, but when I run your build script I get similar errors, now coming client-toolkit itself.

@elinorbgr
Copy link
Member

Oh, you're on MacOS. Well that's expected that the build fails then, as no attempt was made to have SCTK build on MacOS given Wayland is not really a thing on this platform.

.gitignore Outdated
@@ -2,3 +2,4 @@
/target
**/*.rs.bk
Cargo.lock
.DS_Store
Copy link
Member

Choose a reason for hiding this comment

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

As @chrisduerr noted, this kind of files should be added to a global gitignore on your computer rather than in each project gitignore.

src/window/mod.rs Outdated Show resolved Hide resolved
@elinorbgr
Copy link
Member

Okay, this is good now, thank you!

@elinorbgr elinorbgr merged commit 4e7466c into Smithay:master Dec 22, 2020
@brightly-salty brightly-salty deleted the improve-error-handling branch December 22, 2020 12:44
@catleeball
Copy link

Thank you for making a PR to address my alacritty issue! 🎉

@chrisduerr
Copy link
Contributor

@vberger Do you want to release a patch for this or are there any other things in the pipeline?

@elinorbgr
Copy link
Member

You're right, there nothing else waiting currently. 0.12.2 is now released.

@chrisduerr
Copy link
Contributor

Awesome, thanks for the quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when loading fontconfig
4 participants