Show hidden bar #1510

towoe · 2022-04-07T15:41:21Z

Show bar if a workspace becomes urgent
Clear urgency hint with modifier press
Show bar on sway mode

Feedback on the coding is welcome, not sure with regards to the signal and ipc creation.

I followed the comment in swaybar to implement the behavior to hide the bar again after the modifier is pressed.
But my implementation might differ a bit as it will show the bar again if multiple workspaces have the urgency hint, the bar is hidden again, and then one of the urgent workspaces is focused. For me this makes sense as it will give a notification that other workspaces sill have the urgency hint.

alebastr · 2022-04-07T16:06:58Z

src/modules/sway/bar.cpp

+  ipc_mode_.subscribe(R"(["mode"])");
+  ipc_mode_.signal_event.connect(sigc::mem_fun(*this, &BarIpcClient::onEventMode));
+  ipc_workspace_.subscribe(R"(["workspace"])");
+  ipc_workspace_.signal_event.connect(sigc::mem_fun(*this, &BarIpcClient::onEventUrgency));


You don't need to create multiple ipc connections to handle different event types; one connection is already enough.

void BarIpcClient::onIpcEvent(const struct Ipc::ipc_response& res) { switch(res.type) { case IPC_EVENT_BARCONFIG_UPDATE: ... break; case IPC_EVENT_BAR_STATE_UPDATE: ... break; case IPC_EVENT_WORKSPACE: ... break; case IPC_EVENT_MODE: ... }

Thanks for the tip!

alebastr · 2022-04-07T16:19:23Z

src/modules/sway/bar.cpp

+    } else if (payload.isMember("change")) {
+      spdlog::trace("BarIpcClient::onIpcEvent: got event {}", payload["change"].asString());
+      signal_last_modifier_(false);


What is the event type here?

I used this to store that the last modifier key press was followed by an event which was the "reason" for the key press, and not clear the visible status for the bar.

My reason to use a signal here was that I assumed that one onIpcEvent could be active while another one could be started, but not sure how ipc_.signal_event.connect(...) works.

not sure how ipc_.signal_event.connect(...) works

The logic is following:

everything connected to the ipc_ signals directly will be executed in the ipc worker thread, sequentially (in order of receiving the events from the socket). But each ipc connection will have its own worker thread.

everything connected to SafeSignal instances will be queued for execution in the main (UI) thread, in the order of event emission.

SafeSignal.emit called from the main (UI) thread will bypass the queue and get processed synchronously.

tl;dr: event handlers in different threads may run in parallel, events in the same thread are serialized.

alebastr · 2022-04-07T16:27:39Z

src/modules/sway/bar.cpp

+  if (!visible_by_modifier_ && last_press_modifier_) {
+    // Modifier key was pressed and released without a different action.
+    // This signals an acknowledgment and should hide the bar again.
+    // Hide the bar and clear the mode and urgency flag.
+    visible = false;
+    visible_by_urgency_ = false;
+    visible_by_mode_ = false;


That doesn't match with swaybar logic. Why do we need to track last_press_modifier_?
Also, you can set visible_by_urgency_/visible_by_mode_ in onVisibilityUpdate.

Could you explain why that does not match with swaybar logic?
I had a look at swaybar, but I also think that the behavior was a little buggy, but I do not claim that I know the correct logic which should be implemented.

swaybar logic - reset visible_by_urgency_/visible_by_mode_ when visible_by_modifier_ is set by the IPC event.

I think I get why you believe it is buggy - new urgent or mode event may come while the modifier is still pressed and set the flag again.
That may as well be intentional, in order to make mode indicator stick after it was enabled by the binding. I don't claim I know how that should work either. Maybe pinging emersion to clarify swaybar behavior would be a good idea.

Anyways, following could be an easier way to get a similar result:

void BarIpcClient::onVisibilityUpdate(bool visible_by_modifier) { visible_by_modifier_ = visible_by_modifier; /* reset other visibility flags */ if (visible_by_modifier) { visible_by_urgency_ = false; } } void BarIpcClient::onUrgencyUpdate(bool visible_by_urgency) { ... /* don't set the flag if the modifier is held */ visible_by_urgency_ = visible_by_urgency && !visible_by_modifier_; }

Thanks for feedback!

swaybar logic - reset visible_by_urgency_/visible_by_mode_ when visible_by_modifier_ is set by the IPC event.

So this is this comment. My interpretation of this was to "ignore" visible_by_modifier if another reason for visibility existed, as the bar is not visible by modifier but visible by e.g. urgency already.

In my setup the modifier key that shows the bar is the same as my "normal" modifier key for all the other key bindings, this might be the reason why I implemented the logic to wait for other events after the visible_my_modifier to make sure it was not followed by another event, meaning that the intended action of pressing the key was not meant to clear the visibility flag.

I think I get why you believe it is buggy - new urgent or mode event may come while the modifier is still pressed and set the flag again.

Actually I did not think about this behavior, but I think if the event comes while the key is pressed, it is still shown after releasing the key.

My observation was the following (with the sway prefix key equal the modifier key for the bar):

if the bar is visible by urgency, pressing and releasing the mod key hides the bar: okay

if the bar is visible by urgency, pressing the mod key in combination with an workspace change keeps the bar visible after releasing the key: according to the referenced comment not correct, but from my point of view a correct behavior

if the bar is visible by urgency, pressing the mod key to e.g. change the focus in the same workspace hides the bar: not good

changing the workspace will make the bar visible again

Maybe pinging emersion to clarify swaybar behavior would be a good idea.

If you are now also in doubt of the intended behavior it might be good the get some input, but maybe it is just that I have some edge case setup for which is was not intended and needs a fix.
Not sure if you agree on this, but if sway does not make any rules how the bar should behave, might it not be okay if different bars behave differently?

I just confirmed that the swaybar behavior was copied mostly verbatim from i3bar. Any modifier press resets the visibility state of the bar, even if it was a part of the binding. So we can say it's intentional.

I'm fine with both options, with a slight preference towards swaybar/i3bar behavior.

if the bar is visible by urgency, pressing the mod key to e.g. change the focus in the same workspace hides the bar: not good

changing the workspace will make the bar visible again

Actually, that second part sounds like a bug. i3 does not make the bar visible again on the workspace change, unless the last change in the workspace event was "urgent" and there are other workspaces with urgency hint set (and I can't figure out from the code why/how i3bar does that).

I added a separate commit in which both options should be possible.
It is a bit messy, I think the bar config from the waybar config file will not be used if sway ipc is active? I guess it is not possible to see from the ipc message if a change occurs so we just use the values from sway?

One option is to reset the bar visibility right at pressing the mod key.
The other is the "release" part, which I defined as pressing the mod key and releasing it again, which for me makes more sense because it allows to start something without loosing the urgency hint and also to get rid of the bar if one intends to do so.
But I am open to feedback to adding a new config option.

I also changed the way the bar will reset visibility on a workspace change, it should now stay hidden if the urgency is cleared until a new event is received.

Alexays · 2022-04-10T08:39:34Z

src/modules/sway/bar.cpp

+        if (auto id = payload["id"]; id.isString() && id.asString() != bar_.bar_id) {
+          spdlog::trace("swaybar ipc: ignore event for {}", id.asString());
+          return;
+        }


We should have this condition for all type, no?

No, only these 2 events can be sent to a specific instance of a bar and may have bar_id.

alebastr

One more thing to consider - this change shouldn't affect the bars that don't show mode indicator or workspaces.

Possible implementation:

bool isModuleEnabled(const Json::Value& config, std::string name) {
  for (const auto& section : {"modules-left", "modules-center", "modules-right"}) {
    if (const auto& modules = config.get(section, {}); modules.isArray()) {
      for (const auto& module : modules) {
        if (module.asString().rfind(name, 0) == 0) {
          return true;
        }
      }
    }
  }
  return false;
}

BarIpcClient::BarIpcClient(waybar::Bar& bar) : bar_{bar} {
  // ...
  Json::Value events{Json::arrayValue};
  events.append("bar_state_update");
  events.append("barconfig_update");

  bool has_mode = isModuleEnabled(bar.config, "sway/mode");
  bool has_ws = isModuleEnabled(bar.config, "sway/workspaces");
  if (has_mode) {
    events.append("mode");
  }
  if (has_ws) {
    events.append("workspace");
  }
  if (has_mode || has_ws) {
    events.append("binding");
  }

  // Subscribe to non bar events to determine if the modifier key press is followed by another
  // action.
  std::stringstream oss;
  oss << events;
  ipc_.subscribe(oss.str());
  // ...
}

alebastr · 2022-04-10T20:06:01Z

src/modules/sway/bar.cpp

-      auto config = parseConfig(payload);
-      signal_config_(std::move(config));
+    switch (res.type) {
+      case static_cast<uint32_t>(IPC_EVENT_WORKSPACE):


Ouch. I made #1516 to fix the enum type.
Please, drop the casts once it's merged.

Should have followed your suggestion literally :)

Wasn't possible before the aforementioned PR; the compiler wouldn't allow you :)

alebastr · 2022-04-10T20:12:30Z

src/modules/sway/bar.cpp

+        if (auto id = payload["id"]; id.isString() && id.asString() != bar_.bar_id) {
+          spdlog::trace("swaybar ipc: ignore event for {}", id.asString());
+          return;
+        }


No, only these 2 events can be sent to a specific instance of a bar and may have bar_id.

alebastr · 2022-04-10T20:23:45Z

src/modules/sway/bar.cpp

+            // least one workspace
+            ipc_.sendCmd(IPC_GET_WORKSPACES);
+          }
+          last_press_modifier_ = false;


You are changing the last_press_modifier_ directly from the ipc worker thread. It would be preferable to use std::atomic<bool> or signal.

Sorry, might have misunderstood your explanation before, I thought it will only be possible to write to this at one point.
Added the atomic, not sure if just changing the type was enough, though?

Yep, changing the type is enough. std::atomic<T>::operator= is overloaded to take care of atomic assignments when we don't need to swap or check the current value.

I thought it will only be possible to write to this at one point.

Change to non-atomic variable in one thread may be not (immediately) visible to a read operation in another thread; this behavior is undefined in the C++ memory model. That's why I insisted on some kind of synchronization.

Add a second reason to show the bar besides visible by modifier. Update the visibility based on changes in the workspace urgency. Check all workspaces for urgency and keep the bar visible if at least one has an urgency hint.

towoe · 2022-04-14T17:16:16Z

One more thing to consider - this change shouldn't affect the bars that don't show mode indicator or workspaces.

That's a good point, I did not have that in mind. Thanks for the code, I added this check in a different commit so it is easier for review, can squash that later.

alebastr · 2022-04-15T03:31:37Z

src/modules/sway/bar.cpp

+  }
+
+  for (auto& module : module_check) {
+    if (active_modules.find(module.first) != active_modules.end()) {


Note that sway/workspaces#bar-0 with an optional id after # is a valid module reference in the config. Exact string comparison will fail to find that, thus the prefix match with module.asString().rfind(name,0) in my PoC code.

To address that, you can try dropping the module ids (# and everything after) at line 19.

Your PoC code was cleaner than the one I added now. Any preference on how to do it differently? My thinking was to not do the loop multiple times...

Well, that's a short loop (just how many modules can be placed on a single bar?) and it's done only once for each visible bar instance. I'd personally pick readability over the optimization here.

Made the function into a member, looked nicer with the config access, hope that is not a problem?

src/modules/sway/bar.cpp

If the modifier is pressed and release without another event, the intended behaviour is to clear an urgency hint and hide the bar again. Note that if multiple workspaces have the urgency hint set, the bar is hidden again and an urgent workspace is focused, the bar does not stay hidden anymore.

Display the bar if the sway mode is not the default mode.

Prevent visibility updates to occur for inactive modules. Check active modules and subscribe to only those events.

Add an option to change the behaviour of the modifier key to reset the visibility.

towoe · 2022-04-15T18:33:48Z

@alebastr thanks so much for all the review already.
I renamed the last_press_modifier_ to modifier_no_action_, hopefully that makes it a bit easier to understand.

Alexays · 2022-04-20T03:13:50Z

Thx a lot!
Can you also update the github wiki?

alebastr reviewed Apr 7, 2022

View reviewed changes

towoe force-pushed the sway-bar-show branch from e5d0d10 to 7c32302 Compare April 9, 2022 11:58

Alexays reviewed Apr 10, 2022

View reviewed changes

alebastr reviewed Apr 10, 2022

View reviewed changes

Show bar if a workspace becomes urgent

e3700b9

Add a second reason to show the bar besides visible by modifier. Update the visibility based on changes in the workspace urgency. Check all workspaces for urgency and keep the bar visible if at least one has an urgency hint.

towoe force-pushed the sway-bar-show branch from 7c32302 to f6ee6df Compare April 14, 2022 17:13

alebastr reviewed Apr 15, 2022

View reviewed changes

towoe force-pushed the sway-bar-show branch from f6ee6df to 803ab4f Compare April 15, 2022 06:01

alebastr approved these changes Apr 15, 2022

View reviewed changes

src/modules/sway/bar.cpp Outdated Show resolved Hide resolved

towoe added 4 commits April 15, 2022 19:55

Show bar on sway mode

1dcd36b

Display the bar if the sway mode is not the default mode.

Limit visibility updates

5a0e42c

Prevent visibility updates to occur for inactive modules. Check active modules and subscribe to only those events.

Add reset behaviour of modifier key

7763d50

Add an option to change the behaviour of the modifier key to reset the visibility.

towoe force-pushed the sway-bar-show branch from 803ab4f to 7763d50 Compare April 15, 2022 18:28

Alexays approved these changes Apr 20, 2022

View reviewed changes

Alexays merged commit 89be55b into Alexays:master Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show hidden bar #1510

Show hidden bar #1510

towoe commented Apr 7, 2022

alebastr Apr 7, 2022

towoe Apr 7, 2022

alebastr Apr 7, 2022

towoe Apr 7, 2022

towoe Apr 7, 2022

alebastr Apr 7, 2022

alebastr Apr 7, 2022

towoe Apr 7, 2022

alebastr Apr 7, 2022 •

edited

towoe Apr 8, 2022

alebastr Apr 10, 2022

towoe Apr 14, 2022

Alexays Apr 10, 2022

alebastr Apr 10, 2022

alebastr left a comment

alebastr Apr 10, 2022

towoe Apr 14, 2022

alebastr Apr 15, 2022

alebastr Apr 10, 2022

alebastr Apr 10, 2022

towoe Apr 14, 2022

alebastr Apr 15, 2022

towoe commented Apr 14, 2022

alebastr Apr 15, 2022

towoe Apr 15, 2022

alebastr Apr 15, 2022

towoe Apr 15, 2022

towoe commented Apr 15, 2022

Alexays commented Apr 20, 2022

Show hidden bar #1510

Show hidden bar #1510

Conversation

towoe commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebastr Apr 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebastr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

towoe commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

towoe commented Apr 15, 2022

Alexays commented Apr 20, 2022

alebastr Apr 7, 2022 •

edited