jeremyletang / rgtk

GTK+ bindings and wrappers for Rust (DEPRECATED SEE https://github.com/rust-gnome )
GNU Lesser General Public License v3.0
121 stars 22 forks source link

Added gdk_keyval_name function #207

Closed ptersilie closed 9 years ago

ptersilie commented 9 years ago

I implemented the keyval_name function, solving issue #203. I tested it and it works. But could someone have a look and see if the fix looks okay.

For instance I am unsure about line 32 in keys.rs. I had to comment that out since my program would crash otherwise. Similar gdk functions that return the same type (like gdk_screen_get_monitor_plug_name) however suggest that it should be there.

The other thing I'm uncertain about is whether the return type of this

pub fn gdk_keyval_name (keyval:c_uint) -> *mut c_char;

should be immutable as this says "The [returned] String should not be modified".

GuillaumeGomez commented 9 years ago

Just like you thought, you don't have to free it. Otherwise it's good for me. I'll merge after you removed the line in question.

ptersilie commented 9 years ago

But it does say "The string should not be modified.". That's why I was unsure.

GuillaumeGomez commented 9 years ago

Yes, so no modification or free.

ptersilie commented 9 years ago

Ah, so it means it "can" be modified, but shouldn't? That's why we can't free it, cause someone might modify it nontheless?

Anyways, I'll remove that line.

GuillaumeGomez commented 9 years ago

Can you squash your second commit please ? It's not really relevant in here.

gkoz commented 9 years ago

@ptersilie note in the GDK documentation it says about the return value transfer none. It means the library owns the string and you are borrowing it; it's the owner's job to free it.

ptersilie commented 9 years ago

Ah, thanks @gkoz. That "transfer none" explanation makes sense. :)

ptersilie commented 9 years ago

@GuillaumeGomez I squashed the commits.

GuillaumeGomez commented 9 years ago

Good ! Now you just have to replace c_str_to_bytes because it's deprecated. It's better to not add deprecated stuff, we already have enough of it...

gkoz commented 9 years ago

@ptersilie my replacement for c_str_to_bytes would probably be String::from_utf8_lossy(CStr::from_ptr(tmp).to_bytes()).into_owned()

ptersilie commented 9 years ago

@gkoz Thanks. I was just about to look that up. Why "into_owned"?

gkoz commented 9 years ago

from_utf8_lossy returns Cow, so it should be the idiomatic thing to do. No difference in this case.

ptersilie commented 9 years ago

Are you still interested in pulling this?

GuillaumeGomez commented 9 years ago

Yes but you have to fix travis issues and update your code before that.

ptersilie commented 9 years ago

Travis failed because there was something wrong with it, which has been fixed as far as I know. So I guess someone just has to restart Travis (which I'm not sure I can do?).

GuillaumeGomez commented 9 years ago

I just did. We just have to wait now...

ptersilie commented 9 years ago

Thanks. :)

GuillaumeGomez commented 9 years ago

It took 32 minutes to run tests. O.o That's insane... Anyway, I merge your PR. Thanks !