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

SvgSurface: make filename param optional #294

Merged
merged 1 commit into from Dec 5, 2019

Conversation

@bilelmoussaoui
Copy link
Contributor

bilelmoussaoui commented Nov 28, 2019

Per the docs https://www.cairographics.org/manual/cairo-SVG-Surfaces.html
This allows drawing parts of a SVG file without saving it to a tmp file.

I'm not sure if what I did is right or not :P

Fixes #293

@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch from 99e5b8a to d915b7a Nov 28, 2019
Copy link
Member

GuillaumeGomez left a comment

👍

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Nov 28, 2019

@GuillaumeGomez Any idea why the CI fails with

error[E0599]: no method named `to_glib_none` found for type `std::option::Option<&std::string::String>` in the current scope

  --> src/svg.rs:44:61

   |

44 |                 ffi::cairo_svg_surface_create(path.as_ref().to_glib_none().0, width, height),

   |                                                             ^^^^^^^^^^^^ method not found in `std::option::Option<&std::string::String>`

It builds and works fine locally here.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 28, 2019

Try to change your code as follow:

path.map(|s| s.as_str()).to_glib_none().0
@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch 2 times, most recently from fd366b5 to aabfe42 Nov 28, 2019
@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Nov 28, 2019

My bad! I was running the tests without --features svg. It should pass this time. Thanks!

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Nov 28, 2019

It fails because tests are run without --features use_glib 🤦‍♂

@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch from aabfe42 to ce8b704 Nov 28, 2019
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 28, 2019

Oh then it depends: is it supposed to be run even without glib or not? If so, please don't use to_glib calls.

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Nov 28, 2019

It doesn't need to. In such case, what I'm supposed to do?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 28, 2019

Like this:

let c_str = CString::new(c_str).unwrap();
// then:
c_str.as_ptr();
src/svg.rs Outdated Show resolved Hide resolved
@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch from ce8b704 to 8649963 Dec 4, 2019
src/svg.rs Show resolved Hide resolved
@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch from 8649963 to 8892c7c Dec 4, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 4, 2019

lgtm

src/svg.rs Outdated
#[cfg(windows)]
let path = {
// Based on GLib path to C
let path_str = path

This comment has been minimized.

Copy link
@sdroege

sdroege Dec 4, 2019

Member

This assumes that path is not an Option and already a Path. Needs the map(), as_ref() part from above too

@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch from 8892c7c to 9b0b9d3 Dec 4, 2019
src/svg.rs Show resolved Hide resolved
src/svg.rs Outdated
CString::new(p.as_ref().as_os_str().as_bytes())
.expect("Invalid path with NULL bytes")
});
path.map(|p| p.as_ptr()).unwrap_or(ptr::null())

This comment has been minimized.

Copy link
@sdroege

sdroege Dec 4, 2019

Member

The path will be freed once this block is done, and then you store a dangling pointer in path in the outer scope. Just return the Option<CString> from this block, and do the as_ptr() part for both Windows/UNIX just before calling the C function.

@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch from 9b0b9d3 to 3539d5e Dec 4, 2019
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 4, 2019

Looks good for real now :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 4, 2019

Then I'll merge for real once CI passed. 😆

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

@bilelmoussaoui Thanks, 👍

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 4, 2019

@EPashkin Oh indeed! Do you mind opening an issue for it please? So it can be done for the next release. :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 4, 2019

That would not help here as otherwise cairo would have to always depend on glib

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

@sdroege Agree about this case, but IMHO here also better add separate path_to_c and use with map to make code more clear

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Dec 4, 2019

@sdroege Agree about this case, but IMHO here also better add separate path_to_c and use with map to make code more clear

I don't see a valid reason to do that as the function won't be used anywhere else but I agree it makes that part of code less readable.

The failing tests are not related to this change I think.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

@bilelmoussaoui Why you think that it not related?

failures:

---- svg::test::file stdout ----

thread 'svg::test::file' panicked at 'called `Result::unwrap()` on an `Err` value: "error while writing to output stream"', src/libcore/result.rs:1189:5

failures:

    svg::test::file

test result: FAILED. 39 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

IMHO test with None also will be good

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Dec 4, 2019

Because I had that issue once, tried to reproduce it and it was gone the next time I ran the tests. Yeah a test with None will be good. Will add that.

src/svg.rs Outdated Show resolved Hide resolved
@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch 3 times, most recently from c9bd3d3 to 16536d9 Dec 4, 2019
src/svg.rs Show resolved Hide resolved
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 4, 2019

Still fails to write the output file, on Windows and macOS but not on Linux.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

On Windows SvgSurface::new not work even for direct path

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

Old version works on let surface = SvgSurface::new(100., 100., "_.tmp").unwrap();,
I try to find problem

src/svg.rs Outdated Show resolved Hide resolved
Per the docs https://www.cairographics.org/manual/cairo-SVG-Surfaces.html
This allows drawing parts of a SVG file without saving it to a tmp file.
@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:patch-1 branch from 16536d9 to f447b72 Dec 4, 2019
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 4, 2019

CI passed

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Dec 4, 2019

Great, let's get it in then :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Dec 5, 2019

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit fe65952 into gtk-rs:master Dec 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bilelmoussaoui bilelmoussaoui deleted the bilelmoussaoui:patch-1 branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.