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

Sway window newstyles #1419

Merged
merged 1 commit into from
Jan 13, 2023
Merged

Conversation

RobertMueller2
Copy link
Contributor

@RobertMueller2 RobertMueller2 commented Feb 2, 2022

A more sophisticated implementation for issue 1399, following the suggestions from #1399 (comment)

  • Might break existing styles, but only if people relied on floating nodes being counted (introduced in commit 9972384 -- so basically kinda breaks again what we fixed in that recent commit, but by using the new floating style, there should be a suitable workaround.)
  • Provides classes empty, floating, tabbed, tiled, solo, stacked, and app_id. Some trace log output.
  • Adds an "offscreen-css" bool option (default false), only effective when "all-outputs" is set. This provides styling according to windows on the given output when it's not focused, rather than showing the focused screens focused window and style.
  • Adds an "offscreen-css-text" string option (default empty), only effective when "all-ouputs" and "offscreen-style" are set. This is shown as a text on the unfocused output.
  • Adds a "show-focused-workspace" bool option (default false) to indicate the workspace name if the whole workspace is focused if nodes are present. If not set, empty text is shown but css classes according to nodes in the workspace are still applied.

Limitations:

  • when the top-level layout changes (e.g. tabbed to stacked), there is no sway event (well, there might be a keybinding event, but that is too weak) therefore the implementation cannot react here. The only way around I could think of would be to poll get_tree on a timer. Asking that sway fire an event for this is unlikely, considering that i3 doesn't do it either. I think I tested that. If I remember correctly.
  • I think the recursion could still be improved, getFocusedNode already iterates all leaves, and then leafNodesInWorkspace does it again for at least one workspace per output. It did not come across as a problem during testing, but I think the code could be more concise. On the other hand, the given change is already big enough as it is, but I have as much as I think was possible tried to stay within the existing implementation. I wouldn't exactly feel comfortable with almost rewriting the whole thing. ;)
  • while we're at it, I think the active workspace as well as the focused window could be passed as Json::Value instead of string content, then the update function can handle how the info is displayed, this could allow for different replacements for format, like marks, idle_inhibition status...

Review questions:

  • I'm not sure if the case for immediateParent is good. It looks like parentWorkspace (which existed before), but I'm unsure if that's correct according to the style guideline.

Still to do:

  • FIXME: app_id should only be set when it's the only window. manpage says so, so keep that behaviour.
  • FIXME: there's still a problem with floating windows
  • Optimise code here and there
  • make output/workspace filtering optional EDIT: Don't think this makes sense.
  • use clang-format on each of the commits to allow rebase again
  • consider "offscreen" style vs offscreen window situation, based on option
  • revisit code style
  • perhaps add "floating"/"tiled" styles depending on what the focused window is EDIT: leaving this for a future feature
  • Man page update
  • show-focused-workspace still isn't correct
  • More testing after rebasing

Tested with this style:

window#waybar {
  background-color: #990000;
}

window#waybar.empty {
  background-color: transparent;
}

window#waybar.empty #window {
  padding: 0px;
  margin: 0px;
  border: 0px;
/*  background-color: rgba(66,66,66,0.5); */ /* transparent */
  background-color: transparent;
}

window#waybar.solo #window {
    padding-left: 5px;
    padding-right: 5px;
    color: #eee8d5; /* base2 */
    background-color: #073642; /*base02*/
}

window#waybar.floating #window {
    padding-left: 5px;
    padding-right: 5px;
    color: #eee8d5; /* base2 */
    background-color: #b58900; /*yellow*/
}

window#waybar.tiled #window {
    padding-left: 5px;
    padding-right: 5px;
    color: #eee8d5; /* base2 */
    background-color: #cb4b16; /* orange */

}
window#waybar.stacked #window {
    padding-left: 5px;
    padding-right: 5px;
    color: #eee8d5; /* base2 */
    background: #2aa196; /*cyan*/
}
window#waybar.tabbed #window {
    padding-left: 5px;
    padding-right: 5px;
    color: #eee8d5; /* base2 */
    background: #859900; /*green*/
}

window#waybar.code {
    background-color: #007ACC;
}

and this config (testing all combinations of all-output/offscreen-style, offscreen-text set or not set):

{
    "layer": "top", // Waybar at top layer
    "position": "top", // Waybar position (top|bottom|left|right)
    "height": 39, // Waybar height (to be removed for auto height)
    "modules-center": ["sway/window", "custom/media"],
    "sway/window": {
        "format": "{}",
        "all-outputs" : true,
        "offscreen-css" : true,
        "offscreen-css-text": "unfocused",
        "show-focused-workspace-name" : true,
        "max-length": 50,
        "icon": true,
        "rewrite": {
            "(.*) - Mozilla Firefox": " $1",
            "fish (.*)": "> [$1]"
        },
}

@RobertMueller2
Copy link
Contributor Author

Something I noticed, when switching the workspace layout (e.g. tabbed to splith), there is no immediate class change. Only after focus change.

I'm not exactly sure how we can deal with that, because the only event I saw was for the keybinding.

@primalmotion
Copy link

This seems that can be a solution to my issue here too #1349
However I can't get that patch to work. I have added the classes described above but it does not seems to change anything

@RobertMueller2
Copy link
Contributor Author

RobertMueller2 commented Feb 8, 2022

I think your use case should even work before the patch, I'll add a comment in #1349.

Also I'm wondering if it helps for easier to understand styling if we also add classes to the label, not just the whole bar.

But I'm not sure why the patch doesn't do anything for you. Do you see any trace output from the module?

@primalmotion
Copy link

As stated in the #1349, my issue was that I did not use the sway-window module. it works when I enable it. sorry for the noise :)

@RobertMueller2
Copy link
Contributor Author

As stated in the #1349, my issue was that I did not use the sway-window module. it works when I enable it. sorry for the noise :)

no worries :)

