Updated sway-hide work to the latest master #1241
Conversation
Configuration option `layer` can now take a value "overlay", which draws the bar on top of the windows without creating an exclusive zone.
This allows auto-showing the bar when the modifier is pressed Closes Alexays#255 Setup instructions: - Set the `mode` of the bar to "hide" in sway configuration file - Set the `layer` of the bar to "overlay" in Waybar configuration file - Add "sway/hide" into `modules-left` in Waybar configuration file
Currently, it won't build. I am unfamiliar with C++, I don't know what's wrong.
This is also a rebase-fix
Call this when toggling visibility with SIGUSR1, and for `sway/hide` events.
This doesn't solve the issue, but it crashes less often...
Keep this as similar to "upstream" as possible.
This is a CSS-only change. It should be properly hidden.
|
Thanks for your work! |
src/bar.cpp
Outdated
| } else { | ||
| window.get_style_context()->remove_class("hidden"); | ||
| window.set_opacity(1); |
There was a problem hiding this comment.
You are fixing the symptom and also breaking existing features (show/hide by signal).
set_opacity should not fail on a correct visible Gtk window. The only cases of crash I can imagine without looking at the backtrace are when the window is not yet initialized or already destroyed.
Can you try checking window.get_realized() or window.get_mapped() right after setting the visible property?
There was a problem hiding this comment.
Sorry by introducing breaking changes, that was not my intention. I will reset it to the current master branch implementation.
There was a problem hiding this comment.
I reset the changes to the setVisible function. Calling get_realized or get_mapped did not make a difference.
The following error occurs when set_opacity is in the function and is used when the modifier key is pressed.
fish: “./build/waybar” terminated by signal SIGSEGV (Address boundary error)
In my configuration, I have a transition on the opacity property. Could it be possible that this conflicts to each other?
src/bar.cpp
Outdated
| wl_surface_commit(surface); | ||
| } | ||
|
|
||
| auto waybar::Bar::removeExclusiveZone() const -> void { |
There was a problem hiding this comment.
I'd suggest to do it like this:
void waybar::Bar::setExclusive(bool value) {
exclusive = value;
surface_impl_->setExclusiveZone(exclusive && visible);
}
src/bar.cpp
Outdated
| surface_impl_->setLayer(bar_layer::BOTTOM); | ||
| } else { | ||
| window.get_style_context()->remove_class("hidden"); | ||
| surface_impl_->setLayer(bar_layer::TOP); |
There was a problem hiding this comment.
This overrides the layer set in the config file.
You can implement that differently:
void waybar::Bar::setLayer(bar_layer layer) {
layer_ = layer;
surface_impl_->setLayer(layer_);
}and call the method from the same location where you are disabling exclusive zone.
src/bar.cpp
Outdated
| window.get_style_context()->remove_class("hidden"); | ||
| surface_impl_->setLayer(bar_layer::TOP); | ||
| } | ||
| wl_surface_commit(surface); |
There was a problem hiding this comment.
Extraneous wl_surface_commit is known to be able to crash Gtk in certain cases (as found and fixed in gtk-layer-shell). Also, surface can be unset you call that on an unmapped window.
That's why the code in master branch is calling surface_impl_->commit() which is noop for the most cases.
include/bar.hpp
Outdated
| auto setVisible(bool nvis) -> void; | ||
| auto toggle() -> void; |
There was a problem hiding this comment.
I know I'm nitpicking, but why was that change necessary?
There was a problem hiding this comment.
No that's totally justified. I took the wrong definition while resolving the merge conflicts. I will correct this.
Ok that sounds reasonable. My monitors are in energy saving mode, so this could be the case here.
I did not know how to change your suggestion for this case. So I added two lines to the constructor in this commit. I do not know if this update is correct?
I will run waybar with the trace option and hope for it to occur :)
@somini, @xPMo, this use case should be handled already from this commit on. The iteration through all bars has been replaced by the bar passed in the constructor. @somini Could you share your config for this use case? So it is easier for me to have a look at the behavior.
I will leave that command running during testing. |
|
Hm ok. So I just found another behavior that is close to No2. Resuming from swaylock (monitors are on, did not switch off while swaylock is active). When is the
[2021-09-20 13:14:26.582] [debug] Received SIGCHLD in signalThread
[2021-09-20 13:14:26.582] [debug] Cmd exited with code 0
[2021-09-20 13:14:26.637] [debug] Received SIGCHLD in signalThread
[2021-09-20 13:14:26.637] [debug] Cmd exited with code 0regularly. At some point, one of the bars does not show up anymore. I do not know if this is connected to each other. |
Waybar Config: {
"layer": "top",
"position": "top",
"name": "main",
"modules-left": [
"sway/hide",
"sway/workspaces",
"sway/mode"
],
"modules-center": [
"tray"
],
"modules-right": [
"idle_inhibitor",
"backlight",
"network#wifi",
"network#ethernet",
"pulseaudio",
"battery",
"clock#localtime"
],
"sway/workspaces": {
"disable-scroll-wraparound": true
},
"sway/mode": {},
"idle_inhibitor": {
"format": "{icon}",
"format-icons": {
"activated": "",
"deactivated": ""
}
},
"backlight": {
"format": "{percent}",
"on-click": "brightnessctl -q set 100%",
"on-scroll-up": "brightnessctl -q set 1%+",
"on-scroll-down": "brightnessctl -q set 1%-"
},
"network#wifi": {
"interface": "wifi",
"interval": 5,
"format-wifi": "",
"format-linked": "⚠",
"format-disconnected": "",
"tooltip-format": "{ifname}: {ipaddr}/{cidr}",
"tooltip-format-linked": "{ifname}: No IP",
"tooltip-format-disconnected": "{ifname}: OFF",
"format-alt": "{ifname}[{essid}]: {ipaddr}/{cidr}"
},
"network#ethernet": {
"interface": "ethernet",
"interval": 5,
"format-ethernet": "",
"format-linked": "⚠",
"format-disconnected": "",
"tooltip-format": "{ifname}: {ipaddr}/{cidr}",
"tooltip-format-linked": "{ifname}: No IP",
"tooltip-format-disconnected": "{ifname}: OFF",
"format-alt": "{ifname}: {ipaddr}/{cidr}"
},
"pulseaudio": {
"format": "{icon}{volume}%|{format_source}",
"format-muted": "|{format_source}",
"format-bluetooth": "{icon}{volume}%|{format_source}",
"format-bluetooth-muted": "|{format_source}",
"format-source": "{volume}%",
"format-source-muted": "",
"format-icons": {
"headphone": "",
"hands-free": "",
"headset": "",
"phone": "",
"portable": "",
"car": "",
"default": ["", "", ""]
},
"on-click": "pulseaudio-ctl mute toggle",
"on-click-right": "pavucontrol",
"on-clock-middle": "pulseaudio-ctl set 20"
},
"battery": {
"format": "{icon}",
"format-charging": "{capacity}%",
"format-plugged": "{capacity}%",
"format-alt": "{icon}{capacity}%",
"format-time": "{H}h{M}",
"format-icons": [
"",
"",
"",
"",
""
],
"tooltip": true
},
"tray": {
"spacing": 10
},
"clock#localtime": {
"format": "{:%d%b %H:%M:%S}",
"tooltip": true,
"tooltip-format": "<big>{:%Y %B}</big>\n<tt>{calendar}</tt>"
}
}Sway Config:
#!/bin/bash
bar_id=
while getopts ":b:" opt; do
case "$opt" in
b)
bar_id="$OPTARG"
;;
*)
echo "Usage: $0 [-b bar] args"
exit 2
;;
esac
done
shift $((OPTIND - 1))
if [ -z "$bar_id" ]; then
echo "Missing bar ID"
exit 3
fi
bar_folder="$HOME/.config/waybar/$bar_id"
if [ ! -d "$bar_folder" ]; then
echo "Missing bar configuration folder: $bar_folder"
exit 4
fi
declare -a ARGS
folder_config="$bar_folder/config.json"
if [ -r "$folder_config" ]; then
ARGS+=(--config "$folder_config")
fi
folder_style="$bar_folder/style.css"
if [ -r "$folder_style" ]; then
ARGS+=(--style "$folder_style")
fi
exec waybar -b "$bar_id" "${ARGS[@]}" "$@" |
|
Thanks for sharing. I am currently limited on time, so I am not able to do further testing on this currently. I found another thing to consider: |
|
This PR is great, it works for me. It wasn't clear to me how to configure this but here's how I did it: Include the following in:
|
|
Thanks @woutdp! Nice that it works on your setup.
I do not know why, but this issue does not occur any longer here as well. At least on my notebook it is possible to tile at the side of the screen where the bar is. Next week I am going to test multi monitor setup and will have a look at your configuration @somini. In addition, I think it would be nice to add an "alert" that fires when the battery hits critical level. This could prevent surprising shutdowns when the bar is hidden. With "alert" I mean, that the bar automatically switches to be shown when the critical battery level has been reached. From then on, the bar behaves the same as before, if the user presses the MOD key, the bar hides again. Maybe this could be combined with the battery module, so that it automatically will be enabled when the battery module is enabled. |
|
I just tried your config @somini If you change your sway config to this: If Change your waybar config: Note the change to an array of bars in the waybar config. The In my opinion this PR is ready. Until now I did not discover any further issues. |
|
@emirror-de Thanks for testing this. So, from what I can tell, the trick is to have a single Waybar process do both bars at once, using an array of configurations. Did not know about that. I just can't reproduce it myself. I get both bars, but instead of hiding the top bar, the bar remains visible, but on top of the regular window. Here's before the first $Mod event, and afterwards.
Running this with debug logs, I get the message about hiding/showing: I pulled the latest code for this branch (c502e25) and here's my bar-related Sway config: And the waybar config: |
|
It works! I guess, just need more testing. According to #732 (comment), There's only a very small "bug", the bar starts out in the shown position, and only reverts to the hidden position on the first $Mod event (which triggers a main window resize). If the bar is to be hidden, I would expect it to start hidden. I would accept it if there's a strong technical issue that prevents this behaviour. |
Set "hide-on-startup" to true in your sway/hide config to enable.
|
This behavior is indeed intended. The idea behind this is in case there is only one bar per screen and with I added a config parameter to hide the bar containing the module on startup. To enable, add the following lines to your bar that should be hidden: |
|
Makes sense. Many thanks for that configuration option. I guess this is ready now, you can remove the Draft option now. Great work, I'll be using this more widely as soon as it goes in a release. |
|
I did some refactoring on the code to make it more readable. Also the behavior has been changed a bit:
Summary of config options: {
"sway/hide": {
"hide-on-startup": true,
"hide-to-bottom-layer": true
}
} |
|
@emirror-de so I finally found time to complete my alternative implementation in #1244. I looked through all the comments here and ended up with pretty similar approach and functionality (but cleaner design, IMO). Sorry for doing that instead of continuing to review this PR :-( Do you mind checking if there's something missing or behaving not as you expected in my branch? |
Updates the previous work by @somini and @xPMo to the latest master branch.
Fixes #255
Changes:
TOPlayer, when hidden on theBOTTOM