Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

Add vfuncs to WidgetImpl #972

Merged
merged 3 commits into from Mar 13, 2020
Merged

Conversation

tsahyt
Copy link
Contributor

@tsahyt tsahyt commented Mar 12, 2020

These are some vfuncs that I previously added manually into my subclasses and are still missing.

  • realize
  • unrealize
  • map
  • unmap
  • motion_notify_event

I've verified that realize and unrealize work as intended. Map and unmap are mostly the same, and should work. I'm not entirely sure about motion_notify_event however, in particular about passing the second argument and whether it should really be mutable.

- realize
- unrealize
- map
- unmap
- motion_notify_event
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me apart from the mutable reference :) The event is not mutable, it's created and then not to be changed anymore.

src/subclass/widget.rs Outdated Show resolved Hide resolved
src/subclass/widget.rs Outdated Show resolved Hide resolved
src/subclass/widget.rs Outdated Show resolved Hide resolved
src/subclass/widget.rs Outdated Show resolved Hide resolved
src/subclass/widget.rs Outdated Show resolved Hide resolved
src/subclass/widget.rs Outdated Show resolved Hide resolved
@tsahyt
Copy link
Contributor Author

tsahyt commented Mar 13, 2020

I've applied the suggested changes and renamed a variable for clarity. The CI came up red before because of what looked like a network error to me. I just wanted to mention that too.

@sdroege
Copy link
Member

sdroege commented Mar 13, 2020

The CI came up red before because of what looked like a network error to me

Happens :)

Comment on lines 846 to 849
let f = (*parent_class)
.realize
.expect("No parent class impl for \"realize\"");
f(widget.to_glib_none().0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let f = (*parent_class)
.realize
.expect("No parent class impl for \"realize\"");
f(widget.to_glib_none().0)
if let Some(f) = (*parent_class).realize {
f(widget.to_glib_none().0);
}

And the same for unrealize, map, unmap and motion_notify_event. They're all optional and nothing bad (and nothing at all) happens when they are not set.

For the motion_notify_event, see the other _event impls for what to do about the return value

@sdroege
Copy link
Member

sdroege commented Mar 13, 2020

@GuillaumeGomez good to go once the CI is happy :) Thanks @tsahyt !

@sdroege
Copy link
Member

sdroege commented Mar 13, 2020

@GuillaumeGomez time to merge :)

@GuillaumeGomez GuillaumeGomez merged commit ab6554a into gtk-rs:master Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants