Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update layout name on empty tags #974

Closed
wants to merge 1 commit into from
Closed

Conversation

thialfi17
Copy link

The layout name reported by the river-status-unstable-v1 protocol (used by waybar) is not being updated when switching to empty tags or when changing the layout of an empty tag. This is because the name is only updated in response to a LayoutDemand - which aren't being generated for empty tags.

I'm not sure what the etiquette of editing TODOs is but I'm not sure there's any simple optimisations left here since there's currently no way to know how root.applyPending() was called. If it was in response to a layout command then the LayoutDemand always needs generating in case it changed the layout name.

I tried thinking of other solutions, but honestly the amount of extra complexity they would need wouldn't be worth it since the potential penalty here only affects empty tags which people won't be on most of the time. If there were more optimisations already in place for handling the case where the count hadn't changed maybe it would be different, but if it was good enough before then it should still be good enough now.

The layout name reported by the river-status-unstable-v1 protocol was
not being updated when switching to empty tags or when changing the
layout of an empty tag. This was because the name is only updated in
response to a LayoutDemand - which weren't being generated for empty
tags.
@Leon-Plickat
Copy link
Member

Leon-Plickat commented Jan 28, 2024

Strictly speaking this is a protocol violation:

    <event name="layout_demand">
      <description summary="the compositor requires a layout">
        The compositor sends this event to inform the client that it requires a
        layout for a set of views.

Unless you allow the empty set.

@thialfi17
Copy link
Author

Okay so I missed that when checking the protocols for solutions. I think that it's ambiguous, but it definitely would imply to me >0 views. I've added my thoughts on some less than ideal alternatives below but would appreciate input from others.

Without allowing the empty set, the only way to update the layout name for a layout command on an empty tag would be to introduce a new event so that the Layout can push the new name to river. I can't think of any other solution for this case given the current capabilities of the protocols. This is the case I find the most jarring as a user: attempting to change the layout and not seeing the visual confirmation you expect.

To solve changing the layout name when switching tags would require storing the layout name per tag so the correct name could be retrieved when switching or adding a new method of querying the layout generator for the name (perhaps instead of the event mentioned above?). I'm not a fan of these alternatives since they add a lot of complexity for what is otherwise a trivial change and trivial case to handle in a layout generator (if they don't already handle it).

When I have some time I'll look at how other the other layout generators I know of might handle this and update with what I find. I do think what I've submitted is the most painless solution overall, but that doesn't mean it's the best choice.

@thialfi17
Copy link
Author

So I haven't run any of these or gone through them as carefully as I need to, but I can already tell that it will break a couple:

Layout Generator Breaks? Comments
riverguile
owm
kile
stacktile (v1)
stacktile (v2?)
rivercarro Yes At the start of layout generator has assert(view_count > 0) and some / by 0
river-luatile No
river-bsp-layout
river-dwindle
rivertile Yes At the start of layout generator has assert(view_count > 0) and some / by 0

The rest I think are "No"s but I need to check for / by 0s and actually run them.

@MaxVerevkin
Copy link
Contributor

I guess this can be done with a protocol version bump? "If version N or grater is bound, the set may be empty".

@Leon-Plickat
Copy link
Member

I don't really like the ambiguity either and think this should be specified clearly. I also wouldn't mind explicitly allowing the case of a layout demand for zero views if the protocol explicitly states that this is an optional mechanism for querying the layout name.

I don't know how to treat this ambiguity w.r.t. to backwards compatibility. I guess the safest option is to say the current version does not allow layout demands for zero views and then bump the interface version.

@ifreund thoughts?

I think my default response to this to accept the status quo and wait for more window management logic to be moved to layout generators, which would eliminate this problem altogether.

@ifreund
Copy link
Member

ifreund commented Jan 31, 2024

Yeah, I pretty much agree with @Leon-Plickat. If we do this, a protocol version bump as @MaxVerevkin suggests is the right way to do it without breaking existing clients.

It seems that at least one user cares enough about this to have gone to the trouble of opening this PR and checking various layout generators in the wild for compatibility. Therefore I'd be happy merging a patch bumping the river-layout minor version to 3 and explicitly documenting this behavior in the protocol.

@ifreund
Copy link
Member

ifreund commented Mar 22, 2024

Closing as this PR hasn't seen activity in a month and we have an issue to track this feature. Feel free to open a new PR taking the approach I've outlined.

@ifreund ifreund closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants