Add a --wallpaper argument to optionally disable wallpaper draws

mattkae commented 1 year ago

Depends on

https://github.com/MirServer/mir/pull/3086

To test

Checkout https://github.com/MirServer/mir/pull/3086 and run sudo make install
Checkout https://github.com/MirServer/ubuntu-frame/pull/157 and build it

From a VT, run plymouth

sudo plymouthd
sudo plymouth show-splash
sudo plymouth deactivate

The splash should still be on the screen

Run frame:
```
./frame --wallpaper=false
```
Wait however long you like and note that you still see the splash
Run some other app (e.g. gedit) and note that the splash disappears

What's new?

Added a wallpaper_enabled flag that is set to true by default
Only doing things in BackgroundClient::Self::draw_screen if we have errors or the wallpaper is enabled
If we have an error and we don't want the wallpaper but we don't have a diagnostic file, then we draw the wallpaper as we did before (this may be undesirable, but its a very rare case, and it maintains previous behavior)

AlanGriffiths commented 1 year ago

@mattkae were you able to confirm this DTRT as far as smooth boot is concerned?

I don't think this can be to address seamless boot directly: that needs the server to defer compositing until content from the client app is available. This just means that compositing will start without a background (as you say, just like mir_demo_server, or miriway without swaybg).

The idea might be to simplify detecting whether there's an app providing content by preventing the "wallpaper client" from providing content. But I'm not convinced that's necessary, or a good idea.

mattkae commented 1 year ago

@Saviq @AlanGriffiths Alan is right on this one. The compositor itself is going to clear the screen once it is ready to start drawing surfaces. As Alan said, we could make it not do anything until it first has a "renderable" to render. Otherwise, we could do the tougher task of reading in the plymouth buffer and rendering that ourselves until the user has supplied another renderable.

Saviq commented 1 year ago

I don't think this can be to address seamless boot directly: that needs the server to defer compositing until content from the client app is available. This just means that compositing will start without a background (as you say, just like mir_demo_server, or miriway without swaybg).

As Alan said, we could make it not do anything until it first has a "renderable" to render.

Yeah that's what I think we should be doing. Off the top I can't think of why it couldn't be the default behaviour?

The idea might be to simplify detecting whether there's an app providing content by preventing the "wallpaper client" from providing content. But I'm not convinced that's necessary, or a good idea.

Do you have another approach in mind? Why / how would we want to treat the background client in a special way? In truth, we could probably (when this, or similar, option is engaged) just delay the background client altogether by $timeout, no smaller than the diagnostic timeout?

Otherwise, we could do the tougher task of reading in the plymouth buffer and rendering that ourselves until the user has supplied another renderable.

That wouldn't change much compared to the above, would it?

I mean, not until we start doing something more with the buffer.

Saviq commented 1 year ago

OK so this, together with MirServer/mir#3086 looks exactly like what we need :)

I'm just trying to think about what's the best user experience. A --wallpaper-timeout came to mind, but then isn't the diagnostic what we'd like to see first, if configured? So the wallpaper timeout would have to be counted towards the diagnostic one.

But do we really need another timeout? The behaviour here, I think, is mostly correct - the first thing we see (with --wallpaper=false) is the diagnostic. We still see the wallpaper if there is no diagnostic text, so that's the icky bit - alternative would be to stay on the splash screen indefinitely, but maybe that's what we should do?

If someone disabled the wallpaper, and does not have the diagnostic set up - maybe the splash screen should stay on screen?

mattkae commented 1 year ago

OK so this, together with MirServer/mir#3086 looks exactly like what we need :)

I'm just trying to think about what's the best user experience. A --wallpaper-timeout came to mind, but then isn't the diagnostic what we'd like to see first, if configured? So the wallpaper timeout would have to be counted towards the diagnostic one.

But do we really need another timeout? The behaviour here, I think, is mostly correct - the first thing we see (with --wallpaper=false) is the diagnostic. We still see the wallpaper if there is no diagnostic text, so that's the icky bit - alternative would be to stay on the splash screen indefinitely, but maybe that's what we should do?

If someone disabled the wallpaper, and does not have the diagnostic set up - maybe the splash screen should stay on screen?

Maybe if there isn't any diagnostic text but there is some error, then we show some default diagnostic text? I agree that it feels weird to show the background out of nowhere when there's an error. I'm up for either some default text or just keeping the splash on screen. Although, leaving the splash on screen makes it a bit trickier to debug from a user's perspective

Saviq commented 1 year ago

Maybe if there isn't any diagnostic text but there is some error, then we show some default diagnostic text? I agree that it feels weird to show the background out of nowhere when there's an error.

It's not that there is an error, necessarily. And even if there is - Frame can't know what it is.

I'm up for either some default text or just keeping the splash on screen. Although, leaving the splash on screen makes it a bit trickier to debug from a user's perspective

Well, that's exactly what the diagnostic is for. The "user" in the case of Frame shouldn't be doing any debugging, just call the number that's on screen :)

And just to confirm we're DTRT:

The before:

The before

And the after:

And the after

The white flash is rendered by Flutter, so it's in the application author's hands now.

Saviq commented 1 year ago

@mattkae so personally I'm erring on --wallpaper=false doing what it says on the tin… just not draw the wallpaper, ever. If there is a diagnostic on, that will pop up as usual.

@AlanGriffiths thoughts?

mattkae commented 1 year ago

Well, that's exactly what the diagnostic is for. The "user" in the case of Frame shouldn't be doing any debugging, just call the number that's on screen :)

That makes sense to me! So if they fail to set up the diagnostic and the wallpaper is turned off, then we keep showing the splash. Does that sound right?

And just to confirm we're DTRT:

And yup, that's what I would expect. The splash is shown all the way up to the first render of the app.

Saviq commented 1 year ago

That makes sense to me! So if they fail to set up the diagnostic and the wallpaper is turned off, then we keep showing the splash. Does that sound right?

Yeah exactly.

And just to confirm we're DTRT:

And yup, that's what I would expect. The splash is shown all the way up to the first render of the app.

Oh I wasn't asking ;D

mattkae commented 1 year ago

@Saviq I have implemented + tested only showing the diagnostic screen when we have diagnostic info :+1:

Saviq commented 1 year ago

@mattkae that if/else dance looks like it could use some rework?

mattkae commented 1 year ago

@mattkae that if/else dance looks like it could use some rework?

As in its too verbose, or is it incorrect?

Saviq commented 1 year ago

As in its too verbose, or is it incorrect?

I mean this:

https://github.com/MirServer/ubuntu-frame/blob/9f247ce1b325f8b74fe3ddb7d400346a9e36232a/src/background_client.cpp#L401-L416

and then this:

https://github.com/MirServer/ubuntu-frame/blob/9f247ce1b325f8b74fe3ddb7d400346a9e36232a/src/background_client.cpp#L457-L467

The control flow got quite twisted?

canonical / ubuntu-frame