Add keepBorder <theme> option and enable it by default

[Consolatis]

Fixes #813

Just a quick and dirty implementation with not that much testing.

TODO:

• Add comment about !view->ssd in ssd_thickness()
• Decide if we want to keep the keepBorder setting and if not
  • Remove the setting
  • Update docs
• SSD resets on unfullscreen

[johanmalm]

Thanks! Looks good.
I took it for a test-drive and found that on labwc --reconfigure all views go back to showing titlebars. Other than that, seems to work really well.

[Consolatis]

Another issue is that manually enabling SSDs on a CSD client correctly shows the SSD but when then disabling it again (with keepBorder enabled) it will keep the borders, e.g. once SSD is enabled you can't get rid of it anymore. I am not sure how that could be fixed as the intention to disable the SSD can mean different things.

[hype3]

Hi.

Any news how this goes?

[Consolatis]

The first issue (keep state on Reconfigure) should be relatively easy to fix (but requires saving some state outside of struct ssd).

The second issue however.. I am not sure.

• Maybe we could try to figure out if the view is using client side decorations and in that case completely disable the SSD - including the border - rather than just hiding the titlebar.
• Or we could have this being a 3-state action, e.g. SSD visible, ToggleDecorations with keepBorder set
  • 1st time: disable titlebar, e.g. only border visible
  • 2nd time: destroy the whole SSD
  • 3rd time: create the whole SSD

The second option (3-state) sounds not that hard to implement actually but I am not sure if that is the behavior a user would expect. In my eyes it seems like the most flexible option though as it allows the user to express all variants including having no decorations at all, even with keepBorder enabled. That is not the case with the first option or with the current state of this PR.

Edit:
I have implemented the 3-state option and personally I like how it behaves. Testing / comments about its behavior welcome. We could even remove the keepBorder setting completely and just always have ToggleDecorations use that 3-state variant. Thoughts @johanmalm?

Restore-on-Reconfigure still needs fixing.

[johanmalm]

Yes, I think this proposal makes a lot of sense. It would be pretty annoying if someone turned on SSD on a CSD client and then couldn't get rid of it 😄

I've had a play and it feels nice. It keeps it clean with the current client-menu too.

If - in the future - there is a use-case of a more direct set/unset of titlebar/ssd I guess we could add window-action like:

<action name="SetServerSideDecorationMode" mode="none|border|all"/>

I've just had a fresh look through the code. It's probably just my brain not keeping up, but...

src/ssd/ssd.c:36: why !view->ssd in if-statement

[Consolatis]

Yes, I think this proposal makes a lot of sense. It would be pretty annoying if someone turned on SSD on a CSD client and then couldn't get rid of it smile

I've had a play and it feels nice. It keeps it clean with the current client-menu too.

So completely remove the keepBorder setting and just always use the 3-state variant?

If - in the future - there is a use-case of a more direct set/unset of titlebar/ssd I guess we could add window-action like:

<action name="SetServerSideDecorationMode" mode="none|border|all"/>

Good idea. That would also be useful for the window rules. Or we add further options to the existing property instead.

I've just had a fresh look through the code. It's probably just my brain not keeping up, but...
src/ssd/ssd.c:36: why !view->ssd in if-statement

Just looked at that part and was confused as well. But it actually makes sense if what the comment on top says is true: that function has to return the correct dimensions even before ssd_create() has been called (view->ssd == NULL) but the view is configured to use a SSD (view->ssd_enabled). If the view doesn't want the SSD we return early on the top:

	/*
	 * Check preconditions for displaying SSD. Note that this
	 * needs to work even before ssd_create() has been called.
	 */
	if (!view->ssd_enabled || view->fullscreen) {
		return (struct border){ 0 };
	}

Thus when we are at the bottom of the function it doesn't matter if view->ssd has already been created or not and we always add the titlebar height (unless it has been disabled by only showing the border). I'll try to add a sensible comment on there so it makes sense on the first look rather than requiring a complete analyses of the function.

Edit:
Fixed border-only-on-reconfigure without adding a new struct view state.
Found a new issue though: unfullscreening resets the SSD as well and this likely won't work without having an external state.
But I'll wait with fixing that until it is clear if we want to keep the keepBorder setting or not.

[johanmalm]

Good stuff.

Yes, completely remove the keepBorder setting and just always use the 3-state variant. It’s a deviation from Openbox, but I think we need the flexibility.

[Consolatis]

I had some further thoughts on the keepBorder setting. There may be situations where people don't want the border only mode (I am using ToggleDecorations as part of a keybind together with Maximize for example). To allow keep using the old pattern of on / off I left the config setting in place but have it default to true.

So unless the user manually disables the setting the 3-state variant is used, with the config setting disabled the old 2-state variant. I think this is a good compromise about flexibility and respecting user wishes. But I would also be up to remove the option if @johanmalm prefers.

Also added docs and fixed exiting fullscreen. IMHO this PR is basically ready, would just need to squash the commits into one and write a proper commit message. A final test would also be appreciated.

[johanmalm]

Good compromise.
LGTM

[johanmalm]

Have tested. Works well.