experiment: requests v2

[GeopJr]

sup: #1481
fix: #1480

[bugaevc]

Some quick notes

[bugaevc]

Suggested change

	if (this.cancellable != null && !this.cancellable.is_cancelled ()) this.cancellable.cancel ();
	this.cancellable?.cancel ();

GLib.Cancellable.cancel () already performs the already-cancelled check.

[GeopJr]

Removed the is_cancelled check, but fwiw, I'm hesitant on using optional chaining in vala due to #1002 and f5bd1c9 (couldn't reliably reproduce to report upstream)

[GeopJr]

Thanks for the review!

Maybe the API should change again and return the inputstream instead of outing it, now that we don't need to return bool

[bugaevc]

Yeah that makes sense too.

[GeopJr]

Moving forward with replacing all instances, something that caught my attention is vala's construction order:

// Home.vala, moving the refreshing away from idle and to just the async function
	construct {
		url = "/api/v1/timelines/home";
		label = _("Home");
		icon = "tuba-user-home-symbolic";

fails with url == null

but

    public Home () {
        Object (
            url: "/api/v1/timelines/home",
	    label: _("Home")
        );
    }

works. At the same, the Notifications.vala url depends on the account, which is not available during Notifications () {, but leaving it as is doesn't cause any url == null

🤔 Personally, I'd expect construct to run before using the Object at all. I know I'm wrong now, but I thought it was like this: construct -> Home () -> Object ()

[GeopJr]

More annoyances:

public Views.ContentBase add_timeline_tab (string label, string icon, string url, Type accepts, string? empty_state_title = null, string? empty_state_icon = null) {
		var tab = new Views.Accounts () {
			url = url,
			label = label,
			icon = icon,
			accepts = accepts
		};

url == null, but introducing this works:

public class Tuba.Views.Accounts : Views.Timeline {
	public Accounts.prefilled (string url, string label, string icon) {
		Object (
			url: url,
			label: label,
			icon: icon
		);
	}

[bugaevc]

On top of GObject's already complicated construction/initialization flow, Vala adds its own layer of confusion 😭 I hate everything about the way it's done, and it's unlikely that I could change much here about Vala semantics (especially given the state of Vala maintenance, which makes it hard to get any non-trivial change merged into Vala).

You would think new Type () { prop1= value1, prop2 = value2 }; syntax calls g_object_new (THE_TYPE, "prop1", value1, "prop2", value2, NULL), right? But it doesn't, instead it calls the regular constructor (the_type_new ()) followed by setting the individual properties.

Properties whose values are visible at constsruct { } time are the ones that both:

• are "construct properties", i.e. marked construct or construct set
• were passed to the g_object_new call, i.e. Object () chain-up in Vala

What you mean about Home.vala and URL being null, I couldn't really follow (more details please?), but possibly you're reading the value of the url property (or starting an async operation which does just that) from the construct block of a parent class, but setting that property from the construct block of a derived class? The derived class's construct runs later than the parent's, hence the value is not yet set at that point — if that's what you're doing & running into.

From the GObject standpoint, the right thing to do is:

1. Passing as many properties as possible into the g_object_new call, as opposed to creating an instance first and setting properties later — i.e. the opposite of what new Type () { prop = value } syntax does in Vala. You could achieve this in Vala by calling g_object_new more explicitly (Object.@new (typeof (Type), prop: value)), but this is somewhat ugly; alternatively, creating special-case constructors with Object () chain-ups, as in your Accounts.prefilled example — also cumbersome.
2. Generally, doing/starting various things in property setters, when a value is set for a property, and not "upon creation". E.g. I imagine a request could be started from the url property's setter. This way, if the object does get created without values for all these properties, those "various things" do happen when the values do get set. It's also a good opportunity to make the property re-settable, i.e. start the setter by undoing whatever setup it did for the old value (e.g. cancelling an old request, if any, when the URL changes). For example, see how Gtk.DirectoryList reacts when its file property is set (gtk_directory_list_set_file).
3. When doing things in setters is not feasible, marking properties construct and doing things in the constructed vfunc; Vala's construct blocks actually translate to the constructor vfunc, which is worse, but close enough.

Here's a piece of code for you to play with:

#! /usr/bin/vala

public class Base : Object {
	public int some_prop {
		set { print ("Setting some_prop to %d\n", value); }
	}
	public int some_construct_prop {
		construct set { print ("Setting some_construct_prop to %d\n", value); }
	}

	public Base () {
		print ("Base: before chain-up\n");
		Object (some_prop: 42);
		print ("Base: after chain-up\n");
	}

	construct {
		print ("Base: construct block\n");
	}
}

public class Derived : Base {
	public Derived () {
		print ("Derived: before chain-up\n");
		base ();
		print ("Derived: after chain-up\n");
	}

	construct {
		print ("Derived: constrct block\n");
	}
}

void main () {
	new Derived ();
}

I get:

Derived: before chain-up
Base: before chain-up
Setting some_construct_prop to 0
Base: construct block
Derived: constrct block
Setting some_prop to 42
Base: after chain-up
Derived: after chain-up

[GeopJr]

What you mean about Home.vala and URL being null, I couldn't really follow (more details please?), but possibly you're reading the value of the url property (or starting an async operation which does just that) from the construct block of a parent class, but setting that property from the construct block of a derived class? The derived class's construct runs later than the parent's, hence the value is not yet set at that point — if that's what you're doing & running into.

Timeline.vala inherits from AccountHolder which when constructed refreshes the timeline (initial fetch), and that's where url is null. Prior to changing everything to the async one, it would do the request in idle_add (also why we don't come across it in the current state of this PR) and 🤞 we were lucky it finished after the constructions. But now it seems that running the async request directly is faster. (FWIW, url is construct set)

I imagine a request could be started from the url property's setter

That sounds good, but currently it's more tied to the account (so changing accounts refreshes the timelines as the new account) and there's also other things in some timelines (from pagination to filters and api versions; that might not need immediate fetching). I'll see if I can do it without breaking too many things.

When doing things in setters is not feasible, marking properties construct and doing things in the constructed vfunc; Vala's construct blocks actually translate to the constructor vfunc, which is worse, but close enough.

I think I'll go with this. Right now, I've been literally just moving them to:

    public Home () {
        Object (
            url: "/api/v1/timelines/home",
            label: _("Home")
        );
    }

constructed looks less random "let's hope the order is right with this one".

Here's a piece of code for you to play with:

as suspected, I really did think it was base construct -> derived construct -> derived before chain-up -> base before chain-up -> base after chain-up -> derived after chain-up :p I wonder what other things are wrong in Tuba from that alone...

---

especially given the state of Vala maintenance, which makes it hard to get any non-trivial change merged into Vala

I read that, and honestly, what happens next depends on Rico who hasn't really acknowledged it as far as I can tell. Outside of GNOME/Elementary, maybe the Frida / NowSecure people would be interested in moving Vala forward? (since it's written in Vala)

Speaking of Elementary, they use lambdas a lot... https://github.com/search?q=org%3Aelementary%20%3D%3E&type=code one can only imagine the memory benefits they could get by getting those ref loops fixed