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

use &self for SelectionData::set(), fix #747 #748

Merged
merged 1 commit into from Jan 23, 2019

Conversation

Projects
None yet
5 participants
@jsparber
Copy link
Contributor

jsparber commented Dec 16, 2018

No description provided.

@jsparber

This comment has been minimized.

Copy link
Contributor Author

jsparber commented Dec 17, 2018

I have no idea why the CI fails, the log has only warnings

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 17, 2018

This was error that was fixed today, restarted some jobs

error[E0605]: non-primitive cast: `i32` as `socket::RawSocket
38;5;    ffi::g_socket_get_fd(self.to_glib_none().0) as _
name = "set"
[[object.function.parameter]]
name = "selection_data"
const = true

This comment has been minimized.

@sdroege

sdroege Dec 17, 2018

Member

While you're here, you could also ignore the getter and implement it with its correct name :)

This comment has been minimized.

@sdroege

sdroege Jan 23, 2019

Member

I.e. fn get_data() should call gtk_selection_data_get_data_with_length()

@osa1

This comment has been minimized.

Copy link
Contributor

osa1 commented Jan 23, 2019

What's the status of this? I currently can't use connect_drag_data_get() because of this problem.

Just curious, why we want to update a non-mut SelectionData. Why not make the SelectionData argument &mut? E.g.

fn connect_drag_data_get<F: Fn(&Self, &DragContext, &mut SelectionData, u32, u32) + 'static>(
    &self, 
    f: F
) -> SignalHandlerId

Ping @sdroege @EPashkin

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 23, 2019

@osa1 Thanks for reminder.
We waited for #748 (comment),
but seems @jsparber don't have time and we forgot about this issue.
Changing connect_drag_data_get maybe tough IMHO as it inner function type.
Also there was inconsistency with all other setters which already changed to &self.
Agree change boxed value without mutable reference is strange from rust point of view,
but can seen same as using RefCell

I restarted travis CI just in case, if it still pass 👍 from me.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 23, 2019

I'd prefer if someone also fixed the getter while we're at it. It's a trivial change and otherwise it will never happen. @osa1 do you want to submit another PR for that?

Also there was inconsistency with all other setters which already changed to &self.
Agree change boxed value without mutable reference is strange from rust point of view,
but can seen same as using RefCell

There's nothing strange about that really. It works like this for all shared boxed types and GObjects in the bindings, it's simply interior mutability.

@osa1

This comment has been minimized.

Copy link
Contributor

osa1 commented Jan 23, 2019

I'd prefer if someone also fixed the getter while we're at it. It's a trivial change and otherwise it will never happen. @osa1 do you want to submit another PR for that?

I don't understand what's wrong currently so not sure how can I help with that.

@osa1

This comment has been minimized.

Copy link
Contributor

osa1 commented Jan 23, 2019

But in the meantime it'd be really helpful if we could merge this (for some reason I can't use my fork of gtk-rs in my crate, cargo doesn't like it even though it's just gtk-rs master + one commit).

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 23, 2019

I don't understand what's wrong currently so not sure how can I help with that.

    pub fn get_data_with_length(&self) -> Vec<u8>

should become

    pub fn get_data(&self) -> Vec<u8>

for some reason I can't use my fork of gtk-rs in my crate, cargo doesn't like it even though it's just gtk-rs master + one commit

You need to replace it for all of your dependencies that depend themselves on gtk too. The easiest is via the patch feature of cargo.

@osa1

This comment has been minimized.

Copy link
Contributor

osa1 commented Jan 23, 2019

How do I do that? I don't see get_data_with_length() defined in the Gir.toml file, and the Gtk-3.0.gir says that I the file is generated from C and I should be editing that instead, even though there isn't a line of C in this repo.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 23, 2019

You would add it to Gir.toml and mark is as ignore = true. Then you would copy the current function definition into src/selection_data.rs and rename it.

@EPashkin there's no support for directly renaming things in gir, is there? Or maybe we should implement support for renaming directly in gir and even make use of the rename annotation?

In any case, @GuillaumeGomez please merge this PR for the time being :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 23, 2019

Yes sir!

@GuillaumeGomez GuillaumeGomez merged commit c0d210c into gtk-rs:master Jan 23, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 23, 2019

@sdroege I seen new_name= for parameters, but it seems not works with function (not tested).
IMHO we better try using rename annotation
In both cases may problem with docs.

@jsparber

This comment has been minimized.

Copy link
Contributor Author

jsparber commented Jan 23, 2019

I actually forgot about this MR myself. And i didn't have time to implemented the getter. @sdroege Did i understand it correctly that we should just extant gir witht the rename functionality?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 23, 2019

Yes

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 24, 2019

@jsparber It was done in #762

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 24, 2019

We still need to add support to gir for doing this automatically though :)

@jsparber

This comment has been minimized.

Copy link
Contributor Author

jsparber commented Jan 24, 2019

awesome!

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.