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

Manually bind Binding::unbind() #360

Merged
merged 3 commits into from Jul 26, 2018

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Jul 25, 2018

It is wrongly marked as transfer-none

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 25, 2018

Member

Code looks good, but maybe better fix .gir file and change this PR to only remove trait generation?

Member

EPashkin commented Jul 25, 2018

Code looks good, but maybe better fix .gir file and change this PR to only remove trait generation?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

I thought we generally prefer to only edit gir files if it can't be easily solved otherwise

Member

sdroege commented Jul 25, 2018

I thought we generally prefer to only edit gir files if it can't be easily solved otherwise

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 25, 2018

Member

IMHO it was before we get normal script for update gir files.
Now we have stable way to reapply changes.

Member

EPashkin commented Jul 25, 2018

IMHO it was before we get normal script for update gir files.
Now we have stable way to reapply changes.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

@GuillaumeGomez What do you prefer?

Member

sdroege commented Jul 25, 2018

@GuillaumeGomez What do you prefer?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 25, 2018

Member

I have a preference to @EPashkin's proposition.

Member

GuillaumeGomez commented Jul 25, 2018

I have a preference to @EPashkin's proposition.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

Ok, I don't know how to use this thing. How would this look correct?

xmlstarlet ed -P -L \
    -u '//_:class[@name="Binding"]/_:method[@name="unbind"]//_:instance-parameter[@name="binding"]/@transfer' -v "full" \
    GObject-2.0.gir
Member

sdroege commented Jul 25, 2018

Ok, I don't know how to use this thing. How would this look correct?

xmlstarlet ed -P -L \
    -u '//_:class[@name="Binding"]/_:method[@name="unbind"]//_:instance-parameter[@name="binding"]/@transfer' -v "full" \
    GObject-2.0.gir
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 25, 2018

Member

Typo: "/@transfer-ownership" instead just "transfer"

Member

EPashkin commented Jul 25, 2018

Typo: "/@transfer-ownership" instead just "transfer"

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

Ah

Member

sdroege commented Jul 25, 2018

Ah

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

gtk-rs/gir-files#24 and after that is merged I'll update this one here

Member

sdroege commented Jul 25, 2018

gtk-rs/gir-files#24 and after that is merged I'll update this one here

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 25, 2018

Member

The other PR has been merged. :)

Member

GuillaumeGomez commented Jul 25, 2018

The other PR has been merged. :)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

Updated accordingly

Member

sdroege commented Jul 25, 2018

Updated accordingly

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Jul 25, 2018

👍

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 25, 2018

Member

Looks good, but maybe better combine "Regenerate with latest gir-files" with "Don't generate a trait for Binding", or combine all 3 commits as "Don't generate a trait for Binding"

Member

EPashkin commented Jul 25, 2018

Looks good, but maybe better combine "Regenerate with latest gir-files" with "Don't generate a trait for Binding", or combine all 3 commits as "Don't generate a trait for Binding"

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

Which of the two options you gave? Those two options and the three commits make no difference to me :)

Member

sdroege commented Jul 25, 2018

Which of the two options you gave? Those two options and the three commits make no difference to me :)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 25, 2018

Member

Personally I don't like last commit with one line change,
and prefer one commit option as it clear states what it do.

Member

EPashkin commented Jul 25, 2018

Personally I don't like last commit with one line change,
and prefer one commit option as it clear states what it do.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

Ok, for that @GuillaumeGomez can simply click on the "rebase & squash" button when he merges. That's most easy

Member

sdroege commented Jul 25, 2018

Ok, for that @GuillaumeGomez can simply click on the "rebase & squash" button when he merges. That's most easy

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 25, 2018

Member

I didn't know I could. If you're fine with this then I'll click on "the" button. :p

Member

GuillaumeGomez commented Jul 25, 2018

I didn't know I could. If you're fine with this then I'll click on "the" button. :p

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 25, 2018

Member

@GuillaumeGomez, "Merge" button usually has popup with multiple cases.

Member

EPashkin commented Jul 25, 2018

@GuillaumeGomez, "Merge" button usually has popup with multiple cases.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 25, 2018

Member

@sdroege You forgot to add right versions.txt

Member

EPashkin commented Jul 25, 2018

@sdroege You forgot to add right versions.txt

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

Indeed, I add that later when I'm back at the computer

Member

sdroege commented Jul 25, 2018

Indeed, I add that later when I'm back at the computer

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

Now with correct versions.txt :)

Member

sdroege commented Jul 25, 2018

Now with correct versions.txt :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 25, 2018

Member

Thanks! (just waiting for CIs and then I merge)

Member

GuillaumeGomez commented Jul 25, 2018

Thanks! (just waiting for CIs and then I merge)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

All green :)

Member

sdroege commented Jul 25, 2018

All green :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 26, 2018

Member

Then I merge!

Member

GuillaumeGomez commented Jul 26, 2018

Then I merge!

@GuillaumeGomez GuillaumeGomez merged commit a571d4a into gtk-rs:master Jul 26, 2018

1 of 2 checks passed

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