Add getters for Event properties. #107

Merged
merged 1 commit into from Mar 6, 2016

Conversation

Projects
None yet
3 participants
@hmeyer

hmeyer commented Mar 2, 2016

No description provided.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Mar 2, 2016

Member

Thanks for the PR! I'm adding an issue #108 with a higher level overview of the goals and some more specific comments here:

Since window is a member of EventAny it should be possible to add just one accessor to Event, all the others will get the method via autoderef.

EventKey documentation seems to strongly discourage using string.

Member

gkoz commented Mar 2, 2016

Thanks for the PR! I'm adding an issue #108 with a higher level overview of the goals and some more specific comments here:

Since window is a member of EventAny it should be possible to add just one accessor to Event, all the others will get the method via autoderef.

EventKey documentation seems to strongly discourage using string.

@hmeyer

This comment has been minimized.

Show comment
Hide comment
@hmeyer

hmeyer Mar 5, 2016

I addressed most comments.
In EventSetting I still use std::ffi::CStr - shouldn't that be safe?

hmeyer commented Mar 5, 2016

I addressed most comments.
In EventSetting I still use std::ffi::CStr - shouldn't that be safe?

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Mar 5, 2016

Member

CStr is safe but puts a burden of converting to &str on the user. So far we have been choosing to handle that in the library.

Take GdkEventSetting::name: it's probably reasonable to expect it to be ASCII ...actually I can't make any argument about this event. I don't see any references to in the GIRs. We might do well to just delete it.

Member

gkoz commented Mar 5, 2016

CStr is safe but puts a burden of converting to &str on the user. So far we have been choosing to handle that in the library.

Take GdkEventSetting::name: it's probably reasonable to expect it to be ASCII ...actually I can't make any argument about this event. I don't see any references to in the GIRs. We might do well to just delete it.

src/event_key.rs
+ length as u32
+ }
+
+ pub fn get_hardware_keycode(&self) -> ::enums::key::Key {

This comment has been minimized.

@gkoz

gkoz Mar 5, 2016

Member

I think this and get_keyval need to be the other way around: enums::key is about keyvals and Key is u32, while hardware_keycode is u16.

@gkoz

gkoz Mar 5, 2016

Member

I think this and get_keyval need to be the other way around: enums::key is about keyvals and Key is u32, while hardware_keycode is u16.

src/event_configure.rs
@@ -9,3 +9,19 @@ pub struct EventConfigure(::Event);
event_wrapper!(EventConfigure, GdkEventConfigure);
event_subtype!(EventConfigure, Configure);
+
+impl EventConfigure {
+ pub fn get_position(&self) -> (u32, u32) {

This comment has been minimized.

@gkoz

gkoz Mar 5, 2016

Member

Coordinates probably can legitimately have negative values.

@gkoz

gkoz Mar 5, 2016

Member

Coordinates probably can legitimately have negative values.

src/event_owner_change.rs
+ self.as_ref().reason
+ }
+
+ pub fn get_selection(&self) -> Option<::Atom> {

This comment has been minimized.

@gkoz

gkoz Mar 5, 2016

Member

Atom is tricky. It has a NONE value so it doesn't need Option.

@gkoz

gkoz Mar 5, 2016

Member

Atom is tricky. It has a NONE value so it doesn't need Option.

src/event_grab_broken.rs
+
+impl EventGrabBroken {
+ pub fn is_keyboard(&self) -> bool {
+ self.as_ref().keyboard != 0

This comment has been minimized.

@gkoz

gkoz Mar 5, 2016

Member

Oh, and I'd prefer bool conversions to be consistent with the rest of the code:

from_glib(self.as_ref().keyboard)
@gkoz

gkoz Mar 5, 2016

Member

Oh, and I'd prefer bool conversions to be consistent with the rest of the code:

from_glib(self.as_ref().keyboard)
@hmeyer

This comment has been minimized.

Show comment
Hide comment
@hmeyer

hmeyer Mar 5, 2016

next round...

hmeyer commented Mar 5, 2016

next round...

src/event_window_state.rs
+ self.as_ref().changed_mask
+ }
+
+ pub fn get_new_state(&self) -> ::WindowState {

This comment has been minimized.

@gkoz

gkoz Mar 5, 2016

Member

Naming inconsistency here doesn't seem justified.

@gkoz

gkoz Mar 5, 2016

Member

Naming inconsistency here doesn't seem justified.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Mar 5, 2016

Member

There are a couple of redundant extern crate lines. We already have those crates imported in lib.rs, so it's enough to use them or specific types (e.g. use cairo::RectangleInt;).

Other than these two comments seems good to go.

Member

gkoz commented Mar 5, 2016

There are a couple of redundant extern crate lines. We already have those crates imported in lib.rs, so it's enough to use them or specific types (e.g. use cairo::RectangleInt;).

Other than these two comments seems good to go.

@hmeyer

This comment has been minimized.

Show comment
Hide comment

hmeyer commented Mar 5, 2016

done.

@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Mar 6, 2016

Member

Thanks for contributing @hmeyer!

Member

gkoz commented Mar 6, 2016

Thanks for contributing @hmeyer!

gkoz added a commit that referenced this pull request Mar 6, 2016

Merge pull request #107 from hmeyer/master
Add getters for Event properties.

@gkoz gkoz merged commit 3736379 into gtk-rs:master Mar 6, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment