Allow to build multiple actors from a BuilderUtilizer

[piegamesde]

I had that idea in mind that matched my use case pretty well, but it somewhat clashes with other usage styles of the BuilderUtilizer. I got rid of all compile error except one (I think), and didn't yet bother to clean things up or document.

Closes #3.

[idanarye]

This is a big PR, and I've only skimmed it (maybe I'll have more time to thoughtfully read it during the weekend), but this is what I understood so far:

1. You removed the actor-widgets-signal generics from BuilderUtilizer so that it could be called multiple times with different actors.
2. You changed the BuilderSignal macro so that instead of generating signal handlers on demand by name it'll populate a hashmap with all of them.
3. BuilderUtilizer keeps a hashmap of these signal handlers, and you can attach multiple actors, each with their own signal type, and it'll register that signal to the hashmap.
4. connect_builder_signals must be called at the end in order to register all the signal handlers from that hashmap to GTK.

That's about it?

I have three issues with this:

1. I'm not sure if I like it that you always have to generate all the signal handlers. I see that make_signal_handler is generating all the handlers only to take out one of them and throw away the rest. This function is only called from connect_signal_handler which is called from nowhere, but still - if we want to support getting a single signal handler then this does not seem like a reasonable way to do it.

2. This kind of ruins BuilderUtilizer for my usecase, and I was kind of using it. Specifically I'm talking about that compiler error you were unable to get rid of - connect_tagged_builder_signals is not supposed to create a new actor, it's supposed to connect the signals to an existing actor. The idea is to create many instance of the same thing - mainly gtk::ListBoxRow, which I have a fetish for - and control them all from the same actor instead of creating a new actor for each instance, which I found to be too cumbersome in certain cases. The tags are used so that the actor who receives the signal can know where it came from without having to look up the signal's parameters. Having tagged signals for a new actor is pointless - you can just keep that data in the actor.

   This is a specific usecase, but I want to keep BuilderUtilizer flexible for such usecases, and for that it needs its generic parameters in the type itself. How about you create your own type? I don't mind changing the original BuilderUtilizer to TypedBuilderUtilizer and implement it using your new BuilderUtilizer. Or they can both be implemented with a new UnderlyingBuilderUtilizer or something.

3. To avoid users forgetting to call connect_builder_signals, how about making BuilderUtilizer a Drop and panic if it gets dropped while callbacks is not empty?

And one more thing - I have a fix for #2 which is going to conflict with this PR. I haven't pushed it yet because I wanted to finish its tests and documentations, but maybe I should push it now so that you can include it in your design. It passes a optional closure (inhibit_dlg) through the method that generate the signal handlers. inhibit_dlg receives a reference to the parsed signal and returns the signal's return value (though #[signal(inhibit =)] overrides this). My BuilderUtilizer keeps it in a field - perks of knowing the signal type - but your BuilderSignal will need to have it passed to it for each actor you create.

[piegamesde]

Thanks for looking into this. I'm sorry, I should have written a few words of explanation but I forgot. Regarding you critique points:

1. I totally agree with you. The solution is to revert back to the old style of generating signals, but also have another method to list all of them. connect_signal_handler is public API and will be called from the applications.
2. I'm fine with creating separate structs for separate purposes. But I still hope to find an API that fits them all quite nicely. The most basic cases could be solved by adding moving the methods that are specific to one actor type into Factory.
3. I've tagged the type #[must_use] which will generated compiler errors. This is the common way to do this.

---

In the attempt to improve the API, I'll try my best to collect all usages so far into a list:

• One UI file builds one actor. Possibly multiple times.
• One UI file builds multiple actors that may share widgets
• Widgets are extracted from a UI file (to add them to a running actor?)
• Signals are connected to an existing actor
  • The UI file is instanced multiple times and tags are used to distinguish them
• That thing that the Factories macro does. (If I understand it still one UI file to multiple actors, but in a different way: widgets are not shared.)

Some additional notes:

• Each time when building an actor, the widgets should be optional: some may only care about the events.
• The same goes for the signals. An actor without signals may be rather pointless as it finishes immediately, but it may still act on its widgets in the started method. Also, an event type may be added later on.
• These could always made "optional" by implementing the respective traits for (). However, as there are no default types on generic methods one would still have to annotate them. The only way without annotation is to provide separate methods.

