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

Into wrapping #647

Merged
merged 1 commit into from Oct 28, 2018

Conversation

Projects
None yet
2 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Oct 27, 2018

This code is currently a huge mess and generate stuff like:

fn set_property_cell_background<'a, P: Into<Option<&'a Option<&str>>>>(&self, cell_background: P) {

Which is pretty funny but not what I want. So my main question is:

Why does it generate two Option wrapping instead of one? (I assume the Into wrapping generates one over a type which already has one)

A bit of help would be very appreciated here (I think I did it completely wrong). :)

Once this part done, I'll just have to generate the .into call and we'll be good to go I think.

bound_type.clone(),
false);
bound = codegen::function::bounds(&bounds, &[], false);
format!("{}", bounds.iter().next().unwrap().alias)

This comment has been minimized.

@EPashkin

EPashkin Oct 28, 2018

Member

Alias from inner bounds used while bounds destroyed,
maybe just change to TYPE_PARAMETERS_START ?

This comment has been minimized.

@EPashkin

EPashkin Oct 28, 2018

Member

Ignore this, was wrong.

This comment has been minimized.

@EPashkin

EPashkin Oct 28, 2018

Member

May be this helps: fix for non-trait cases:

--- a/src/codegen/properties.rs
+++ b/src/codegen/properties.rs
@@ -1,5 +1,6 @@
 use std::io::{Result, Write};
 
+use analysis;
 use analysis::bounds::{Bound, Bounds};
 use analysis::properties::Property;
 use analysis::rust_type::{parameter_rust_type, rust_type};
@@ -89,9 +90,10 @@ fn declaration(env: &Env, prop: &Property) -> String {
         }) = prop.bound
         {
             if bound_type.is_into() {
+                let type_name = parameter_rust_type(env, prop.typ, dir, library::Nullable(false), analysis::ref_mode::RefMode::ByRefFake).into_string();
                 let mut bounds = Bounds::default();
                 bounds.add_parameter(&prop.var_name,
-                                     &parameter_rust_type(env, prop.typ, dir, prop.nullable, prop.set_in_ref_mode).into_string(),
+                                     &type_name,
                                      bound_type.clone(),
                                      false);
                 bound = codegen::function::bounds(&bounds, &[], false);
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Oct 28, 2018

I was able to get a working version (thanks @EPashkin!).

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 28, 2018

In enums.rs no newline between Display and ToGlib

let bound_type = match *type_ {
Fundamental(_) => Some(bound_type.clone()),
_ => match bound_type {
BoundType::Into(_, Some(ref x)) => Some(*x.clone()),

This comment has been minimized.

@EPashkin

EPashkin Oct 28, 2018

Member

Despite this branch only string (&str) properties converted.
This strange.
Ex. for me fn set_property_cell_background_rgba<'a, P: Into<Option<&'a gdk::RGBA>>>(&self, cell_background_rgba: P) { works in fine, and don't needed extra '+SetValueOptional` etc.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 28, 2018

Author Member

Yes but for now we only use Into on Fundamental types and I'd like to keep it this way until we fully switch all IsA implementations.

This comment has been minimized.

@EPashkin

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:into-wrapping branch from 3a7a0d2 to e734d76 Oct 28, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Oct 28, 2018

So I think it's ready. Tell me if anything needs to be updated. Once merged, I'll send PRs to other repositories.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 28, 2018

If you think that this enough for this PR, please, squash to single commit.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:into-wrapping branch from e734d76 to f5ff12b Oct 28, 2018

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Oct 28, 2018

Done!

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 28, 2018

@GuillaumeGomez Thanks.
Affected crates: gio, gtk, sourceview.

@EPashkin EPashkin merged commit d7c3f32 into gtk-rs:master Oct 28, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:into-wrapping branch Oct 28, 2018

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Oct 28, 2018

.. so 👍 for these fix in advance

This was referenced Oct 28, 2018

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.