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

Proper glib bindings for font types #122

Merged
merged 7 commits into from Apr 22, 2017

Conversation

Projects
None yet
3 participants
@johncf
Contributor

johncf commented Apr 22, 2017

No description provided.

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

FontOptions commit would need some careful reviewing. I changed the ffi signatures based on documentation found FontOptions, cairo_set_font_options and cairo_scaled_font_create.

PS: I believe I caught at least one soundness bug.

Contributor

johncf commented Apr 22, 2017

FontOptions commit would need some careful reviewing. I changed the ffi signatures based on documentation found FontOptions, cairo_set_font_options and cairo_scaled_font_create.

PS: I believe I caught at least one soundness bug.

Show outdated Hide outdated src/font/font_options.rs Outdated
Show outdated Hide outdated src/font/font_options.rs Outdated
Show outdated Hide outdated src/font/font_options.rs Outdated
Show outdated Hide outdated src/font/font_options.rs Outdated
Show outdated Hide outdated src/font/font_options.rs Outdated
unsafe {
ffi::cairo_get_font_options(self.0, out.get_ptr());
ffi::cairo_get_font_options(self.0, out.to_glib_none_mut().0);

This comment has been minimized.

@EPashkin

EPashkin Apr 22, 2017

Member

IMHO this also don't changes internal pointer

@EPashkin

EPashkin Apr 22, 2017

Member

IMHO this also don't changes internal pointer

This comment has been minimized.

@EPashkin

EPashkin Apr 22, 2017

Member

But as @GuillaumeGomez prefer left &mut for setter this don't need change

@EPashkin

EPashkin Apr 22, 2017

Member

But as @GuillaumeGomez prefer left &mut for setter this don't need change

This comment has been minimized.

@johncf

johncf Apr 22, 2017

Contributor

It doesn't change the internal pointer, but it does modify the underlying data structure, right? So giving it a *mut might be the "better written" (or "better specified") thing to do?

PS: I'm not entirely sure when *const vs *mut should be used.

@johncf

johncf Apr 22, 2017

Contributor

It doesn't change the internal pointer, but it does modify the underlying data structure, right? So giving it a *mut might be the "better written" (or "better specified") thing to do?

PS: I'm not entirely sure when *const vs *mut should be used.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

Yes, my point in here is trying to force users to have a mutable access if the underlying data is modified (unlike get methods/functions).

@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

Yes, my point in here is trying to force users to have a mutable access if the underlying data is modified (unlike get methods/functions).

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 22, 2017

Member

@johncf Big thanks for this.
IMHO only last is to confirm that this crate still working (FontOption clone, etc. ). Can you do it?

Member

EPashkin commented Apr 22, 2017

@johncf Big thanks for this.
IMHO only last is to confirm that this crate still working (FontOption clone, etc. ). Can you do it?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 22, 2017

Member

Forgot say about removed get_ptr/get_ptr_mut.
I proposed leave it but add deprecated attribute to it.
But as this crate still not 1.0.0 this not big problem for me.
@GuillaumeGomez It need be restored?

Member

EPashkin commented Apr 22, 2017

Forgot say about removed get_ptr/get_ptr_mut.
I proposed leave it but add deprecated attribute to it.
But as this crate still not 1.0.0 this not big problem for me.
@GuillaumeGomez It need be restored?

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

only last is to confirm that this crate still working (FontOption clone, etc. ).

By writing automatic test cases to check whether they work? I'm not very sure how to check their validity, though. Are there any test cases already in the repo that I can use as a template?

Contributor

johncf commented Apr 22, 2017

only last is to confirm that this crate still working (FontOption clone, etc. ).

By writing automatic test cases to check whether they work? I'm not very sure how to check their validity, though. Are there any test cases already in the repo that I can use as a template?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

I proposed leave it but add deprecated attribute to it.

This is actually an excellent idea!

But as this crate still not 1.0.0 this not big problem for me.

Well, it is for me. ;) If we have "stable" code enough, it'll only be easier for us to finally release a 1.0 version.

@GuillaumeGomez It need be restored?

What do you mean?

By writing automatic test cases to check whether they work? I'm not very sure how to check their validity, though. Are there any test cases already in the repo that I can use as a template?

I think the "easiest" way to confirm this is adding an example to the examples repo. Or maybe oyu had something else in mind @EPashkin?

Member

GuillaumeGomez commented Apr 22, 2017

I proposed leave it but add deprecated attribute to it.

This is actually an excellent idea!

But as this crate still not 1.0.0 this not big problem for me.

Well, it is for me. ;) If we have "stable" code enough, it'll only be easier for us to finally release a 1.0 version.

@GuillaumeGomez It need be restored?

What do you mean?

By writing automatic test cases to check whether they work? I'm not very sure how to check their validity, though. Are there any test cases already in the repo that I can use as a template?

I think the "easiest" way to confirm this is adding an example to the examples repo. Or maybe oyu had something else in mind @EPashkin?

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

I proposed leave it but add deprecated attribute to it.

Well this is a breaking change anyway. So, if you're following semantic versioning, next release after this is merged should be 0.2.0 (you're welcome 😄 ). So I don't think deprecating an already undocumented function would make sense.