[idanarye]

I totally agree with you. The solution is to revert back to the old style of generating signals, but also have another method to list all of them. connect_signal_handler is public API and will be called from the applications.

The parsing of the signal arguments needs no state, so maybe we can have a method on the BuilderSignal that looks like this:

fn signal_parser(signal_name: &str) -> Option<fn(&[Value]) -> Result<Self, woab::Error>>;

And use it to build both one-signal parsers and all-signals parsers? I don't think one more dynamic call is going to make that much of a difference here...

I've tagged the type #[must_use] which will generated compiler errors. This is the common way to do this.

But as soon as you call make_actor Rust considers the BuilderUtilizer as used.

That thing that the Factories macro does. (If I understand it still one UI file to multiple actors, but in a different way: widgets are not shared.)

Maybe I should explain Factories. I made it so I can use gtk::ListBox with ease. gtk::ListBox's rows are full fledged container widgets and I like to use them to represent entries - instead of the usual selector widget (list / combo / prev&next buttons) that populate a fixed set of widgets with the entry's data, I just make a row for each entry. As long as you don't have too many entries (which I assume will make it slow, but I haven't encountered such a case yet), it looks and feels so much nicer.

The problem is that GTK builder does not exactly support this style. You can design rows individually in Glade, but then you have no way to programatically create more of them at runtime. So what I do is design one row (for each type of row) and then extract the XMLs of these rows to separate builder XMLs. This I do with the dissect_builder_xml function - which is quite unsightly, so I made it #[doc(hidden)] and have the woab::Factories derive macro generate dissect_builder_xml call. It creates a factory for the window and separate factories for each row, and allows me to easily create the components I need.

Because its so convenient, I also use it when I have multiple windows - though it's not that necessary there as I can just use multiple .glade files.

[piegamesde]

But as soon as you call make_actor Rust considers the BuilderUtilizer as used.

I see. How about connecting the signals in Drop? The only mis-use case is if somebody keeps the reference for too long, which is less likely.

I think I now understand Factories. Basically, you take out some objects (xml sub-trees) because you prefer this over editing multiple UI files. You can then instance all the parts separately and individually. This sounds really useful, but I'll try to come up with a better name ^^

But I still hope to find an API that fits them all quite nicely.

I have an idea of some typed-builder-pattern-state-machine-thingy that I'm going to try out soon.

[piegamesde]

The idea turned out to work out rather well IMO. Some discussion points before I clean this up:

• I'm not sure about ActorWidgetsBuilder. I could easily get rid of it simply by splitting out the build method into two variants
  • That's the reason why I haven't fully implemented it, so you need to call with_widgets last ^^
• Factory is still parameterized, i.e. is supposed to build one widget. I'm not sure if it is intended to ever use it for more than one widget?
• I still need to figure out the inhibit handling

[idanarye]

Another style we should consider is the closure-with-context style:

factories.app_win.make(|bcx| { // bcx stands for "builder context" so that ctx can be used for the actor context
    // I wish we could use this style, but it probably won't work because it needs to
    // borrow bcx mutably to register the callbacks (unless we use a refcell?)
    bcx.actor().with_signals::<Signal1>().make(|ctx| Actor1 {
        widgets: bcx.widgets(),
    });

    // This style is not as pretty but has better chances to pass the borrow checker
    woab::actor().with_signals::<Signal2>(bcx).make(|ctx| Actor2 {
        widgets: bcx.widgets(),
    })
});

[idanarye]

Wait - maybe we can combine the contexts?

factories.app_win.make(|ctx| { // ctx here is BuilderContext
    ctx.actor(|ctx| { // ctx here is of type ActorBuilderContext<Actor1> which is DerefMut<Target = Actor::Context>
        ctx.connect_signals::<Signal1>();
        Actor1 {
            widgets: ctx.widgets(),
        }
    });
});

I'm kind of hoping that Rust will recognize that the inner closure returns an Actor1 and use it to infer the generic parameter of the ActorBuilderContext, but I it is possible we encounter the same inference limitation that forced me to put all these types into the Factor generics...

[piegamesde]

I'm not sure I understand what the appealing points of this approach are. (At least it solves the widgets problem rather well I think? Does is solve the Drop problem as well?). The proposed code also wouldn't account for adding signals to already existing builders, which I learned is a rather important use case. Maybe it can be fixed by passing in a context externally though? The borrow checker issues can always be circumvented by re-passing btx alongside ctx in the inner closure as a second argument.

For comparison, here's what my code looks like at the moment:

builder
    .new_actor()
    .connect_signals::<LibrarySignal>()
    .with_widgets()
    .build(|_ctx, widgets| {
        let library = futures::executor::block_on(library::Library::load()).unwrap();
        LibraryActor {
            widgets,
            library: Rc::new(library),
        }
    }).unwrap();

And here the "reuse actor" use case:

let mut builder = self.factories.row_addend.instantiate();
builder.connect_signals_tagged(addend_id, ctx);
let widgets = builder.connect_widgets::<AddendWidgets>().unwrap();

[idanarye]

Actually, now that I think about it, my last two comments are orthogonal. The first one, the one about making a context around the builder utilizer, is mainly about solving the Drop problem - which I think is important enough to justify it. The second one, the one about wrapping Actor::Context in our own ActorBuilderContext, is mainly about solving the widgets problem (and also making the signals nicer) and can actually be used even Drop BuilderUtilizer.

So, if we drop (pun intended) the BuilderUtilizerContext idea and just use the ActorBuilderContext idea, your example would looks something like that:

builder.actor(|ctx| {
    ctx.connect_signals::<LibrarySignal>();
    let library = futures::executor::block_on(library::Library::load()).unwrap();
    LibraryActor {
        widgets: ctx.widgets(),
        library: Rc::new(library),
    }
});

The reuse actor example would look about the same (though I would rather call the widget generation method builder.widgets instead of builder.connect_widgets because it does not generate signals)

If we do use the BuilderUtilizerContext idea, it would look like this

factories.library.make(|ctx| {
    ctx.actor(|ctx| {
        ctx.connect_signals::<LibrarySignal>();
        let library = futures::executor::block_on(library::Library::load()).unwrap();
        LibraryActor {
            widgets: ctx.widgets(),
            library: Rc::new(library),
        }
    });
});

And actor reuse would look like this:

self.factories.row_addend.make(|ctx| {
    builder.connect_signals_tagged(addend_id, ctx);
    let widgets = builder.connect_widgets::<AddendWidgets>().unwrap();
    // Either use `widgets` here, or - since it does not depend on `ctx`, we can just return it and
    // have `make` return the value returned from the closure.
});

I'll need to check later if ActorBuilderContext can work. It highly depends on Rust being able to infer the closure's return type and use that inferred type to set the type of the closure's argument.

[piegamesde]

Some points before I start giving this a try:

• (nitpick) the actor reuse variant needs to more like this factories.library.make(ctx, |ctx| {…}
• What is the return type of the outer closure? Especially if I create multiple actors, how do I get their addresses out of the closure?
• What about the error handling we need when using the Widgets?

[piegamesde]

Without the error handling (for now), my code looks like this

builder.new_actor_2(|ctx| {
    ctx.connect_signals::<LibrarySignal>();
    let library = futures::executor::block_on(library::Library::load()).unwrap();
    LibraryActor {
        widgets: ctx.connect_widgets(),
        library: Rc::new(library),
    }
});

and I like it. The trick with Deref is absolutely great! The only problem is the error handling. If the method can never fail because we're not using the widgets, then the compiler won't be able to infer the error type. And annotating a Closure type is not fun.

I won't try doing the outer closure, because the registration in the Drop handler (or explicitly using the .finish method if you want to) works fine for me IMO. But if we can't get a satisfying solution for this, I've an alternative approach that I could try for registering the signals that doesn't need it:

The key problem is that builder.connect_signals can only be called once. I do at the end for obvious reasons. But we can also do it at the beginning: We put a dummy closure in a Rc<RefCell<_>> for each callback handler. Using interior mutability, we then modify the closures when the actors register themselves. This is less elegant because of interior mutability and an additional layer of indirection during runtime, but I think it worth mentioning in case the gained API usability is worth the tradeoff.

[idanarye]

How did you manage to not pass LibraryActor as neither type annotation or generic parameter? When I tried it, Rust would not accept it - https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c6bd4badcb45b84b09e1f245f6035b17.

The only problem is the error handling. If the method can never fail because we're not using the widgets, then the compiler won't be able to infer the error type. And annotating a Closure type is not fun.

How about we provide both actor and try_actor?

I won't try doing the outer closure, because the registration in the Drop handler (or explicitly using the .finish method if you want to) works fine for me IMO. But if we can't get a satisfying solution for this, I've an alternative approach that I could try for registering the signals that doesn't need it:

I think we can both agree there are no great solutions for this and we have to settle for a good enough solution. I can live with registering everything in the Drop, but we are going to have to warn about it in the README.

The key problem is that builder.connect_signals can only be called once. I do at the end for obvious reasons. But we can also do it at the beginning: We put a dummy closure in a Rc<RefCell<_>> for each callback handler. Using interior mutability, we then modify the closures when the actors register themselves. This is less elegant because of interior mutability and an additional layer of indirection during runtime, but I think it worth mentioning in case the gained API usability is worth the tradeoff.

I thought about this too and disqualified it for the same reason.

[piegamesde]

How did you manage to not pass LibraryActor as neither type annotation or generic parameter?

Honestly, I don't know. It just … worked? I've had a look at your gist and it has a lot of subtle differences in the type signatures to my code. I tried quite a few things, but none of them made the compile error go away :(

How about we provide both actor and try_actor?

Yes, and also widgets and try_widgets for completeness.

[idanarye]

I think this PR is starting to get big, and already has enough content, so let's start cutting some things out. We'll get them in in other PRs - maybe even for 0.2! - but this is a big change that needs to get in. Especially since all the other tickets you've opened are going to be solved in this new style, so neither of us can work on them before this refactor is completed.

• I think I should revert 79abc12336ce363ab2367b707d25ef35d69b909f. The conflicts with this PR are huge and it'll be easier if I just implement it after it gets merged, on top of this new style, and that's going to be different from the code of that commit in both API and implementation. Unless you already did that work and just haven't pushed yet?
• Let's leave try_actor out of this PR. It's not like WoAB supports it now, and supporting it is tricky because Actix' Actor::create does not support errors. It can be supported by using Context::with_receiver and Context::run, but lets leave that out of this PR.
  • Actually... if we do use Context::with_receiver we may be able to use fluent interface after all - which means we won't need try_actor. Then again, this may require more annotations so let's put that off for now.
• try_widgets is completely redundant. Unlike actor, which receives a closure that may or may not want to return an error, widgets can always fail - so there is very little value to having another method that panics instead of failing. If you want to panic just ctx.widgets().unwrap().
• This may be implicitly obvious, but to make it explicit - I retract my demand to have a TypedBuilderUtilizer with the old behavior. Having managed to converge on a compact syntax that's elegant enough for both your usecase and my usecase, we can just use the untyped BuilderUtilizer for everything. This also means that Factory no longer needs to have generic parameters.

This is an amazing refactor. Thank you for putting all this time into this project!

[idanarye]

STOP THE PRESS!!!

I think one of the core problems is that in order to use Actix' context when builder the actor we had to use Context::create, and the closure we would pass to it would have to already represent everything we want to do with this context. This means either registering everything to a builder or passing a closure with a context that can do everything. We came up with as neat a syntax as we can, considering this limitation - but we were still limited by it.

And then you mentioned error handling, and I said it's a problem because Context::create does not support it. But I dag deeper into how it's implemented and figured out you can do the same thing it does with:

let (_, rx) = actix::dev::channel::channel(16);
let ctx = Context::<MyActor>::with_receiver(rx);
ctx.run(MyActor)

And this can be used for error handling, because you don't have to call ctx.run(MyActor) if there is an error. But... this can also be used for using the builder syntax we both seem to prefer! Just create the context, put it inside the builder, and call run at the end.

Here is a PoC for this syntax: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8926f3e787bc0bc59c60ab858e8bc6db

I'll copy the main function here:

fn main() {
    let builder = BuilderUtilizer::default();

    builder
        .actor()
        .connect_signals(Signal1::connector())
        .connect_signals(Signal2::connector())
        .run(Actor1 {
            widgets: builder.widgets().unwrap(),
        });

    builder
        .actor()
        .connect_signals(Signal3::connector())
        .connect_signals(Signal4::connector())
        .run(Actor2 {
            widgets: builder.widgets().unwrap(),
        });

    BuilderUtilizer::default()
        .actor::<Actor3>()
        .connect_signals(Signal1::connector())
        .connect_signals(Signal4::connector())
        .create(|ctx| Actor3 {
            my_own_address: ctx.address(),
            widgets: ctx.widgets().unwrap(),
        });
}

(for connect_signals I'm using my idea from #8)

Notice that for Actor3 I've added a create method that gets a closure. I had to put the turbofish, but I hope that the same thing that made it redundant in your PoC will make it redundant here. I could add methods to the ActorBuilder for getting both the context and the widgets, but because ActorBuilder's methods are almost always move method (because the final method must consume the actor context) it would look like this:

let actor_builder = BuilderUtilizer::default().actor();
let actor_builder = actor_builder
    .connect_signals(Signal1::connector())
    .connect_signals(Signal4::connector());
let actor = Actor3 {
    my_own_address: actor_builder.ctx().address(),
    widgets: actor_builder.widgets().unwrap(),
};
actor_builder.run(actor)

Which is kind of ugly.

[piegamesde]

/me was about to push 😅

[idanarye]

Sorry for always changing the syntax... But this is a good brainstorm.

I don't think #8 should be implemented here - this is already too much. It's enough, for this PR, to only implement

builder
    .actor()
    .connect_signals::<Signal1>()
    .connect_signals_with_tags::<Signal2>(some_tag)

And I'll do #8 in a later PR. Or maybe even not implement connect_signals_with_tags and leave this usecase broken until I do #8.

[piegamesde]

So, consider the current state as a checkpoint—it has an API that is really usable. Please have a look at it, if there are mistakes or other things that should be changed.

Regarding your updated builder API proposal: what are the benefits and downsides compared to the current way of doing so? (Apart from potentially better error handling; but actually I'm fine with panicking there because I don't think there are many situations where one can recover from such an error.)

[idanarye]

So, consider the current state as a checkpoint—it has an API that is really usable. Please have a look at it, if there are mistakes or other things that should be changed.

Very well. I'll revert 79abc12336ce363ab2367b707d25ef35d69b909f and start reviewing this PR.

Regarding your updated builder API proposal: what are the benefits and downsides compared to the current way of doing so? (Apart from potentially better error handling; but actually I'm fine with panicking there because I don't think there are many situations where one can recover from such an error.)

Let's continue this discussion in #9.

[piegamesde]

Well, that was a bit too quick ^^

I made a small mistake that breaks everything. Here's the patch:

From 9079d86afd1dc5d0a069044ca7dae64713738ed1 Mon Sep 17 00:00:00 2001
From: piegames <git@piegames.de>
Date: Sat, 6 Feb 2021 13:08:16 +0100
Subject: [PATCH] Fix small bug

---
 src/builder.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/builder.rs b/src/builder.rs
index fe4eede5..f37a80c8 100644
--- a/src/builder.rs
+++ b/src/builder.rs
@@ -195,7 +195,7 @@ where
 /// See [`woab::Factory`](struct.Factory.html) for usage example.
 pub struct BuilderConnector {
     builder: gtk::Builder,
-    callbacks: HashMap<String, RawSignalCallback>,
+    callbacks: HashMap<&'static str, RawSignalCallback>,
 }

 impl From<gtk::Builder> for BuilderConnector {
@@ -224,7 +224,7 @@ impl BuilderConnector {
     {
         let (tx, rx) = mpsc::channel(16);
         for signal in S::list_signals() {
-            S::bridge_signal(signal, tx.clone());
+            self.callbacks.insert(signal, S::bridge_signal(signal, tx.clone()).unwrap());
         }
         A::add_stream(rx, ctx);
     }
@@ -249,7 +249,7 @@ impl BuilderConnector {
         let (tx, rx) = mpsc::channel(16);
         let rx = rx.map(move |s| (tag.clone(), s));
         for signal in S::list_signals() {
-            S::bridge_signal(signal, tx.clone());
+            self.callbacks.insert(signal, S::bridge_signal(signal, tx.clone()).unwrap());
         }
         use actix::AsyncContext;
         ctx.add_stream(rx);
-- 
2.29.2