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

pad_action_entry: Fix dangling pointer #826

Merged
merged 1 commit into from Jun 14, 2019

Conversation

Projects
None yet
4 participants
@sfanxiang
Copy link
Contributor

commented Jun 13, 2019

This was referenced Jun 13, 2019

Show resolved Hide resolved src/pad_action_entry.rs Outdated
Show resolved Hide resolved src/pad_controller.rs Outdated
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

cc @EPashkin as well. :)

}

pub fn get_mode(&self) -> i32 {
self.0.mode
self.mode

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Jun 13, 2019

Member

Shouldn't this be wrapped inside an enum or something to prevent users to change this value?

This comment has been minimized.

Copy link
@sdroege

sdroege Jun 13, 2019

Member

How do you mean? The user sets this very value

@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

IMHO we can also just remove this one function and the struct. It's only "convenience" (how?!) around gtk_pad_controller_set_action(). I don't see how it makes anything easier :)

@sfanxiang sfanxiang force-pushed the sfanxiang:pad_action_entry branch from 6f37aa1 to 0826be2 Jun 13, 2019

Show resolved Hide resolved src/pad_action_entry.rs Outdated
@EPashkin

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Also think that gtk_pad_controller_set_action is enough in most cases,
but if set_action_entries done and safe IMHO better leave it.

@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Also think that gtk_pad_controller_set_action is enough in most cases,
but if set_action_entries done and safe IMHO better leave it.

Fine with me either way :)

@sfanxiang sfanxiang force-pushed the sfanxiang:pad_action_entry branch from 0826be2 to ab4e13b Jun 14, 2019

fn to_glib(&self) -> gtk_sys::GtkPadActionEntry {
self.0
pub fn get_action_name(&self) -> Option<&str> {
Some(&self.action_name)

This comment has been minimized.

Copy link
@sdroege

sdroege Jun 14, 2019

Member

Even more so, these two string getters don't really need to return an Option. They can't be None ever

pub(crate) index: i32,
pub(crate) mode: i32,
pub(crate) label: String,
pub(crate) action_name: String,

This comment has been minimized.

Copy link
@sdroege

sdroege Jun 14, 2019

Member

Actually... as we have getters for all these, they can as well stay completely private and you can use the getters in the other file :)

@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Thanks, looks good to me once that is fixed. @GuillaumeGomez please merge then :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Yes sir!

Thanks @sfanxiang !

@GuillaumeGomez GuillaumeGomez merged commit aed7c94 into gtk-rs:master Jun 14, 2019

0 of 2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@GuillaumeGomez I said "once that is fixed" :P

@sfanxiang Can you create a new PR for the two open discussions above?

sfanxiang added a commit to sfanxiang/gtk-rs-gtk that referenced this pull request Jun 18, 2019

Apply unmerged reviews from gtk-rs#826
- Make PadActionEntry properties private
- Don't return Option

@sdroege sdroege referenced this pull request Jun 18, 2019

Merged

Fix compilation #828

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.