Contributor

johncf commented Apr 22, 2017

I proposed leave it but add deprecated attribute to it.

Well this is a breaking change anyway. So, if you're following semantic versioning, next release after this is merged should be 0.2.0 (you're welcome 😄 ). So I don't think deprecating an already undocumented function would make sense.

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

I think the "easiest" way to confirm this is adding an example to the examples repo.

This would take me some time. I wouldn't be confident "using" this API yet. The only examples I have created using gtk-rs were by referring to examples in C code. If I could find any (or if you could point me to one), I'll add them.

Contributor

johncf commented Apr 22, 2017

I think the "easiest" way to confirm this is adding an example to the examples repo.

This would take me some time. I wouldn't be confident "using" this API yet. The only examples I have created using gtk-rs were by referring to examples in C code. If I could find any (or if you could point me to one), I'll add them.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

Never used it myself. Looks like it'll be fun haha.

Member

GuillaumeGomez commented Apr 22, 2017

Never used it myself. Looks like it'll be fun haha.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 22, 2017

Member

As I don't use cairo at all, so for me is good even if @johncf say that it works 😉
Updated example\cairotest.rs is better

Member

EPashkin commented Apr 22, 2017

As I don't use cairo at all, so for me is good even if @johncf say that it works 😉
Updated example\cairotest.rs is better

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

@EPashkin: It's good for me as is, do I merge?

Member

GuillaumeGomez commented Apr 22, 2017

@EPashkin: It's good for me as is, do I merge?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 22, 2017

Member

@GuillaumeGomez Yes if you agree with completely removed get_ptr/get_ptr_mut

Member

EPashkin commented Apr 22, 2017

@GuillaumeGomez Yes if you agree with completely removed get_ptr/get_ptr_mut

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

As I don't use cairo at all, so for me is good even if @johncf say that it works 😉

It works!! 🤞

Well, one simple example I could think of is to get_font_options from a cairo context, change antialias or something, and set_font_options. May be do a clone too while at it!! ScaledFont API looks scary! I'll see what I can do after a few days.

Contributor

johncf commented Apr 22, 2017

As I don't use cairo at all, so for me is good even if @johncf say that it works 😉

It works!! 🤞

Well, one simple example I could think of is to get_font_options from a cairo context, change antialias or something, and set_font_options. May be do a clone too while at it!! ScaledFont API looks scary! I'll see what I can do after a few days.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

@EPashkin: I'm just a bit worried about performance issues. If you don't think it'll be too much, then let's merge.

Member

GuillaumeGomez commented Apr 22, 2017

@EPashkin: I'm just a bit worried about performance issues. If you don't think it'll be too much, then let's merge.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 22, 2017

Member

👍 for merge.

Member

EPashkin commented Apr 22, 2017

👍 for merge.

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

@GuillaumeGomez Could you specify the change you suspect to have introduced a performance regression.

Side question: how do you quantify performance of GUIs, in general?

Contributor

johncf commented Apr 22, 2017

@GuillaumeGomez Could you specify the change you suspect to have introduced a performance regression.

Side question: how do you quantify performance of GUIs, in general?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

@johncf: It's quite small, but repeated, it could be big. get_ptr* was shorter than to_glib_* functions. And GUI performance is "simple" to check: as long as it's fluid, it's ok. :)

Thanks to both of you!

Member

GuillaumeGomez commented Apr 22, 2017

@johncf: It's quite small, but repeated, it could be big. get_ptr* was shorter than to_glib_* functions. And GUI performance is "simple" to check: as long as it's fluid, it's ok. :)

Thanks to both of you!

@GuillaumeGomez GuillaumeGomez merged commit 55b073c into gtk-rs:master Apr 22, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

get_ptr* was shorter than to_glib_* functions.

Those will most likely get optimized out by LLVM gods!

And GUI performance is "simple" to check: as long as it's fluid, it's ok. :)

No, seriously, in an automated test-like manner. Is there some way to attach a number to the current performance? For instance, it would be FPS for games.

Contributor

johncf commented Apr 22, 2017

get_ptr* was shorter than to_glib_* functions.

Those will most likely get optimized out by LLVM gods!

And GUI performance is "simple" to check: as long as it's fluid, it's ok. :)

No, seriously, in an automated test-like manner. Is there some way to attach a number to the current performance? For instance, it would be FPS for games.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 22, 2017

Member

For GTK it's very complicated. I had a few discussions about this recently and it seems there isn't any tool "usable" enough for that unfortunately. :-/

Member

GuillaumeGomez commented Apr 22, 2017

For GTK it's very complicated. I had a few discussions about this recently and it seems there isn't any tool "usable" enough for that unfortunately. :-/

@johncf

This comment has been minimized.

Show comment
Hide comment
@johncf

johncf Apr 22, 2017

Contributor

The closest thing I could find was LDTP from this mailing list. Just for testing, not benchmarking.

Contributor

johncf commented Apr 22, 2017

The closest thing I could find was LDTP from this mailing list. Just for testing, not benchmarking.

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