[Feature Request] Allow setting custom startup/shutdown events in brew procs

[PhilippMDoerner]

Feature Request

Essentially what I'd love to be able to do would be to have parameters in the brew proc that would allow me to set my own procs to fire when the application starts up (and maybe also when it shuts down).

My explicit usecase would be that I'm currently working on my appster package as you have seen, which sets up a second thread as a "server". I'd love to be able to add a global timeout proc on startup of the application that listens for messages from said server.

Essentially I want to run this piece of code:

proc addServerListener*[OwlkettleApp: Viewable, SMsg, CMsg](app: OwlkettleApp, data: ServerData[SMsg, CMsg], sleepMs: int = 5) =  
  ## Adds a callback function to the GTK app that checks every 5 ms whether the 
  ## server sent a new message. Triggers a UI update if that is the case.
  proc listener(): bool =
    let msg = data.hub.readServerMsg()
    if msg.isSome():
      app.msg = msg.get()
      discard app.redraw()

    const KEEP_LISTENER_ACTIVE = true
    return KEEP_LISTENER_ACTIVE

  discard addGlobalTimeout(sleepMs, listener)

Right now I'm forced to tell the user "Well you have to know that you must run this in the beforeBuild hook of the App-Widget. It would be far neater (in my opinion) if I could forego that and just tell them "Add this event to your owlkettle-Startup-Events in the brew proc".

These could also be used for various other things along the lines of setting up resources (e.g. loggers) and destroying them on program shutdown.

Implementation

Implementation would be pretty trivial. Just add the types:

type StartUpEvent = proc(app: GApplication, data: ptr AppConfig)
type ShutDownEvent = StartUpEvent

And then add fields with seq[StartUpEvent] and seq[ShutDownEvent] in the AppConfig datatype. Finally add parameters for those fields (default value @[]) to all the brew proc and then (depending on the brew proc) execute the events in an appropriate place depending on the brew proc.

[can-lehmann]

I agree, this would be a good feature to have. Do the callbacks really need the GApplication parameter? Generally owlkettle does not expose GTK directly to the user. When exactly are the callbacks executed? Maybe at the end of setupApp? Then you could also pass in the toplevel app state as a WidgetState. I would pass AppConfig as a value type, not a ptr.

[can-lehmann]

Regarding ShutDownEvent: When exactly is it executed? Does it also executed when the application throws a Defect? What about a Segfault?

[can-lehmann]

Feel free to open a PR for this btw.

[PhilippMDoerner]

Happy to do so! GApplication is not necessary, I was just starting out of a perspective of making all data possible available. Can be cut without issues, as the important bit is access to the Viewable Widget instance that is the entire application representation by owlkettle for access to "redraw" and its state (as startup/shutdown events may want to access it).

As for when to run these, honestly my first trail of though was to implicitly register these with the "startup" and "shutdown" GTK signals that GApplication has access to (reference ). We can of course also do this more directly without jumping through hoops, in terms of "correctness" that just seems to be the state (or an equivalent of it) to go for.

[PhilippMDoerner]

I see now though that we don't consistently have access to GApplication so trying to go for an approach that consistently uses it is not that great of an idea. I'm... somewhat torn on when to execute these now. I think I can consistently get access to WidgetState and AppConfig, but only in a way that means adding "runStartupEvents" to each brew proc in custom places as they appear to have their own logic going on.

Shutdownevents may even have to live without WidgetState (and thus get their own proc signatures) because I don't think I can access WidgetState as that only exists within the activate callback in one of the owlkettle.brew procs

[PhilippMDoerner]

I have first working examples, but particularly ShutDown is really blocking me. I mean, I can add them without access to WidgetState, that just seems like a shame given that you might want to do something with that state when the program closes (e.g. store it to disk).

But I can't really get access to it in the brew procs that instantiate it in a callback. I could move that maybe out of there, but then I'd need to change AppConfig to include WidgetState which I'm not sure is desired (?). Like I'm not aware of the ramifications on why the stuff in activateCallback wasn't done in the brew proc itself and why one can call "runMainloop" and the other doesn't have to.

Could you give me maybe a short rundown on why these 2 brew procs look so different (I mean, the ones in adwaita mimic the ones in owlkettle.nim from what I can see)?

[can-lehmann]

The reason why the brew procs look so different, is that there are always two versions of each brew procedure: There is one that uses Gtk.Application and one that does not. When using Gtk.Application, you need an application ID which is passed as the first parameter to brew. Gtk.Application manages the mainloop for you, that is why owlkettle does not call runMainloop in this case. For the brew procs that do not use Gtk.Application, the mainloop has to managed manually by calling runMainloop. Internally runMainloop just handles incoming events as long as a window is open.

For the shutdown events, it might be fine to call them in the shutdown signal for apps that use Gtk.Application and immediately after "runMainloop" for applications that don't.

I would prefer adding a new closure object AppContext instead of adding a state field to AppConfig. E.g.

proc brew*(id: string,
           widget: Widget,
           icons: openArray[string] = [],
           darkTheme: bool = false,
           stylesheets: openArray[Stylesheet] = []) =
  type AppContext = object
    config: AppConfig
    state: WidgetState
  ...

  var context = AppContext(...)
  ...

  discard g_signal_connect(app, "activate", activateCallback, context.addr)
  ...

[PhilippMDoerner]

Hmm we have 2 ways to do what seems to me the same thing.

I understand the specific reason for the version using GApplication, it allows sending notifications more generally to the OS. Why would I as a user want to use the non-GApplication approach?

[can-lehmann]

I guess the main reason is not wanting to decide on an application id :smile:

[PhilippMDoerner]

I mean we're still in the pre Owlkettle 3.0 zone (afaik, you haven't declared a Feature Freeze to me :smile: ), could this be an opportune moment for a refactor? We could remove the non-id one and just set a default id-value for the id-parameter on the one using GApplication.

This is somewhat of a breaking change but also kinda isn't because strictly speaking we could just provide a default-id, it's just that instead of having access to 2 different brew procs the user would now be calling the same brew proc every time but sometimes override the default value. Would come with the free benefit that they'd always have the possibility to send Notifications without any brew-call changes.

Edit: I just tried out the notification example, for me on Arch it appears to not be working. Nothing crashes, calling "sendNotification" or rather g_application_send_notification just doesn't do anything. I'll investigate.

[can-lehmann]

To be honest I am not exactly sure what else using Gtk.Application affects. We would have to do some research to ensure that using a default ID does not break anything. E.g. if we provide a default id, and a user runs two owlkettle apps with this default id at the same time, does that confuse any other parts of the OS?

Re: Notification example, you need to add a .desktop file.

[PhilippMDoerner]

Re: Notification example, you need to add a .desktop file. Damn the little things >_<

Yeah that worked. I did just test it out, you can basically replicate an issue immediately: Withdrawing a notification from App A will also withdraw a notification that was sent by App B. Interacting with notifications in general (sending from A and withdrawing from B) while 4 windows of the same app listen to the same ID even appears to lead to crashes, at least on my system (made a desktop file for the notification example, pinned it to the taskbar and launched a new window from there 4 times).

For launching in general I couldn't replicate any issues.

[PhilippMDoerner]

In that regard we could also contemplate just setting a random string there. If the user wants to send notifications, they have to properly set the id anyhow. Should be easy enough to generate something very unique with urandom over... i dunno, 120 bytes.

In this scenario we'd likely still have 2 brew procs but it'd be one main one while the second generates a random string for an id and calls the first one with that.

[can-lehmann]

I think it is probably easier to keep the current behavior. For this specific issue, the additional effort for supporting both types of brew procs is pretty low (1LOC per proc).