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

Adding TreeRowReference #387

Merged
merged 2 commits into from Sep 9, 2017

Conversation

Projects
None yet
5 participants
@thk1
Contributor

thk1 commented Oct 16, 2016

Adding TreeRowReference (persistent reference to a node in a TreeModel). new_proxy() and related methods are ignored since reordered() can't be auto-generated. However, proxy references are "not generally needed by most applications." (cit. from docs)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Oct 17, 2016

Member

IMHO reordered fact that not generated is no reason to ignore it (without comment we just forget about that function and never implement it 😉 ).
Same as for other: while it normally used together with reordered, IMHO there are cases where allowed only insert\remove without reordering and new_proxy can be safely used

Member

EPashkin commented Oct 17, 2016

IMHO reordered fact that not generated is no reason to ignore it (without comment we just forget about that function and never implement it 😉 ).
Same as for other: while it normally used together with reordered, IMHO there are cases where allowed only insert\remove without reordering and new_proxy can be safely used

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Oct 19, 2016

Member

Seems good for me. Waiting for @gkoz's opinion.

Member

GuillaumeGomez commented Oct 19, 2016

Seems good for me. Waiting for @gkoz's opinion.

@thk1

This comment has been minimized.

Show comment
Hide comment
@thk1

thk1 Oct 19, 2016

Contributor

@EPashkin Maybe you are right. Generation of new_proxy/inserted/deleted is now enabled. I also added a manual implementation of reordered(). TreeRowReference bindings are now complete.

Contributor

thk1 commented Oct 19, 2016

@EPashkin Maybe you are right. Generation of new_proxy/inserted/deleted is now enabled. I also added a manual implementation of reordered(). TreeRowReference bindings are now complete.

@EPashkin

Thanks.

Show outdated Hide outdated src/tree_row_reference.rs Outdated
Show outdated Hide outdated src/auto/tree_row_reference.rs Outdated
Show outdated Hide outdated src/auto/tree_row_reference.rs Outdated
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Nov 2, 2016

Member

Added nullable constructor support to gir.
@gkoz Better add nullable for TreePath.new_from_string here or we add it separately?

Member

EPashkin commented Nov 2, 2016

Added nullable constructor support to gir.
@gkoz Better add nullable for TreePath.new_from_string here or we add it separately?

@thk1

This comment has been minimized.

Show comment
Hide comment
@thk1

thk1 Sep 6, 2017

Contributor

All requested changes have been carried out. I just rebased the PR and tested the code again. I think it should now be ready to be merged.

Contributor

thk1 commented Sep 6, 2017

All requested changes have been carried out. I just rebased the PR and tested the code again. I think it should now be ready to be merged.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 7, 2017

Member

@thk1 There no TreeRowReference generated and src/tree_row_reference.rs not added as module.
So this PR currently adds nothing.

Member

EPashkin commented Sep 7, 2017

@thk1 There no TreeRowReference generated and src/tree_row_reference.rs not added as module.
So this PR currently adds nothing.

@thk1

This comment has been minimized.

Show comment
Hide comment
@thk1

thk1 Sep 7, 2017

Contributor

@EPashkin Ok, I wasn't sure if a regen should be part of the PR. The mod is now imported and auto/mod.rs and auto/tree_row_reference.rs are (re)generated.

Contributor

thk1 commented Sep 7, 2017

@EPashkin Ok, I wasn't sure if a regen should be part of the PR. The mod is now imported and auto/mod.rs and auto/tree_row_reference.rs are (re)generated.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 7, 2017

Member

Seems all good now, waiting for CI just in case

Member

EPashkin commented Sep 7, 2017

Seems all good now, waiting for CI just in case

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 7, 2017

Member

Thanks, @thk1

@GuillaumeGomez 👍 for this.

Member

EPashkin commented Sep 7, 2017

Thanks, @thk1

@GuillaumeGomez 👍 for this.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 7, 2017

Member

Just two nits and we're good to go.

Member

GuillaumeGomez commented Sep 7, 2017

Just two nits and we're good to go.

thk1 added some commits Sep 6, 2017

@thk1

This comment has been minimized.

Show comment
Hide comment
@thk1

thk1 Sep 7, 2017

Contributor
Contributor

thk1 commented Sep 7, 2017

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 9, 2017

Member

@GuillaumeGomez, merge this please, before @sdroege's

Member

EPashkin commented Sep 9, 2017

@GuillaumeGomez, merge this please, before @sdroege's

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 9, 2017

Member

Does not interfere with my PRs AFAIU. Should be good to merge before or after without problems.

Member

sdroege commented Sep 9, 2017

Does not interfere with my PRs AFAIU. Should be good to merge before or after without problems.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 9, 2017

Member

Thanks!

Member

GuillaumeGomez commented Sep 9, 2017

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit f0f1322 into gtk-rs:master Sep 9, 2017

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