-
-
Notifications
You must be signed in to change notification settings - Fork 62
Conversation
See #678 (comment) . Can you also add that here for symmetry? :) |
src/variant.rs
Outdated
@@ -725,6 +725,12 @@ where | |||
} | |||
} | |||
|
|||
impl FromVariant for Variant { | |||
fn from_variant(variant: &Variant) -> Option<Self> { | |||
Some(variant.clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this and not Variant::new_variant()
? I would expect this to box the variant in another variant (and that the behaviour for glib::Value
is currently the same as what you implement here is a bug that I'm going to fix in the near future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. new_variant()
creates a variant of type v
instead of the original type. My thought was that it could be useful to use the abstractions in the new container API when one already has a variant of the type one wants, but needs to put it in a tuple or some such.
In any case, I don't think it's new_variant()
that should be used here, but rather get_variant()
. (new_variant()
would be used in ToVariant
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I do like how in the code I showed in #678 (comment), I could use this implementation of FromVariant
to partially deconstruct a complicated Variant
type.
I don't know if there's a clean way to support both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I do like how in the code I showed in #678 (comment), I could use this implementation of FromVariant to partially deconstruct a complicated Variant type.
While I agree that this is convenient, this seems more like a hack then a nice solution :) By having this implemented the API is asymmetric and the Variant
wrapping is not possible to express anymore.
In any case, I don't think it's new_variant() that should be used here, but rather get_variant(). (new_variant() would be used in ToVariant.)
Indeed, sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rebased on master and changed to using .variant()
and .get_variant()
.
For my immediate use case, I think the .iter()
method is probably a better way to go through a collection variant without fully deconstructing it, so I'm mostly happy with this implementation, supposing the gtk-rs maintainers all agree it should work this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for having a type that allows deconstructing via a "wildcard", you could add some dummy type that implements the trait and just does nothing, or not?
3e80e14
to
633b61a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https:/gtk-rs/glib/pull/679/files#r483437430
@zeenix @GuillaumeGomez Do you have any opinions?
Trying to use the current implementation to access D-Bus property change notifications with their dd.connect_local("g-properties-changed", true, move |args| {
let new_props = args[1]
.get::<glib::Variant>()
.unwrap()
.unwrap()
.get::<std::collections::HashMap<String, glib::Variant>>()
.unwrap();
None
}) It does work now:
But I get the aforementioned "boxed" variants and there doesn't seem to be a way to access the inner variant in these, since the instances just return the outer variant itself :/ |
You need to use the |
Oh. Thanks. idk why I keep staring at docs.rs for the release version expecting it to show something new from a PR.. (or from git from that matter) :D |
hmm..
|
(using (why do they exist?) |
This is useful when combined with the container variant functions and trait implementations from gtk-rs#651. BREAKING CHANGE: This remove `impl<T: ToVariant> From<T> for Variant`, which made it impossible to implement `ToVariant` since it conflicts with the standard library `impl<T> From<T> for T`: gtk-rs#678
633b61a
to
f6b61f3
Compare
@@ -529,12 +527,6 @@ impl ToVariant for str { | |||
} | |||
} | |||
|
|||
impl<T: ToVariant> From<T> for Variant { | |||
fn from(value: T) -> Variant { | |||
value.to_variant() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This impl also had the problem that it took its argument by value, i.e. took ownership of it and consumed it, but then only used it by reference. This is rather inefficient...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. @EPashkin @zeenix @GuillaumeGomez ?
Already fixed in master.
What? |
@sdroege sounds good to me. FWIW, this is what I was doing in zvariant as well but @elmarco insisted that use of std traits rather than our own, was more important than a uniform API to deal with transformation of all types (including Variant itself) to/from Variant type. The huge drawback of not having a uniform API is that user has to be conscious of Variant boxing. So what I'm saying is that I believe this (PR) and custom traits is the best way forward! |
@zeenix the |
This is useful when combined with the container variant functions and trait implementations from #651.
Implementing
ToVariant
would be useful as well, but isn't currently possible: #678