glib: Panic in Object::new() and related functions and add try_new()

[sdroege]

Fixes https://github.com/gtk-rs/gtk-rs-core/issues/766

---

Depends on https://github.com/gtk-rs/gir/pull/1380

[sdroege]

Before I continue updating everything, @GuillaumeGomez @bilelmoussaoui does this look OK to you?

[bilelmoussaoui]

Looks good to me yes, thank you!

[sdroege]

Actually I'm wondering if we really need all those try functions (also: try_set_property() etc). Can you think of a situation where this is actually useful to handle and not a programmer error? In C we don't have any such try functions and the errors are g_critical()s.

[bilelmoussaoui]

If I remember correctly, people complained thinking there was no try_*_property variants when we made those functions panic by default. I wonder if those cases make sense or we should just drop those APIs. I don't remember who complained, it was someone from the gstreamer community iirc.

[sdroege]

Hmm I can't really come up with a reason for having the non-panicking variant :) Maybe it came from a misunderstanding that setting properties can fail (it can't, see ObjectImpl::set_property signature)?

[sdroege]

So how should we move forward here? I don't want to unnecessarily add lots of try functions if we decide they're not useful :) (There are more to be added for (Async)Initiable FWIW).

[sdroege]

Maybe @fengalin or @gdesmott have some opinions

[bilelmoussaoui]

cc @philn as well

[sdroege]

cc @thiblahute @MathieuDuponchelle

[fengalin]

I'm not sure I have the big picture in mind as I have only followed gtk-rs remotely since a year or so. The only case I can think of would be a workspace with a dynamic language extension that would feed the user back when they request an incompatible property. That seems like a very niche case and we could probably accept such an application relied on ffi.

There's an important case of try_* in gstreamer-rs, though not directly related to gobject. I feel the need to mention it here so that we keep it in mind and refrain from concluding for the lack of need for try_* in gtk-rs-core. I'm thinking about the Formats integer new types, which in C represent a subset of the integer range. We use the TryFromGlib trait and related types for this.

Back to properties, there is an invasive case of not panicking function IMO. In most ObjectImpl::set_property implementations, we use this kind of pattern:

    match pspec.name() {
        "ttl" => {
            settings.ttl = value.get().expect("type checked upstream");
        }
        [...]
    }

OC, expect can be replaced by unwrap for concision: the message only states that the framework is expected to perform the (here inferred) type check before calling set_property. In my opinion, Value::get should just panic in the event of a type mismatch. We could add Value::try_get for the cases where the Value's type can't be checked beforehand if it doesn't boil down to the same niche case as for properties.

[sdroege]

That seems like a very niche case and we could probably accept such an application relied on ffi.

Or it does exactly the same it would do in C: Check first if a property of the given type exists. That's 1 line of code extra or so :)

There's an important case of try_* in gstreamer-rs, though not directly related to gobject [...]

Yeah that's a completely different situation, I agree :)

We could add Value::try_get for the cases where the Value's type can't be checked beforehand if it doesn't boil down to the same niche case as for properties.

You could check it beforehand, or you could get it as a glib::Value and then handle it dynamically. I think that's both fine too (i.e. don't provide a try variant of the function here).

[thiblahute]

I have used try_set_property in the case I use properties that were added in a specific version. I could check either the version or if the prop exists before setting it instead in any case.

[MathieuDuponchelle]

Also note that this all makes me think of GInitable, which exists specifically for fallible constructors, https://docs.gtk.org/gobject/tutorial.html#object-construction goes as far as explicitly stating that It is important to note that object construction cannot ever fail.

[sdroege]

To extend on that, property setting/getting also cannot ever fail :)

[sdroege]

So I think the conclusion is that we don't have try variants for object construction, getting and setting properties.

Should the try variants for signal connection/emission also be removed then? That's basically the same story as for properties, and for the rare case where checking things is needed there is API available.

[MathieuDuponchelle]

I don't know what my own conclusion here is to be honest :) I think a try version would be useful, ideally with a more descriptive return value than just True / False

[sdroege]

How would you use it in practice, and wouldn't it be cleaner in those cases to first check and then do things instead of just doing things and seeing if it fails?

[fengalin]

IMO, if we decide the try_* are useful, this is because we take advantage of the internal type check that panics in the non-try variant so that users get the type check error or the ok result in a single call (just like for the similar method in the std API). Or am I missing something?

[MathieuDuponchelle]

Well, you're describing the difference between EAPF and LBYL, and I don't know what the preferred approach is with rust :) Re what I would do, I don't know, like @fengalin the use case I think of is presenting users with useful errors.

[sdroege]

because we take advantage of the internal type check that panics in the non-try variant so that users get the type check error or the ok result in a single call (just like for the similar method in the std API)

For properties there's also a single function that checks for a property existence/type though.

I think the bigger question here is: Is there any use case where it makes sense to handle the Err case without a panic where it wouldn't have been cleaner to check beforehand if the e.g. property exists?

[fengalin]

I think the bigger question here is: Is there any use case where it makes sense to handle the Err case without a panic where it wouldn't have been cleaner to check beforehand if the e.g. property exists?

I agree with this and from my limited use case imagination, I don't think we need that.

However, if we think it could be useful, I think the way to go would be to allow users to do the try_* thing, which boils down to:

• Check the type, return an error if it fails.
• Return the value / object.

Instead of:

• Check the type, return an error if it fails.
• If above succeeded, call the get function, which will:
  • Check the type, panic if it fails.
  • Return the value / object.

[MathieuDuponchelle]

I think the bigger question here is: Is there any use case where it makes sense to handle the Err case without a panic where it wouldn't have been cleaner to check beforehand if the e.g. property exists?

Note that g_object_new_is_valid_property makes another check, and logs a critical when you try to set a construct / construct only property multiple times. Also checks for writability.

[sdroege]

Note that g_object_new_is_valid_property makes another check, and logs a critical when you try to set a construct / construct only property multiple times. Also checks for writability.

We should probably add that first check also to our code here. Writability is already checked.

[sdroege]

Ok so the conclusion is: no try variants for constructor, set/get property, signals?

[MathieuDuponchelle]

Ok so the conclusion is: no try variants for constructor, set/get property, signals?

What I can say is that I've never used the try version for set and get, and don't think I would use one for constructors either.

[sdroege]

Well that's a lot of code and functions that go away :)

[sdroege]

@bilelmoussaoui @GuillaumeGomez Can you give this a short review? I'd like to merge this today and get the other repos updated then, so that tomorrow this change at least is out of the way.