Improved hyprland/window by fixing icon search and implementing configurable spacing

[Azazel-Woodwind]

I noticed that some windows were displaying icons, while others weren't. I also found the gap between the icon and the window descriptor slightly excessive, and the module to be placed slightly too highly. This pull request aims to fix this.

Search

I fixed the icon issue by matching the app descriptor with the suffix of the .desktop file, instead of the whole file name. I did this because I noticed that some applications like the terminal emulator "Foot", had "footclient" as the class name but "org.codeberg.dnkl.footclient.desktop" as the file name.

I also made it so that the search attempts to match the lowercase string version of the class name to its respective .desktop file, as well as the original. I did this because I noticed that some applications like LibreWolf had "LibreWolf" as the class name but "librewolf.desktop" as the file name.

Implementation Details

This is my first time making a pull request here, so I'm not sure how tight the coding standards are. I tried to follow conventions in the codebase where possible, but I will explain some areas I wasn't sure about - feel free to critique my code and suggest any improvements - I would fix them promptly.

To fix the search, I made some utility functions like to_lowercase, and getFileBySuffix. These functions are reusable, so I wasn't sure whether to put them in the waybar namespace or not. I did anyways, but if someone suggests that I put them outside the namespace, or define them in a separate utility functions file (since that is what I would do), I'm happy to do so.

In my getFileBySuffix function, I added overloading for a check_lower_case boolean value which is false by default. I did this as file names are case sensitive, so I did not want to check for lower and uppercase by default.

Spacing

I'm not sure if this is an issue on my system specifically, but the spacing for me was rather jarring. I made the icon and window descriptor closer together, and moved them both down by default. You may wonder why I moved them down in the code instead of allowing the user to do this through CSS, and that's because for some reason, CSS margins and padding only affect the window descriptor, not the icon.

To try to be less opinionated, I made them both configurable in the config file under names icon-spacing and margin-top. I didn't think to add any other margin types, because horizontal spacing can be changed in the waybar config file, and vertical spacing can be controlled completely with margin-top as when this is set to 0, the box is right at the top of the bar. (See EDIT/UPDATE below)

I added comments where I thought necessary - let me know if they're too excessive or if there aren't enough of them.

For context, here are some screenshots of what it looked like before (with icon set to true):


And here are some after:

EDIT/UPDATE:

As per changes recommended by @alebastr, the #window selector now refers to the container box of both the image and label, as opposed to just the label. Each can be styled individually with #window > label and #window > image. The aforementioned configuration options have been removed in favour of this.

[alebastr]

Indeed, it's unfortunate that the AIconLabel is written in a way that prevents matching the image with a selector.

I see two solutions here:

