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

Expose gtk_clipboard_set_with_data. #617

Merged
merged 1 commit into from Feb 7, 2018

Conversation

Projects
None yet
4 participants
@tmiasko
Contributor

tmiasko commented Jan 23, 2018

Rust implementation exposes only GtkClipboardGetFunc as customization
point. GtkClipboardClearFunc is used implicitly to clean up the provided
closure.

}
impl<O: IsA<Clipboard>> ClipboardExtManual for O {
fn set_with_data<F: Fn(&Clipboard, &SelectionData, c_uint) + 'static>(&self, targets: &[TargetEntry], f: F) -> bool {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 23, 2018

Member

I think we never directly expose C-like types (I'm referring to c_uint in here).

This comment has been minimized.

@tmiasko

tmiasko Jan 23, 2018

Contributor

OK, I will replace this with u32, which is the same type as in TargetEntry::info field.
Those two are supposed to go together.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 23, 2018

Member

Use the from_glib function to make the conversion.

This comment has been minimized.

@tmiasko

tmiasko Jan 23, 2018

Contributor

@GuillaumeGomez you refer to u32 -> c_uint conversion? As far as I can see
there seems to be no suitable FromGlib implementation. Either way those types
are exactly the same on all platforms supported by Rust so far.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 23, 2018

Member

I thought it did exist. It's been a while I didn't write bindings by hand so I'm maybe completely wrong...

cc @sdroege @EPashkin

Expose gtk_clipboard_set_with_data.
Rust implementation exposes only GtkClipboardGetFunc as customization
point. GtkClipboardClearFunc is used implicitly to clean up the provided
closure.
}
impl<O: IsA<Clipboard>> ClipboardExtManual for O {
fn set_with_data<F: Fn(&Clipboard, &SelectionData, u32) + 'static>(&self, targets: &[TargetEntry], f: F) -> bool {

This comment has been minimized.

@sdroege

sdroege Jan 23, 2018

Member

This seems more like a set. The with data part is an implementation detail of how the bindings implement it. No data is provided by the caller, the bindings use the data for passing a closure

This comment has been minimized.

@tmiasko

tmiasko Jan 23, 2018

Contributor

Essentially it configures a callback that provides clipboard data when
requested to do so at some point in the future.

Regarding the name, maybe set_data_provider, set_provider, or set like you have
suggested. Though, it seems to me that either way you will have to describe its
behaviour in terms of gtk_clipboard_set_with_data, so changing the name doesn't
seem to be of much help. I would rather remain with current one.

This comment has been minimized.

@sdroege

sdroege Jan 24, 2018

Member

I guess set_data is fine, or set_with_closure

Some(trampoline), Some(cleanup), user_data))
};
if !success {
// Cleanup function is not called in case of a failure.

This comment has been minimized.

@sdroege

sdroege Jan 23, 2018

Member

What an ugly API :)

This comment has been minimized.

@sdroege

sdroege Jan 24, 2018

Member

This seems to be wrong.

clear_func | when the clipboard contents are set again, this function will be called, and get_func will not be subsequently called.

It is called whenever the contents are set again. Our function that we set can be called again if I don't misunderstand.

I think the correct way of doing this is by using the set_with_owner function. Then you can have a GObject subclass that has reference counting and could store the closure.

You would however have to create a new GObject for that from Rust, which is rather boring and verbose. See the first chapters here, the others are not required for this simple case.

This comment has been minimized.

@sdroege

sdroege Jan 24, 2018

Member

The docs of GtkClipboardClearFunc are more useful here

A function that will be called when the contents of the clipboard are changed or cleared. Once this has called, the user_data_or_owner argument will not be used again.

So this should be fine after all

let t_ptr: *mut ffi::GtkTargetEntry = t.as_mut_ptr();
let f: Box_<Box_<Fn(&Clipboard, &SelectionData, u32) + 'static>> = Box_::new(Box_::new(f));
let user_data = Box_::into_raw(f) as *mut _;
let success : bool = unsafe { from_glib(ffi::gtk_clipboard_set_with_data(self.to_glib_none().0,

This comment has been minimized.

@sdroege

sdroege Jan 23, 2018

Member

Is the closure only called from here or might it also be called after the function returned? In case of the former the static bound is not needed, and boxing neither.

This comment has been minimized.

@tmiasko

tmiasko Jan 23, 2018

Contributor

It won't be called immediately. Only after the function returns.

@EPashkin

This comment has been minimized.

Member

EPashkin commented Feb 7, 2018

Oh, I forgot about this PR.
LGFM but seems @sdroege want function rename (docs will be lost in this case: IMHO not big problem).

@sdroege

This comment has been minimized.

Member

sdroege commented Feb 7, 2018

I'm fine with keeping it as-is, it's all bad options so let's keep it as in C :)

@EPashkin

This comment has been minimized.

Member

EPashkin commented Feb 7, 2018

@GuillaumeGomez Then this PR ready for merge it

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Feb 7, 2018

Ok! Thanks @tmiasko!

@GuillaumeGomez GuillaumeGomez merged commit 1bd4350 into gtk-rs:master Feb 7, 2018

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