snowcap: Make programs composable

[Ottatop]

This PR changes Program to enable composition within other Programs.

This is pretty much just folding Glacier's Widget and TryWithEmitter traits into the Program trait (with Lua doing similar), because there's no way to easily embed something stateful into a Program without just recreating Glacier. It also introduces signals.

There are some differences.

• I renamed Emitter to Signaler
• Instead of a toplevel surface handling signals, the new_widget functions connect to RedrawNeeded and Message signals
• The Rust API introduces a UniversalMsg as a general catch-all message
  • Messages that impl Universal also impl From<T> and Into<Option<T>> to unify messages within a program tree

@Ph4ntomas I stole a bunch of stuff from Glacier so I'm going to need your permission to re-license to MPL. Also any input on this PR is helpful.

[Ph4ntomas]

@Ph4ntomas I stole a bunch of stuff from Glacier so I'm going to need your permission to re-license to MPL.

You have my permission, and for any part of Glacier you might find interesting. I'll add something in Glacier repo to make it clear that I'm fine with pinnacle re-licensing code under MPL.

Also any input on this PR is helpful.

I'll track it and chime in on stuff.

Let me know if you want a review at some point.

[Ph4ntomas]

Just some thought about that part:

Instead of a toplevel surface handling signals, the new_widget functions connect to RedrawNeeded and Message signals.

In Glacier's, the Bar (and Menu) connect to the Widgets Emitter, and forward the messages via the handle (that is captured when the toplevel is created). I initially captured the handle because being downstream, I could not change the way the Program work, but I think this is better.
However, the toplevel will still have to forward the messages (via signals instead of the surface handle directly).

Glacier Widgets' update function originally only received the message, and was expected to emit a Signal if needed (which is one of the reason Operation are sent via signals instead of using the handle in e.g. Glacier's Prompt widget (here).

With the addition of Popup I couldn't find a clean way to handle this and ended up sending the handle along with the message when calling the update function (wrapped inside a Parent). Opening the popup from the toplevel meant sending the whole Program via Signals (meaning the Program would be required to be Clone), and it added the need to forward the handle back to the Widget (so Widget can close their popup and not another Widget's popup).

I'm still unsure what would be the best design here. I don't think Program should have full, unrestricted access to their surface handle, but I do think update should have some way to interact with the handle. Here are all the current use-case in Glacier:

• setting/unsetting keyboard interractivity (toplevel only).
• sending operation (toplevel only, a signal could do the trick).
• opening Popup (all widget, done in glacier by passing a Parent handle).

Since popup are still un-merged, it's not really critical right now, but I think this should be solved as soon as possible, and I don't know if Popup's will be the only use-case (in which case only having a Parent might be limiting).

[Ph4ntomas]

@Ph4ntomas I stole a bunch of stuff from Glacier so I'm going to need your permission to re-license to MPL.

I've added an explicit license exception to Glacier (https://git.sr.ht/~phantomas/glacier#license-exception), that clarify that it's OK for Pinnacle & Snowcap to re-use the code.

[Ottatop]

I'm still unsure what would be the best design here. I don't think Program should have full, unrestricted access to their surface handle, but I do think update should have some way to interact with the handle. Here are all the current use-case in Glacier:

• setting/unsetting keyboard interractivity (toplevel only).
• sending operation (toplevel only, a signal could do the trick).
• opening Popup (all widget, done in glacier by passing a Parent handle).

Since popup are still un-merged, it's not really critical right now, but I think this should be solved as soon as possible, and I don't know if Popup's will be the only use-case (in which case only having a Parent might be limiting).

I suppose we could do something like

pub trait Program {
    // ...
    fn created(&mut self, handle: ShellHandle);
    // ...
}

pub enum ShellHandle {
    Layer(OpaqueLayerHandle),
    Popup(OpaquePopupHandle),
    Decoration(OpaqueDecorationHandle),
}

where the opaque handles only allow certain operations. Then when we call new_widget we can call program.created() with a handle and the program can store it and pass it along to child programs. Though that is another thing that has to be manually propagated.

[Ottatop]

Ok for the most part I think this is good.

Regarding passing through the surface handle, I feel like providing a nerfed version of the handle just makes it more inflexible. Program now has the created method to allow passing down a SurfaceHandle for use by the program and its children. Though there are now two ways to send messages to the program, not sure how I feel about that.

I've also added a Source trait. My idea is that it can register some external source to a signaler to provide messages. A program would hold a source, register it to its signaler, and pass messages from the source back to it so the source can update. Then the program can use whatever state the source provides to build a widget tree. I think this would be a better way to handle things like the tasklist in #415, and it also allows users to create custom sources without having to make changes to Snowcap itself.

[Ph4ntomas]

I'm not really sure I understand how Source are meant to work.

Is the register function meant to clone the Signaler for later use ?
If so, I'm not a huge fan since that introduces a way to leak the Signaler state if misused, especially since there's no current way to know a Widget is being discarded or signals are being deleted.

