client: keep ontop/fullscreen/above/below when setting the others

[blueyed]

Fixes #667.

This does not break any test.

It should have a new test though when accepted.

Order is handled internally here:

awesome/stack.c

Lines 125 to 156 in 57e05cd

	/** Get the real layer of a client according to its attribute (fullscreen, …)
	* \param c The client.
	* \return The real layer.
	*/
	static window_layer_t
	client_layer_translator(client_t *c)
	{
	/* first deal with user set attributes */
	if(c->ontop)
	return WINDOW_LAYER_ONTOP;
	/* Fullscreen windows only get their own layer when they have the focus */
	else if(c->fullscreen && globalconf.focus.client == c)
	return WINDOW_LAYER_FULLSCREEN;
	else if(c->above)
	return WINDOW_LAYER_ABOVE;
	else if(c->below)
	return WINDOW_LAYER_BELOW;
	/* check for transient attr */
	else if(c->transient_for)
	return WINDOW_LAYER_IGNORE;
	/* then deal with windows type */
	switch(c->type)
	{
	case WINDOW_TYPE_DESKTOP:
	return WINDOW_LAYER_DESKTOP;
	default:
	break;
	}
	return WINDOW_LAYER_NORMAL;
	}

Note that e.g. client.get.visible allows for getting the clients by stacking order already.

[codecov]

Codecov Report

Merging #2512 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2512      +/-   ##
==========================================
- Coverage   84.32%   84.32%   -0.01%     
==========================================
  Files         479      479              
  Lines       33277    33261      -16     
==========================================
- Hits        28062    28048      -14     
+ Misses       5215     5213       -2

Flag	Coverage Δ
#c_code	72.98% <ø> (-0.04%)	⬇️
#functionaltests	72.86% <ø> (ø)	⬆️
#lua53	87.39% <ø> (ø)	⬆️
#samples	71.22% <ø> (ø)	⬆️
#unittests	58.17% <ø> (ø)	⬆️

Impacted Files	Coverage Δ
objects/client.c	79.18% <ø> (-0.11%)	⬇️

[Elv13]

Ref: #2047 (comment)

[Elv13]

Shall we merge this or fix some deeper issues and get this as a side effect?

[blueyed]

Let's wait for @psychon to approve/review it.

I think it is a good start, and awesome itself would still behave the same.

Note that hacks similar to #667 (comment) might break due to this, e.g. if you expect "fullscreen" to be unset when "ontop" gets set etc, but I think that's fine / easy to fix then.

[psychon]

Fine by me, if you want to stay around and fix the resulting problems.

@Elv13 Is this allowed in a minor release or is this possibly-breaking enough to need to wait for v5?

[psychon]

Oh and what will be your answer to the incoming bug reports saying stuff like "I tried to set a client below, but nothing happened"?

[blueyed]

I tried to set a client below, but nothing happened

Good point.
Maybe this should only take out "fullscreen" then for now? (i.e. fullscreen is kept when setting the others and does not reset the others)
All the others define the stacking only, while "fullscreen" can be more seen like "maximized" in this sense.

[Elv13]

@Elv13 Is this allowed in a minor release or is this possibly-breaking enough to need to wait for v5?

I think the current API can be implemented on top of the new one but deprecated. I can take care of that part if you submit such API changes. Adding convoluted maintenance heavy code to support our legacy features is something I have done a lot ;) (just see that ugly c:keys() hack in the other PR).

[actionless]

i would also add a test to be sure what the next "deeper fix" will not break it again

[blueyed]

@actionless
What do you think about #2512 (comment)?

@Elv13

I think the current API can be implemented on top of the new one but deprecated.

Do you mean to keep the current behavior on the Lua side, based on this PR / removal / internal change?

[actionless]

@blueyed yes, that sounds more logical to me

[actionless]

@Elv13 was the current behavior documented somewhere to deserve to be preserved? for me it looks more like a bug

[Elv13]

@actionless looking back at the last time I modified the maximize behavior because people reported "bugs" about it, I can say that changing these things quickly becomes a rabbit hole. People will notice and wont be very happy. Right now the design of this feature is "broken" in the sense people can't make it behave like they want. The only reason few notice is because the tiled client tweak their indices in the client list to avoid the bug(s) from being visible. I think it should be fixed properly because "random" changes in behavior will just cause more open issues with more user requests and more changes to accommodate the user whose favorite behavior broke, then more bugs due to the added complexity.

[actionless]

@Elv13 that's the reason why i insisting on having the test for it -- so it will turn it from undefined state to somehow defined (if not in docs then in tests, which is even better in maintenance point of view)

[Elv13]

@actionless as long as we wait for after 4.3 is released I am ok with trying. However all comments near the end of #2047 still stand. Doing this a few days before releasing 4.3 is too risky.

[actionless]

i know about z-index thing, i think we were discussing that even more times longer ago

however speaking of this particular issue (especially not the actual diff but amended version proposed by @blueyed in the comment above):
i can't imagine how reseting ontop state after making client fullscreen could be an expected behavior

however i agree what resetting below state on fullscreen could be expected by some people