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

Make KeyFile get_boolean and has_key return bool #320

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
4 participants
@bvinc
Copy link
Contributor

bvinc commented Apr 19, 2018

No description provided.

@bvinc

This comment has been minimized.

Copy link
Contributor Author

bvinc commented Apr 19, 2018

Should close #279

unsafe {
let mut error = ptr::null_mut();
let ret = ffi::g_key_file_get_boolean(self.to_glib_none().0, group_name.to_glib_none().0, key.to_glib_none().0, &mut error);
if error.is_null() { Ok(from_glib(ret)) } else { Err(from_glib_full(error)) }

This comment has been minimized.

@EPashkin

EPashkin Apr 19, 2018

Member

As I understand from gtk-rs/gir#580 this function better use g_key_file_get_value internally?

This comment has been minimized.

@EPashkin

EPashkin Apr 19, 2018

Member

Or this if need 3th case if g_key_file_get_boolean return false and error = null for false value

This comment has been minimized.

@bvinc

bvinc Apr 19, 2018

Author Contributor

I'm not sure what you mean. The problem that I'm fixing is that GIR assumed that the gboolean return value from g_key_file_get_boolean was redundant and useless, and so it generated a function that dropped the gboolean return value and returned Result<(), Error> instead. This is an OK assumption because it's often the right thing to do for functions that throw and also return gboolean.

But in this particular case, the gboolean return value is exactly what we want to return back to the caller.

The &mut error parameter is still important and still needs to be checked.

It looks good to me.

This comment has been minimized.

@EPashkin

EPashkin Apr 19, 2018

Member

Oops, I missed that if condition changed.
No problem for me now.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 19, 2018

Also now you think that g_spawn_check_exit_status don't need fix?

@bvinc

This comment has been minimized.

Copy link
Contributor Author

bvinc commented Apr 19, 2018

PR #321
I decided to do it in a different PR since it's a weird one off that might need more review.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 19, 2018

@GuillaumeGomez only problem is canonicalize() but IMHO it better fixed in #315.
By adding to assert it only for macs.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Apr 19, 2018

Looks good to me. What do you mean with canonicalize(), @EPashkin?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Apr 19, 2018

Looks good to me as well so let's merge.

@sdroege : He wanted to say windows I assume.

@GuillaumeGomez GuillaumeGomez merged commit 1fda951 into gtk-rs:master Apr 19, 2018

1 of 2 checks passed

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

This comment has been minimized.

Copy link
Member

EPashkin commented Apr 19, 2018

@GuillaumeGomez I want revert effect of #318
and add canonicalize only for macos

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Apr 19, 2018

It doesn't work anyway so I think we'll just ignore macos...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.