feat(qttypes): Do more wrappers around QVariant

[ratijas]

Wrapped converters to and constructors from all available types. The rest
of them are marked as TODO comments for future implementers.
Methods and constructors, which I'm not sure about whether they are needed
to be exposed in Rust at all, are marked with TODO?: prefix.

Constructors are reordered according to the official Qt documentation
for ease of code navigation. Return types are shortened to Self.

Fixed some misleading links to upstream docs in From<{i,u}{32,64}> impls.

Some wrappers are impossible to write at this point without introducing
QMetaType and its Type enum in this crate or its dependencies, as currently
it would create a circular deps between qmetaobject and qttypes crates.
Such hypothetical implementations are left commented out for future ideas.

---

I acknowledge that this is some controversial big boy commit droppin' here like a bomb. I ask to carefully review it, and discuss key design concerns outlined below.

1. Wouldn't it be better if all the QVariant::toSomething() methods return Option<Something> in rust, by checking for canConvert() first? Would be much more Rusty way, at the cost of deviating from Qt origins.
2. QVariant(const Something &val) constructors actually take values by references, they never really consume original value. Wouldn't it make more sense to add & to all those impl From<Something> for QVariant i.e. impl From<&Something> for QVariant?
3. Comments with links are missing from implicitly generated basic traits by cpp! macro. No idea what to do.
4. Do we really want all those TODOs stay there probably forever?

[ogoffart]

I only made a partial review so far.

In general, i don't think it is worth it to try to exhaustively wrap every call and expose the full API.
The philosophy behind this whole project is to come up with a idiomatic API to use QML from Rust without having to write C++ or learn Qt. And to do so in a most efficient way.
Yes, we also want to keep close to the Qt API and i understand this is a conflicting goal, but in doubt, better have idiomatic rust API than blindly imitate Qt.
For example, all these conversion might not be worth it when there is the QMetaType trait doing them. And also the From and Into trait.

Wouldn't it be better if all the QVariant::toSomething() methods return Option in rust, by checking for canConvert() first? Would be much more Rusty way, at the cost of deviating from Qt origins.

Yes, i agree, it should

QVariant(const Something &val) constructors actually take values by references, they never really consume original value. Wouldn't it make more sense to add & to all those impl From for QVariant i.e. impl From<&Something> for QVariant?

It all depends if the function can sonsume (take ownership) of the memory or not. If not, then it might indeed be better to take value by reference.

Comments with links are missing from implicitly generated basic traits by cpp! macro. No idea what to do.

I think that's fine. These links wouldn't really help anyone IMHO.

Do we really want all those TODOs stay there probably forever?

No.

[ogoffart]

I think we don't need to expose this function in this crate.

The function in the QMetaType trait should be enough.

[ratijas]

OK

[ogoffart]

In general, we don't have to replicate the whole API, only the API that make sense. This clearly make no sense in rust.
Same for the function that operates on the QMetaType which are better done by the QMetaType trait.

[ratijas]

agreed.

[ogoffart]

This function also doesn't make sense in rust. One would use the std::mem::swap function.

[ratijas]

yeah, kinda thought about std::mem::swap as well. It was so weird to work with reference to a pointer passed through FFI, that I even wrote a test to make sure everything matched up.

[ogoffart]

Is there a particular use case for this function?

[ratijas]

Idk, just a shortcut, a convenience. Kinda making it more Rust-oriented and usable for non-Qt code.

I wanted to name it to_vec, but it would cause all sorts of conflicts with toList() -> QVariantList.

[ogoffart]

I guess i'd call this to_f64 instread

[ratijas]

then, it would apply to all the others in family as well

[ratijas]

Overall, a big thanks for such a fast review. I'm going to call it a day for now, re-think about it overnight, and come up with (suggested) changes tomorrow.

Side note: As much as I'd like to conduct it in Gerrit-style, GitHub doesn't allow viewing inter-patchset diff, so I'll be adding new commits, but manually squash before final merge.

[ratijas]

I've finally had a true weekend, phew. So didn't have a chance to code, but gave it a few thoughts.

I totally agree that in order to make it the Rust way, all those to_something() methods should return an optional value. In fact, they can be implemented as TryInto trait with unit () error type, and we should teach folks to use .try_into().unwrap_or_default() if they insist on Qt-like behavior. Such documentation shall be located at Wrapper-specific section of the QVariant type itself.

Once thing that still bothers me, is that canConvert() does not guarantee the success of actual conversion, like in String -> UInt directions:

let mut v = QVariant::from(QString::from("Yahaha!"));
let uint_type = 3_i32;
// According to QVariant::canConvert(int) rules, QString can be converted to UInt,
assert!(v.can_convert(uint_type));
// but result of such an attempt may fail miserably.
assert_eq!(v.to_uint(), None);

which means, for those methods, it might be reasonable to check either both or only bool *ok, but not only canConvert() result.

Will try my best to push updates ASAP.

[ratijas]

Wanted to implement to_hash_map() as a feature parity for to_bytes() -> Vec<u8>. But it ended up looking so scary, I gave up all hopes on passing any sane code review. Here it is, for archeologists with a lot of historical interest.

pub fn to_hash_map(&self) -> HashMap<String, QVariant> {
    // QVariant::toHash() is guaranteed to be cheap, fast and constant time O(1).
    // If `self` is not a QHash, an empty one will be returned.
    let size = cpp!(unsafe [self as "const QVariant*"] -> i32 as "int" {
        return self->toHash().size();
    });
    let mut map = HashMap::with_capacity(size as usize);
    let m = &mut map; // Rust's HashMap is opaque for C++
    cpp!(unsafe [self as "const QVariant*", m as "void*"] {
        using namespace std;
        QVariantHash hash = self->toHash();
        QHashIterator<QString, QVariant> i(hash);
        while (i.hasNext()) {
            i.next();
            QString key = i.key();
            QVariant value = i.value();
            rust!(QVariant_toHash_insert [
                m: &mut HashMap<String, QVariant> as "void*",
                key: &QString as "QString&",
                value: &QVariant as "QVariant&"
            ] {
                m.insert(key.clone().into(), value.clone());
            });
        }
    });
    map
}

[ratijas]

For now, it was only a rebase. Not worth looking into.