@RobertMueller2
Copy link
Contributor Author

RobertMueller2 commented Feb 19, 2022

Moved left TODOs to first post.

@RobertMueller2
Copy link
Contributor Author

RobertMueller2 commented Feb 25, 2022

Just noticed that not all nodes necessarily have the output property set. That requires more changes.

@RobertMueller2
Copy link
Contributor Author

RobertMueller2 commented Feb 27, 2022

Sadly, the current approach fails with several tiled windows, at least when using the offscreen-text option. I was hoping not to change the underlying logic too much, but now I think it'll be more work. Could take a bit, too.

Edit: Actually, found a way to fix it. I hope anyway. :P

@RobertMueller2
Copy link
Contributor Author

I think I've done all I can here. I'll test the latest for a few more days and then remove the draft flag.

@RobertMueller2
Copy link
Contributor Author

RobertMueller2 commented Mar 24, 2022

While using this PR, I've noticed some behaviour which seems to be related to external screen connection, disconnection and reconnection. The display of the focused window gets "stuck" when changing screen focus, because there is some node that is being iterated where there's no Type child node and/or string comparison is not possible.

I'm not sure if I can reproduce it just like that, but I feel I need to investigate further what went wrong, which, unfortunately, delays the PR even more. (And if possible I'd like to find out if the main branch already has the same issue.)

Perhaps this is an issue in the get_tree, but even if it is, the module needs to be able to deal with it.

EDIT: With a combination of mobile try/catch blocks and gdb, I think I might have fixed it, need to observe some more.

@RobertMueller2
Copy link
Contributor Author

RobertMueller2 commented Apr 2, 2022

I've had a look at the failing FreeBSD check and I'm not sure why it fails. Looking at the logs, it seems to be a timeout while installing packages rather than actual building. I've setup a FreeBSD VM and I can compile and run the test suite without problems.

I guess it was temporary.

@RobertMueller2
Copy link
Contributor Author

RobertMueller2 commented May 28, 2022

Rebasing is adding more and more pain. :D I don't think I can guarantee the sanity of each individual commit post-rebasing any longer, which defeats the purpose of keeping them, so I decided to squash them.

I'd like to get this out of the way, but unfortunately, it's difficult at the moment to find enough time to properly test this.

@primalmotion
Copy link

I use this patch especially for the .floating and it works as expected for me

@primalmotion
Copy link

Oh noes. conflicts :(

@RobertMueller2 RobertMueller2 force-pushed the sway-window-newstyles branch 2 times, most recently from bbc3601 to 2091395 Compare August 28, 2022 11:46
@RobertMueller2
Copy link
Contributor Author

Oh noes. conflicts :(

rebased, at first glance still seems to work.

@primalmotion
Copy link

Thanks! Yeah everything I use still seems to work

@RobertMueller2 RobertMueller2 marked this pull request as ready for review November 28, 2022 17:44
@RobertMueller2
Copy link
Contributor Author

I've been using this for ages and have not encountered any issues, except one waybar crash in the context of replugging an external screen, where I was unable to replicate, and where I'm not even sure the reason was the module itself.

That said, I'm a bit swamped at this time. It might be a few weeks before I'm able to look into any issues, should any be reported.

Provides CSS classes empty, floating, tabbed, tiled, solo, stacked and
app_id.

Adds offscreen-css bool option (default false), only effective when
"all-outputs" is true. This adds styles on outputs without focused
node, according to its focused workspaces window situation.

Adds an "offscreen-css-text" string option (default empty), only
effective when "all-outputs" and "offscreen-style" are set. This
is shown as a text on outputs without a focused node.

Adds a "show-focused-workspace" bool option (default false) to indicate
the workspace name if the whole workspace is focused when nodes are
also present. If not set, empty text is shown, but css classes
according to nodes in the workspace are still applied.

Limitation:
When the top level layout changes, there is no sway event so the
module cannot react. Perhaps in the future recurring polling can
be added to go around this limitation.
@Alexays Alexays merged commit e59b4e4 into Alexays:master Jan 13, 2023
@Alexays
Copy link
Owner

Alexays commented Jan 13, 2023

Can you also update the github wiki?
Thanks for this update and your time :)

@RobertMueller2
Copy link
Contributor Author

Sure, done.

While doing so I've noticed a small error I've added to the manpage, will add a pull request in a min.

RobertMueller2 added a commit to RobertMueller2/Waybar that referenced this pull request Jun 11, 2023
Probably a rebase error during development of Alexays#1419. The code block now
removed was not supposed to be there anymore.
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.

3 participants