The update function is also a bit weird IMO. I would expect a message Source to be stateless from the Program perspective (but that's more a case of "I'm not sure I understand the use-case").

From the comment on Program::update, the source are meant to be held by the Program, but there's nothing enforcing that, which increase the risk of leaking the Signaler.

I could kinda see the point for #415 to have a Source maintain the tasks list state, and have every widget connect to that source. The widgets would then receive the initial state upon registration, then will only get updates when the state changes (meaning you would have only one channel that speak to the server instead of one per widgets).

But that would mean reversing the logic, so:

/// A source of data that can be used in widget programs.
pub trait Source {
    /// The type of signal that this source emit
    type Signal;
    
    /// Maybe have a type for the initial state

    /// Returns the initial state as a vec of `Signal`, and the `Signaler` to get future updates
    fn register(&mut self) -> (&Signaler, Vec<Self::Signal>);
}

or

/// A source of data that can be used in widget programs.
pub trait Source {
    /// The type of signal that this source emit.
    type Signal;

    // Maybe add a type for the initial state

    /// Connect the program to this signaler signal.
    fn connect<P, F, R>(&self, program: &mut P, initial_state_processor: F, update_processor: R)
        where
            P -> Program,
            F -> FnOnce(&mut P, Vec<Signal>)
            R -> Fn(Signal)
    ;
    
    // Maybe add a function for stateless connection without the initial_state_processor, or split the trait into stateless and stateful Sources
}

[Ph4ntomas]

OTOH, if the source are meant to be fully contained inside Program, I don't think there's a need for a trait. The Program will already need to store the object implementing Source, so there's no reason to limit ourself to the Source since the full type is known.

It would be useful for the task list to be able to make use of a generic Source<Signal: TaskChanges> that could either get its data from wlr-foreign-toplevel-management or xdg-foreign-toplevel-list, but I don't really see how the current impl would make it easier than having a TaskSource trait with a more specialized API

[Ottatop]

Is the register function meant to clone the Signaler for later use ?

Clone or otherwise move, yes. As an example:

struct WindowTitle {
    pub name: String,
}

impl Source for WindowTitle {
    type Message = String;

    fn register(&mut self, signaler: Signaler) {
        pinnacle_api::window::connect_signal(WindowSignal::TitleChanged(Box::new(move |_win, title| {
            signaler.emit(Message(title.to_string()));
        })));
    }

    fn update(&mut self, msg: String) {
        self.name = msg;
    }
}

Then some other Program creates an instance of WindowName, calls source.register() with its signaler, and stores it. Then when a window's title changes and a message gets emitted, the program calls source.update() with the message. When update on the program is called it can use the title from the source.

If so, I'm not a huge fan since that introduces a way to leak the Signaler state if misused, especially since there's no current way to know a Widget is being discarded or signals are being deleted.

That's... definitely a problem lol. Perhaps it would be a good idea to make all Signalers apart from the one in the WidgetBase weak.

The update function is also a bit weird IMO. I would expect a message Source to be stateless from the Program perspective (but that's more a case of "I'm not sure I understand the use-case").

I guess "source" isn't a great name for it. It's supposed to be stateful (as seen above).

From the comment on Program::update, the source are meant to be held by the Program, but there's nothing enforcing that, which increase the risk of leaking the Signaler.

I'm not sure how we would enforce that tbh. Leaking the signaler is much less of an issue if we make it weak as per above.

OTOH, if the source are meant to be fully contained inside Program, I don't think there's a need for a trait.

It would be useful for the task list to be able to make use of a generic Source<Signal: TaskChanges> that could either get its data from wlr-foreign-toplevel-management or xdg-foreign-toplevel-list, but I don't really see how the current impl would make it easier than having a TaskSource trait with a more specialized API

Yea I suppose ditching this Source trait for more specialized traits is desirable seeing that this one doesn't really provide any extra functionality.

[Ph4ntomas]

That's... definitely a problem lol. Perhaps it would be a good idea to make all Signalers apart from the one in the WidgetBase weak.

I don't think it's required (you'd need to promote it to a Signaler to use it anyway, so it's kinda a useless step), but the API should definitely not encourage passing it around or storing it. Maybe it would be better to just return it by reference. That way it's obvious from reading the API that you're not supposed to hold onto it, and the internals can clone it when/if needed to make the borrow-checker happy.

Yea I suppose ditching this Source trait for more specialized traits is desirable seeing that this one doesn't really provide any extra functionality.

There might be a case to be made to reduce the amount of open stream with the server and reduces the amount of memory consumed using a stateful object to hold shared state and notify of said state changes. I'm not sure we currently need that, and that would be kinda the opposite to what this trait does anyway.

[Ottatop]

I don't think it's required (you'd need to promote it to a Signaler to use it anyway, so it's kinda a useless step)

Yea I suppose so

Maybe it would be better to just pass it by reference.

I mean it's going to need to call emit after the fact which will require storage anyway, passing a reference just introduces one more forced clone unless it stores the reference which would suck in terms of lifetimes.

For now let's leave everything as is, we can assume that the source is responsible for cleanup of uses of the signaler on drop or something.

There might be a case to be made to reduce the amount of open stream with the server and reduces the amount of memory consumed using a stateful object to hold shared state and notify of said state changes. I'm not sure we currently need that, and that would be kinda the opposite to what this trait does anyway.

Yea this doesn't seem like a huge issue. I'll remove the trait, we can figure something out later.

[Ph4ntomas]

Not sure where to put it in the review, but IMO having a set of signal to notify the lifetime is useful. I've only used that for popup/menu, but I wonder if that should be part of the "standard" signal set. (But they can be added down the line, too).

[Ph4ntomas]

Overall, other than the Source trait where I have a few doubt/question, LGTM :)

[Ottatop]

Not sure where to put it in the review, but IMO having a set of signal to notify the lifetime is useful. I've only used that for popup/menu, but I wonder if that should be part of the "standard" signal set. (But they can be added down the line, too).

Should be easy enough to add a Closed signal.

[Ottatop]

Ok, Source trait has been removed and the closed signal has been added, that should be everything

[Ph4ntomas]

Alright, other than this, LGTM :)