Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assign multiple Actors to a Builder #3

Closed
piegamesde opened this issue Jan 31, 2021 · 11 comments · Fixed by #4
Closed

Assign multiple Actors to a Builder #3

piegamesde opened this issue Jan 31, 2021 · 11 comments · Fixed by #4

Comments

@piegamesde
Copy link
Contributor

My UI is pretty static, as in it defines all in one file and there are not many changes to the hierarchy during the runtime of the program. Therefore, I don't really need factories, only one Builder instance is enough.

But I'd still like to split my design into multiple actors: for example, the full screen handling is a separate strand of logic that works independently of the rest and thus can be encapsulated as a separate actor.

I can create multiple BuilderUtilizer instances from one builder object and for the widgets it works fine. But for the signals it does not, only the first actor will get all the signals and all others won't.

@idanarye
Copy link
Owner

So my first instinct is to do something like this: https://gist.github.com/idanarye/25b24d72e30edf23cf3057c4b62b506c. Basically I'm putting a part of the GUI into a a container, and use woab::Factories to take the GUI part out of the XML. Then, in the code, I create both the main app and the sub-GUI and add the sub-GUI to the container that held it in the designer. The end result is the same as we see in Glade, but there are two builder invocations with two different XML files, which splits the signals each to its appropriate builder.

In that gist I'm using gtk::ListBox as the container, because all the other containers have <packing>. But I can probably make a more elegant solution by making the WoAB XML manipulation replace it with a simpler container (gtk::Box? Ideally I would use gtk::Bin but subclassing it is an overkill...)