• make the icon separately identifiable in the CSS by #window-icon

  @@ -10,7 +10,7 @@ AIconLabel::AIconLabel(const Json::Value &config, const std::string &name, const
       : ALabel(config, name, id, format, interval, ellipsize, enable_click, enable_scroll) {
     event_box_.remove();
     box_.set_orientation(Gtk::Orientation::ORIENTATION_HORIZONTAL);
  -  box_.set_spacing(8);
  +  image_.set_name(name + "-icon");
     box_.add(image_);
     box_.add(label_);
     event_box_.add(box_);

• lift the name to the box_ and style #window > image/#window > label
  (Not a complete diff, need to move CSS classes from the label_ too)

  @@ -9,8 +9,9 @@ AIconLabel::AIconLabel(const Json::Value &config, const std::string &name, const
                          bool enable_click, bool enable_scroll)
       : ALabel(config, name, id, format, interval, ellipsize, enable_click, enable_scroll) {
     event_box_.remove();
  +  label_.unset_name();
     box_.set_orientation(Gtk::Orientation::ORIENTATION_HORIZONTAL);
  -  box_.set_spacing(8);
  +  box_.set_name(name);
     box_.add(image_);
     box_.add(label_);
     event_box_.add(box_);

The former is 100% backward compatible, but the latter is more architecturally correct — we already have name on the GtkBox for keyboard-state, upower, gamemode etc.

[Azazel-Woodwind]

@alebastr I've implemented the second solution you proposed - let me know what you think.

I've run clang-format and clang-tidy on the files that I changed - clang-format runs fine but clang-tidy cries about all the snake case being used. Although it's unusual for a cpp repository, I stuck to using snake_case for variable names since that seemed to be the convention, but clang-tidy complains about it, including in files that I haven't touched. Should I bother with this?

[alebastr]

@alebastr I've implemented the second solution you proposed - let me know what you think.

Looks fine to me, but the default state without styles has regressed to no padding :(

I'm looking at Gamemode and Privacy modules now, and icon-spacing seems like a good idea (independently of moving set_name), just because it can have a default without requiring a certain rule in style.css.
Sorry for asking you to reintroduce the option right after deleting it.

I've run clang-format and clang-tidy on the files that I changed - clang-format runs fine but clang-tidy cries about all the snake case being used. Although it's unusual for a cpp repository, I stuck to using snake_case for variable names since that seemed to be the convention, but clang-tidy complains about it, including in files that I haven't touched. Should I bother with this?

Uh... clang-tidy is currently tuned for the style used in modules/hyprland, which slightly differs from the rest of the project :(

---

I'm seeing an interesting effect with this PR: an icon for an unfocused output in sway/window (app_id '', alternative_app_id '') now resolves to com.github.Eloston.UngoogledChromium. Any ideas?

[Azazel-Woodwind]

Looks fine to me, but the default state without styles has regressed to no padding :(

Yeah so I realised that the reason why the spacing between the icon and window descriptor was so jarringly large for me was because I missed that there were margins added to all elements in the bar in my style.css, which is why I removed the spacing altogether - I've added the default spacing back.

I'm looking at Gamemode and Privacy modules now, and icon-spacing seems like a good idea (independently of moving set_name), just because it can have a default without requiring a certain rule in style.css.
Sorry for asking you to reintroduce the option right after deleting it.

It's fine, I've added this back.

I'm seeing an interesting effect with this PR: an icon for an unfocused output in sway/window (app_id '', alternative_app_id '') now resolves to com.github.Eloston.UngoogledChromium. Any ideas?

So this is an interesting one - I'm not running sway so I can't be certain but I have a hypothesis. First of all, without my PR, I believe that there is an existing bug that causes the icon of the last active window to display even when the current workspace has no windows, or there is no focused window. This is because there is no check for this, and the image is never removed. I fixed this in hyprland/window in my most recent push.

Do you mean that the path for the icon resolves to com.github.Eloston.UngoogledChromium.desktop? If so, I think the reason is because without my PR, AAppIconLabel.cpp would simply search for UngoogledChromium.desktop, which wouldn't exist, and therefore no icon would be displayed. Is this what happens on your system without the PR?

So now assuming that it finds the correct .desktop file, the reason why you should be seeing the chromium icon even though your output is unfocused is due to the bug that I just mentioned - the last active window should have been chromium. Let me know if this is not the case.

As I mentioned, I only fixed this in hyprland/window since that is what this pull request is about, but if you like, I can either edit this PR to include fixes to sway/window or open a new PR.

Implementation details of my most recent changes

About my most recent push, I needed to find a way to determine in Window::update whether an image should be displayed. An image should be displayed if a window is focused, but I couldn't find a reliable measure to tell if no window is focused. Initially, I used workspace_.windows > 0, but this doesn't take into account a case where you have a workspace with windows but are not focused on any of them (if that's even possible).

However, you can determine in Window::queryActiveWorkspace if the window is focused or not. I could have simply run image_.hide(); in Window::queryActiveWorkspace if no window is focused, but I do not believe this to be architecturally correct, as if I'm not mistaken, visible updates should be delegated to the update method. Therefore, I created a new member variable focused in window.hpp and used it accordingly. @alebastr Let me know if this approach is fine.

[alebastr]

I mean that getIconName("", "") returned com.github.Eloston.UngoogledChromium (checked with a log statement). I wasn't running it, and getIconName is stateless anyways.
I'll try to debug that in a few hours.

Sorry, can't help with the Hyprland module changes. You might want to tag @zjeffer or other people who are actively working on Hyprland support.

[zjeffer]

I'll try to have a look at this PR sometime this week. I'm only familiar with the workspaces.cpp file, not the window.cpp file though.

[Azazel-Woodwind]

I mean that getIconName("", "") returned com.github.Eloston.UngoogledChromium

I see the cause - I will push a fix for this soon. UPDATE: think this is fixed?

Thanks @alebastr for your help so far, and thanks @zjeffer for taking a look - window.cpp is not so complicated. I don't understand everything going on in that file but enough to make the changes. I haven't changed much anyways so looking over it shouldn't be too much of a hassle.

[Alexays]

Mhmm 8 was the default before the change, should be set to 8

[Azazel-Woodwind]

My bad, I've amended this in my most recent push.

[zjeffer]

window.cpp looks good to me

[Alexays]

LGTM, thx!

[tomben13]

The window name is no longer centered on the module after this PR. It is all the way to the left. Is this expected ?

I use #window { min-width: 300px; }

[Azazel-Woodwind]

The window name is no longer centered on the module after this PR. It is all the way to the left. Is this expected ?

Yes, this is expected as previously, #window referred to the label element that contains the window descriptor. Now, #window refers to the entire container consisting of both the optional image element and label element.

I use #window { min-width: 300px; }

If you want the label to be centered, try applying min-width to #window > label instead. If you want to apply styling to the image, use #window > image. Let me know if that works.

[tomben13]

If you want the label to be centered, try applying min-width to #window > label instead.

It works, thank you.