Of course - this is assuming you are splitting the actors based on the GUI, where each actor is responsible for one subtree of the GUI and receives all the signals from that subtree (as long as they don't come from a subtree of that subtree that was taken by another actor). If this is not the case, then we may need a more elaborate scheme - maybe namespacing the signals?

@piegamesde
Copy link
Contributor Author

Of course - this is assuming you are splitting the actors based on the GUI, where each actor is responsible for one subtree of the GUI and receives all the signals from that subtree

At the moment, I am referring to widgets from within multiple actors. For example, the header bar is shown and hidden by the fullscreen actor, but other actors hold a reference to it as well in order to add buttons etc. I also have a HdyDeck with multiple cards. Each card has its own actor managing the content, but they all hold a reference to the deck to flip pages. Of course, one could always transform this into a "pure" tree hierarchy by factoring out widgets that are used multiple times and then let the actors communicate within each other.

If this is not the case, then we may need a more elaborate scheme - maybe namespacing the signals?

How does this work internally? Is there anything preventing me to have multiple actors listen on a signal if they use the same name? If I can have multiple actors share a widget, why shouldn't they share signals as well?

@idanarye
Copy link
Owner

idanarye commented Feb 1, 2021

BuilderUtilizer::stream_builder_signals() is calling BuilderSignal::stream_builder_signals(), which BuilderExtManual::connect_signals() - a GTK utility that allows allows registering signals from a builder by name. The macro generates a big match with an arm for each enum variant that translates the &[glib::Value] to a value of the signal type with proper variant and fields, and sends it via a Tokio stream. Finally stream_builder_signals() returns that stream, so that it could be connected to the StreamHandler.

I thought calling stream_builder_signals() multiple times should create multiple streams that all transmit the same signals. Did this not work for you?

@piegamesde
Copy link
Contributor Author

Thanks for the insights.

I thought calling stream_builder_signals() multiple times should create multiple streams that all transmit the same signals. Did this not work for you?

I expected this as well, but it doesn't. Apparently it isn't intended to be called multiple times. Fixing this would require the implementation to switch to using gtk_builder_add_callback_symbol[s]: we can do this, because we know all signals of our interest as these are precisely the enum variants. This has the advantage to only add a callback when it's specified, so this would only register twice if the user would do so.

I don't know about the behavior of calling gtk_builder_add_callback_symbol multiple times, but if it ignores all but one I'd be fine with simply documenting that fact. We can later optionally add some renaming and namespacing features if desired.

@piegamesde
Copy link
Contributor Author

So the semantic of those methods is: you can call them multiple times, only the first callback for each signal is registered.

@idanarye
Copy link
Owner

idanarye commented Feb 1, 2021

I'm not a big fan of using unsafe FFI outside the crate that's supposed to wrap it. I'd like to propose a different solution - using an actor to route the signals: https://gist.github.com/idanarye/de191a9b42fd5b5afbc0dd3ceef83b6e

I can start with a simple router that just clones the signal and transmits it to all registered recipients, and in future updates, if the need arises, I can add routers that route by namespace, or by tags, or by parsing the arguments, and it can all be composed to fit the projects' requirements.

@piegamesde
Copy link
Contributor Author

I'm not a big fan of using unsafe FFI outside the crate that's supposed to wrap it.

Oh, I didn't notice this. There are no bindings for this function. I've also had a closer look at it, and the way gtk_builder_add_callback_symbol is defined makes it pretty useless for our use case and offers only little benefit over the current situation.

Regarding your proposal, do I understand it correctly that all actors that potentially share signals also share a Message enum? So they all get all messages, even though they probably ignore most of them?

I have a vague idea in mind on how this could be done in a way that keeps them separate, but I'll have to think about that one some more.

@idanarye
Copy link
Owner

idanarye commented Feb 2, 2021

With the basic router - yet. But I can probably work a more advanced router. I envision something like that:

#[derive(Clone, woab::SignalNamespace)]
pub enum Namespace {
    #[namespace(root)]
    Root(RootSignal),
    Namespace1(Namespace1Signal),
    Namespace2(Namespace2Signal),
}

#[derive(Clone, woab::BuilderSignal)]
pub enum RootSignal {
    Signal1,
    Signal2,
}

#[derive(Clone, woab::BuilderSignal)]
pub enum Namespace1Signal {
    Signal3,
    Signal4,
}

#[derive(Clone, woab::BuilderSignal)]
pub enum Namespace2Signal {
    Signal5,
    Signal6,
}

This represents the signals:

  • Signal1
  • Signal2
  • Namespace1::Signal3
  • Namespace1::Signal4
  • Namespace2::Signal5
  • Namespace2::Signal6

And each actor can use its own namespace:

impl actix::StreamHandler<RootSignal> for Actor1 { ... }

impl actix::StreamHandler<Namespace1Signal> for Actor2 { ... }

impl actix::StreamHandler<Namespace2Signal> for Actor3 { ... }

Of course, nothing stops two actors from using the same namespace - they'll each get a copy of the signal - and nothing stops one actor from using two namespaces - it just has to be a StreamHandler of both types.

Routing registration can use the same message as the basic router, but we'd have to specify the specific signal type because the router supports multiple signals (though if the actor only has one signal I think Rust can infer that):

Actor1::create(|ctx| {
    let stream = woab::register_route<RootSignal>(router_ctx.address().recipient()).unwrap();
    Actor1::add_stream(stream, ctx);
    Actor1 { ... }
});

Actor2::create(|ctx| {
    let stream = woab::register_route<Namespace1Signal>(router_ctx.address().recipient()).unwrap();
    Actor2::add_stream(stream, ctx);
    Actor2 { ... }
});

Actor3::create(|ctx| {
    let stream = woab::register_route<Namespace2Signal>(router_ctx.address().recipient()).unwrap();
    Actor3::add_stream(stream, ctx);
    Actor3 { ... }
});

@piegamesde
Copy link
Contributor Author

I feel like I now know enough about the problem and the code base that I'd like to try to solve this myself (though it may take some time).

@idanarye
Copy link
Owner

idanarye commented Feb 2, 2021

When you say "solve" you mean "do a PR" or are you going to solve this externally, in your own code (e.g. with manual routing)? If you are going to solve this externally I may still want to add routing facilities to WoAB somewhere in the future.

Either way, I should probably finish #2 and make a release without waiting for this ticket, because while the multiple builders problem is not relevant for all architecture (it's not relevant for mine. It is relevant for yours. So far this is our entire sample...) and can be worked around externally, the signal return value prevents the usage of certain signals, which is a serious issue.

@piegamesde
Copy link
Contributor Author

When you say "solve" you mean "do a PR" or are you going to solve this externally, in your own code (e.g. with manual routing)?

I'm hoping to make a PR in the end, yes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants