Icon & Tray support #324

snowytrees commented 2023年11月14日 09:12:01 +01:00

This adds icon support (first commit) and tray support (second commit). See commit messages for details. Closes #4

Example config:

bar:
 height: 32
 location: top
 background: ff00ff00
 icon-basedirs: ["/home/jd/.nix-profile/share/icons"]
 icon-themes:
 - "La Capitaine"
 - "Breeze"
 icon-size: 24
 right:
 - label:
 content:
 - icon:
 name: "firefox"
 icon-size: 32
 - icon:
 name: "impress"
 - tray:
 content:
 map:
 conditions:
 +icon-pixmap:
 - icon:
 name: "icon-pixmap"
 use-tag: true
 ~+icon-pixmap:
 - icon:
 name: "{icon-name}"
 use-tag: false
 fallback:
 - string: {text: "{icon-name}"}

This adds icon support (first commit) and tray support (second commit). See commit messages for details. Closes #4 Example config: ``` bar: height: 32 location: top background: ff00ff00 icon-basedirs: ["/home/jd/.nix-profile/share/icons"] icon-themes: - "La Capitaine" - "Breeze" icon-size: 24 right: - label: content: - icon: name: "firefox" icon-size: 32 - icon: name: "impress" - tray: content: map: conditions: +icon-pixmap: - icon: name: "icon-pixmap" use-tag: true ~+icon-pixmap: - icon: name: "{icon-name}" use-tag: false fallback: - string: {text: "{icon-name}"} ```

krath commented 2023年12月26日 15:05:49 +01:00

Looking forward to this feature. Tried to build and run on minimal config (w and wo tray), but segfaults on load. Using same build parameters as AUR yambar-git (which works fine, but obviously no tray :-D).

Looking forward to this feature. Tried to build and run on minimal config (w and wo tray), but segfaults on load. Using same build parameters as AUR yambar-git (which works fine, but obviously no tray :-D).

bagnaram commented 2023年12月28日 19:55:20 +01:00

It works for me! Although icon lookups across themes seems to be finnicky. Nice work!

bar:
 icon-basedirs: ["/home/bagnaram/.icons"]
 icon-theme: "GNUstep"
 icon-size: 24
...
 right:
 - label:
 content:
 - icon:
 icon-size: 24
 name: "KolourPaint"
 - icon:
 name: "firefox"
 icon-size: 24
 - tray:
 content:
 - icon:
 name: "icon-pixmap"
 use-tag: true 

image

Edit:
I also added a couple NULL checks

diff --git a/icon.c b/icon.c
index fec02c9..24ba741 100644
--- a/icon.c
+++ b/icon.c
@@ -472,6 +472,7 @@ load_themes_in_dir(char *basedir)
 void
 log_loaded_themes(themes_t themes)
 {
+ LOG_INFO("Logging loaded themes");
 if (tll_length(themes) == 0) {
 LOG_INFO("Warning: no icon themes loaded");
 return;
@@ -480,7 +481,11 @@ log_loaded_themes(themes_t themes)
 size_t sep_len = strlen(sep);
 
 size_t len = 0;
- tll_foreach(themes, it) { len += strlen(it->item->name) + sep_len; }
+ tll_foreach(themes, it) {
+ if (it->item->name != NULL) {
+ len += strlen(it->item->name) + sep_len;
+ }
+ }
 
 char *str = malloc(len + 1);
 if (!str) {
@@ -497,9 +502,12 @@ log_loaded_themes(themes_t themes)
 start = false;
 
 struct icon_theme *theme = it->item;
- size_t name_len = strlen(theme->name);
- memcpy(p, theme->name, name_len);
- p += name_len;
+ LOG_DBG("Logging theme '%s'", theme->name);
+ if (it->item->name != NULL) {
+ size_t name_len = strlen(theme->name);
+ memcpy(p, theme->name, name_len);
+ p += name_len;
+ }
 }
 *p = '0円';
 
@@ -537,11 +545,16 @@ init_themes(struct basedirs *basedirs)
 tll_foreach(basedirs->basedirs, it)
 {
 themes_t dir_themes = load_themes_in_dir(it->item);
+
 tll_foreach(dir_themes, it)
 {
 struct icon_theme *theme = it->item;
- tll_remove(dir_themes, it);
- tll_push_back(themes, theme);
+ LOG_DBG("themes '%s' loaded", theme->name);
+ if (theme->name != NULL)
+ {
+ tll_remove(dir_themes, it);
+ tll_push_back(themes, theme);
+ }
 }
 }

It works for me! Although icon lookups across themes seems to be finnicky. Nice work! ``` bar: icon-basedirs: ["/home/bagnaram/.icons"] icon-theme: "GNUstep" icon-size: 24 ... right: - label: content: - icon: icon-size: 24 name: "KolourPaint" - icon: name: "firefox" icon-size: 24 - tray: content: - icon: name: "icon-pixmap" use-tag: true ```  Edit: I also added a couple NULL checks ```diff diff --git a/icon.c b/icon.c index fec02c9..24ba741 100644 --- a/icon.c +++ b/icon.c @@ -472,6 +472,7 @@ load_themes_in_dir(char *basedir) void log_loaded_themes(themes_t themes) { + LOG_INFO("Logging loaded themes"); if (tll_length(themes) == 0) { LOG_INFO("Warning: no icon themes loaded"); return; @@ -480,7 +481,11 @@ log_loaded_themes(themes_t themes) size_t sep_len = strlen(sep); size_t len = 0; - tll_foreach(themes, it) { len += strlen(it->item->name) + sep_len; } + tll_foreach(themes, it) { + if (it->item->name != NULL) { + len += strlen(it->item->name) + sep_len; + } + } char *str = malloc(len + 1); if (!str) { @@ -497,9 +502,12 @@ log_loaded_themes(themes_t themes) start = false; struct icon_theme *theme = it->item; - size_t name_len = strlen(theme->name); - memcpy(p, theme->name, name_len); - p += name_len; + LOG_DBG("Logging theme '%s'", theme->name); + if (it->item->name != NULL) { + size_t name_len = strlen(theme->name); + memcpy(p, theme->name, name_len); + p += name_len; + } } *p = '0円'; @@ -537,11 +545,16 @@ init_themes(struct basedirs *basedirs) tll_foreach(basedirs->basedirs, it) { themes_t dir_themes = load_themes_in_dir(it->item); + tll_foreach(dir_themes, it) { struct icon_theme *theme = it->item; - tll_remove(dir_themes, it); - tll_push_back(themes, theme); + LOG_DBG("themes '%s' loaded", theme->name); + if (theme->name != NULL) + { + tll_remove(dir_themes, it); + tll_push_back(themes, theme); + } } } ```

krath commented 2023年12月28日 20:10:29 +01:00

It works for me! Although icon lookups across themes seems to be finnicky. Nice work!

bar:
 icon-basedirs: ["/home/bagnaram/.icons"]
 icon-theme: "GNUstep"
 icon-size: 24
...
 right:
 - label:
 content:
 - icon:
 icon-size: 24
 name: "KolourPaint"
 - icon:
 name: "firefox"
 icon-size: 24
 - tray:
 content:
 - icon:
 name: "icon-pixmap"
 use-tag: true 

image

Well, good for you! Have you tried without any icon config? If missing or wrong icon config is the issue I experience with the segfault instead of startup, then it's definitely a no-go, and not "nice work" ("nice try", at best) xD. People who don't want this feature shouldn't be forced to configure it.

> It works for me! Although icon lookups across themes seems to be finnicky. Nice work! > > ``` > bar: > icon-basedirs: ["/home/bagnaram/.icons"] > icon-theme: "GNUstep" > icon-size: 24 > ... > right: > - label: > content: > - icon: > icon-size: 24 > name: "KolourPaint" > - icon: > name: "firefox" > icon-size: 24 > - tray: > content: > - icon: > name: "icon-pixmap" > use-tag: true > > ``` > >  Well, good for you! Have you tried without any icon config? If missing or wrong icon config is the issue I experience with the segfault instead of startup, then it's definitely a no-go, and **not** "nice work" ("nice try", at best) xD. People who don't want this feature shouldn't be forced to configure it.

bagnaram commented 2023年12月28日 20:38:12 +01:00

Mainly nice job to the contributor for implementing this greatly awaited capability 😅

See edit above I saw my theme config contained invalid entries while gdb testing so I added some checks

Mainly nice job to the contributor for implementing this greatly awaited capability 😅 See edit above I saw my theme config contained invalid entries while `gdb` testing so I added some checks

bagnaram commented 2024年01月07日 17:40:15 +01:00

I have been running this branch as my daily panel for a week and I noticed a gradual memory leak issue causing consumption to go up. I have observed upwards of 200MB memory usage over a couple hours while the inital startup of yambar is around 39MB. It often increases to over 100MB memory usage after a while

I have been running this branch as my daily panel for a week and I noticed a gradual memory leak issue causing consumption to go up. I have observed upwards of 200MB memory usage over a couple hours while the inital startup of `yambar` is around 39MB. It often increases to over 100MB memory usage after a while

snowytrees commented 2024年07月21日 03:24:22 +02:00

@bagnaram I am revisiting this to push it over the finish line. Do you have the theme file that caused the nullptr de-reference? We should not have any loaded themes without theme names.

 if (strcasecmp(old_group, "icon theme") == 0) {
 if (!theme->name) {
 return "missing required key 'Name'";
 } else if (!theme->comment) {
 return "missing required key 'Comment'";
 } else if (tll_length(theme->directories) == 0) {
 return "missing required key 'Directories'";
 }
 } 

The parser is either doing something incorrect, or not catching an invalid theme before insertion.

edit: found the bug & fixed.

Also @dnkl is there a reason yambar is not doing xalloc style allocations? I am trying to handle alloc failures but things like e.g. tll don't allow for it.

@bagnaram I am revisiting this to push it over the finish line. Do you have the theme file that caused the nullptr de-reference? We should not have any loaded themes without theme names. ``` if (strcasecmp(old_group, "icon theme") == 0) { if (!theme->name) { return "missing required key 'Name'"; } else if (!theme->comment) { return "missing required key 'Comment'"; } else if (tll_length(theme->directories) == 0) { return "missing required key 'Directories'"; } } ``` The parser is either doing something incorrect, or not catching an invalid theme before insertion. edit: found the bug & fixed. Also @dnkl is there a reason yambar is not doing xalloc style allocations? I am trying to handle alloc failures but things like e.g. tll don't allow for it.

snowytrees commented 2024年07月21日 09:47:13 +02:00

Pushed a new commit with a bunch of fixes:

1. All memory leaks except for sd-bus.
2. Misc. nullptr fixes
3. Cleaned up code & brought icon code more inline with fuzzel. which brings along...
4. Pixmap/png scaling :)

The following things are still left:

1. The theme parsing nullptr issue above
2. Some internal memory leaks related to sd-bus
3. Clean up formatting + redo the commit messages
4. Public documentation

Leaving the icon-cache to a future diff.

Pushed a new commit with a bunch of fixes: 1. All memory leaks except for sd-bus. 2. Misc. nullptr fixes 3. Cleaned up code & brought icon code more inline with fuzzel. which brings along... 4. Pixmap/png scaling :) The following things are still left: 1. The theme parsing nullptr issue above 2. Some internal memory leaks related to sd-bus 3. Clean up formatting + redo the commit messages 4. Public documentation Leaving the icon-cache to a future diff.

snowytrees commented 2024年07月21日 21:28:04 +02:00

Force pushed another update.

• Fixes the theme nullptr issue (parser returned a warning rather then an error).
• Format using clang-format
• More code cleanup
• Better config programmability (icon tag conditions, multiple icon themes, icon particle fallback). See example config in commit for details.

Still todo:

• sd-bus memory leaks
• Public documentation

Force pushed another update. * Fixes the theme nullptr issue (parser returned a warning rather then an error). * Format using clang-format * More code cleanup * Better config programmability (icon tag conditions, multiple icon themes, icon particle fallback). See example config in commit for details. Still todo: * sd-bus memory leaks * Public documentation

snowytrees commented 2024年07月22日 03:40:57 +02:00

Updates:

• Move overviews into commit messages
• Correctly follow the spec for dbus property change events. This resolves a bug in the swaybar implementation. It manifested as a startup race condition where solaar would sometimes beat the registration of the watcher and would never register it's item. As it expected to be notified upon registration change through IsStatusNotifierHostRegistered property change event.
• Cleaned up tray code & much better debug logging.

Still todo:

• sd-dbus memory leaks
• Public documentation

Updates: * Move overviews into commit messages * Correctly follow the spec for dbus property change events. This resolves a bug in the swaybar implementation. It manifested as a startup race condition where solaar would sometimes beat the registration of the watcher and would never register it's item. As it expected to be notified upon registration change through `IsStatusNotifierHostRegistered` property change event. * Cleaned up tray code & much better debug logging. Still todo: * sd-dbus memory leaks * Public documentation

snowytrees commented 2024年07月22日 07:15:32 +02:00

This should now be truly stable to try out (famous last words). Only thing left is to write public documentation & go through review.

Updates:

• Solved sd-bus memory leaks :)

==592831== LEAK SUMMARY:
==592831== definitely lost: 0 bytes in 0 blocks
==592831== indirectly lost: 0 bytes in 0 blocks
==592831== possibly lost: 0 bytes in 0 blocks
==592831== still reachable: 29,224 bytes in 14 blocks
==592831== suppressed: 0 bytes in 0 blocks
==592831=わ=わ 
=わ=わ592831== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

• Some more code cleanup

Todo:

• Public documentation

This should now be truly stable to try out (famous last words). Only thing left is to write public documentation & go through review. Updates: * Solved sd-bus memory leaks :) ``` ==592831== LEAK SUMMARY: ==592831== definitely lost: 0 bytes in 0 blocks ==592831== indirectly lost: 0 bytes in 0 blocks ==592831== possibly lost: 0 bytes in 0 blocks ==592831== still reachable: 29,224 bytes in 14 blocks ==592831== suppressed: 0 bytes in 0 blocks ==592831=わ=わ =わ=わ592831== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) ``` * Some more code cleanup Todo: * Public documentation

dnkl commented 2024年07月22日 07:16:23 +02:00

Also @dnkl is there a reason yambar is not doing xalloc style allocations?

No reason other than "no one has implemented it yet" ;)

> Also @dnkl is there a reason yambar is not doing xalloc style allocations? No reason other than "no one has implemented it yet" ;)

bagnaram commented 2024年07月23日 17:44:26 +02:00

Thank you for the implementation! I am building this locally but the uninitialized warnings how did you get past those for the nano-svg library without setting werror for the proj?

Thank you for the implementation! I am building this locally but the uninitialized warnings how did you get past those for the nano-svg library without setting werror for the proj?

bagnaram commented 2024年07月24日 03:32:20 +02:00

I got it to build and have the same segmentation fault as I had earlier parsing the themes. I am also on a Fedora box this time. Here is the tail of my syscall trace

openat(AT_FDCWD, "/usr/share//icons/oxygen/index.theme", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share//icons/default/index.theme", O_RDONLY) = 6
fstat(6, {st_mode=S_IFREG|0644, st_size=30, ...}) = 0
read(6, "[Icon Theme]\nInherits=Adwaita\n", 4096) = 30
read(6, "", 4096) = 0
close(6) = 0
openat(AT_FDCWD, "/usr/share//icons/HighContrast/index.theme", O_RDONLY) = 6
fstat(6, {st_mode=S_IFREG|0644, st_size=5634, ...}) = 0
read(6, "[Icon Theme]\nName=HighContrast\nC"..., 4096) = 4096
read(6, "gories\nSize=256\nMinSize=56\nMaxSi"..., 4096) = 1538
read(6, "", 4096) = 0
close(6) = 0
openat(AT_FDCWD, "/usr/share//icons/Adwaita/index.theme", O_RDONLY) = 6
fstat(6, {st_mode=S_IFREG|0644, st_size=3424, ...}) = 0
read(6, "[Icon Theme]\nName=Adwaita\nCommen"..., 4096) = 3424
read(6, "", 4096) = 0
close(6) = 0
openat(AT_FDCWD, "/usr/share//icons/gnome-logo-text-dark.svg/index.theme", O_RDONLY) = -1 ENOTDIR (Not a directory)
openat(AT_FDCWD, "/usr/share//icons/gnome-logo-text.svg/index.theme", O_RDONLY) = -1 ENOTDIR (Not a directory)
openat(AT_FDCWD, "/usr/share//icons/breeze/index.theme", O_RDONLY) = 6
fstat(6, {st_mode=S_IFREG|0644, st_size=19288, ...}) = 0
read(6, "[Icon Theme]\nName=Breeze\nName[ar"..., 4096) = 4096
read(6, "ies=actions/16@2x,actions/16@3x,"..., 4096) = 4096
read(6, "ons\nType=Fixed\n\n#16x16@2x - Fixe"..., 4096) = 4096
read(6, "2x - Fixed size - For medium dev"..., 4096) = 4096
read(6, "Scale=3\nContext=Places\nType=Fixe"..., 4096) = 2904
read(6, "", 4096) = 0
close(6) = 0
openat(AT_FDCWD, "/usr/share//icons/breeze-dark/index.theme", O_RDONLY) = 6
fstat(6, {st_mode=S_IFREG|0644, st_size=19635, ...}) = 0
read(6, "[Icon Theme]\nName=Breeze Dark\nNa"..., 4096) = 4096
read(6, "rDefault=22\nToolbarSizes=16,22,3"..., 4096) = 4096
read(6, "16 - Fixed size - Application ic"..., 4096) = 4096
read(6, "ale=2\nContext=Devices\nType=Fixed"..., 4096) = 4096
read(6, "es/16]\nSize=16\nContext=Places\nTy"..., 4096) = 3251
read(6, "", 4096) = 0
close(6) = 0
openat(AT_FDCWD, "/usr/share//icons/locolor/index.theme", O_RDONLY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/share//icons/AdwaitaLegacy/index.theme", O_RDONLY) = 6
fstat(6, {st_mode=S_IFREG|0644, st_size=4045, ...}) = 0
read(6, "[Icon Theme]\nName=AdwaitaLegacy\n"..., 4096) = 4045
read(6, "", 4096) = 0
close(6) = 0
getdents64(5, 0x270f3750 /* 0 entries */, 32768) = 0
close(5) = 0
--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=NULL} ---
+++ killed by SIGSEGV (core dumped) +++
zsh: segmentation fault (core dumped) strace yambar

I got it to build and have the same segmentation fault as I had earlier parsing the themes. I am also on a Fedora box this time. Here is the tail of my syscall trace ``` openat(AT_FDCWD, "/usr/share//icons/oxygen/index.theme", O_RDONLY) = -1 ENOENT (No such file or directory) openat(AT_FDCWD, "/usr/share//icons/default/index.theme", O_RDONLY) = 6 fstat(6, {st_mode=S_IFREG|0644, st_size=30, ...}) = 0 read(6, "[Icon Theme]\nInherits=Adwaita\n", 4096) = 30 read(6, "", 4096) = 0 close(6) = 0 openat(AT_FDCWD, "/usr/share//icons/HighContrast/index.theme", O_RDONLY) = 6 fstat(6, {st_mode=S_IFREG|0644, st_size=5634, ...}) = 0 read(6, "[Icon Theme]\nName=HighContrast\nC"..., 4096) = 4096 read(6, "gories\nSize=256\nMinSize=56\nMaxSi"..., 4096) = 1538 read(6, "", 4096) = 0 close(6) = 0 openat(AT_FDCWD, "/usr/share//icons/Adwaita/index.theme", O_RDONLY) = 6 fstat(6, {st_mode=S_IFREG|0644, st_size=3424, ...}) = 0 read(6, "[Icon Theme]\nName=Adwaita\nCommen"..., 4096) = 3424 read(6, "", 4096) = 0 close(6) = 0 openat(AT_FDCWD, "/usr/share//icons/gnome-logo-text-dark.svg/index.theme", O_RDONLY) = -1 ENOTDIR (Not a directory) openat(AT_FDCWD, "/usr/share//icons/gnome-logo-text.svg/index.theme", O_RDONLY) = -1 ENOTDIR (Not a directory) openat(AT_FDCWD, "/usr/share//icons/breeze/index.theme", O_RDONLY) = 6 fstat(6, {st_mode=S_IFREG|0644, st_size=19288, ...}) = 0 read(6, "[Icon Theme]\nName=Breeze\nName[ar"..., 4096) = 4096 read(6, "ies=actions/16@2x,actions/16@3x,"..., 4096) = 4096 read(6, "ons\nType=Fixed\n\n#16x16@2x - Fixe"..., 4096) = 4096 read(6, "2x - Fixed size - For medium dev"..., 4096) = 4096 read(6, "Scale=3\nContext=Places\nType=Fixe"..., 4096) = 2904 read(6, "", 4096) = 0 close(6) = 0 openat(AT_FDCWD, "/usr/share//icons/breeze-dark/index.theme", O_RDONLY) = 6 fstat(6, {st_mode=S_IFREG|0644, st_size=19635, ...}) = 0 read(6, "[Icon Theme]\nName=Breeze Dark\nNa"..., 4096) = 4096 read(6, "rDefault=22\nToolbarSizes=16,22,3"..., 4096) = 4096 read(6, "16 - Fixed size - Application ic"..., 4096) = 4096 read(6, "ale=2\nContext=Devices\nType=Fixed"..., 4096) = 4096 read(6, "es/16]\nSize=16\nContext=Places\nTy"..., 4096) = 3251 read(6, "", 4096) = 0 close(6) = 0 openat(AT_FDCWD, "/usr/share//icons/locolor/index.theme", O_RDONLY) = -1 ENOENT (No such file or directory) openat(AT_FDCWD, "/usr/share//icons/AdwaitaLegacy/index.theme", O_RDONLY) = 6 fstat(6, {st_mode=S_IFREG|0644, st_size=4045, ...}) = 0 read(6, "[Icon Theme]\nName=AdwaitaLegacy\n"..., 4096) = 4045 read(6, "", 4096) = 0 close(6) = 0 getdents64(5, 0x270f3750 /* 0 entries */, 32768) = 0 close(5) = 0 --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=NULL} --- +++ killed by SIGSEGV (core dumped) +++ zsh: segmentation fault (core dumped) strace yambar ```

snowytrees commented 2024年07月24日 05:44:54 +02:00

Thanks for testing @bagnaram. Since I can't reproduce the segfault and nothing immediately stands out to me as the offending error, getting a copy of the index.theme file can help. Can you attach gdb to the coredump, go to the offending frame, and do p theme->dir. That should always be set. It will give the directory of the theme causing the segfault. Then could you attach the offending index.theme file so I can debug why the parser is failing.

If theme->dir == NULL or is uninitialized, I am somehow pushing an uninitialized theme into the theme list which is still useful information.

edit: Was able to reproduce by forcibly corrupting an index theme file. No need to follow the steps above. Working on figuring it out and will push a fix.

Thanks for testing @bagnaram. Since I can't reproduce the segfault and nothing immediately stands out to me as the offending error, getting a copy of the `index.theme` file can help. Can you attach gdb to the coredump, go to the offending frame, and do `p theme->dir`. That should always be set. It will give the directory of the theme causing the segfault. Then could you attach the offending `index.theme` file so I can debug why the parser is failing. If `theme->dir == NULL` or is uninitialized, I am somehow pushing an uninitialized theme into the theme list which is still useful information. edit: Was able to reproduce by forcibly corrupting an index theme file. No need to follow the steps above. Working on figuring it out and will push a fix.

snowytrees commented 2024年07月24日 06:12:51 +02:00

Found the second bug related to it. Part of the parser invariant is calling group_handler at the beginning & end in addition to between each group. When
porting the parser over, I missed calling group_handler at the end (see source
here: github.com/swaywm/sway@4d4c88f0a7/swaybar/tray/icon.c (L294)).

group_handler
[Group]
group_handler
[Group]
group_handler <-- this was never called

This would manifest the theme->name segfault with the following single group index file since Icon Theme was not being verified:

[Icon Theme]
<--- no icon theme name
Key=Val

Found the second bug related to it. Part of the parser invariant is calling `group_handler` at the beginning & end in addition to between each group. When porting the parser over, I missed calling group_handler at the end (see source here: https://github.com/swaywm/sway/blob/4d4c88f0a73f6ee3da1c99355f04362ef2ad68c9/swaybar/tray/icon.c#L294). ``` group_handler [Group] group_handler [Group] group_handler <-- this was never called ``` This would manifest the `theme->name` segfault with the following single group index file since `Icon Theme` was not being verified: ``` [Icon Theme] <--- no icon theme name Key=Val ```

dnkl commented 2024年07月24日 06:13:34 +02:00

@snowytrees just letting you know I've seen this. Due to the size of this PR, it'll likely be a while before I have time to review it. Please be patient, and thank you for implementing this :)

@snowytrees just letting you know I've seen this. Due to the size of this PR, it'll likely be a while before I have time to review it. Please be patient, and thank you for implementing this :)

snowytrees commented 2024年07月24日 06:14:19 +02:00

@dnkl No rush, I already left it sitting for 8 months :). Since I am switching back to yambar, just want it stable at least for myself and others.

@dnkl No rush, I already left it sitting for 8 months :). Since I am switching back to yambar, just want it stable at least for myself and others.

snowytrees commented 2024年07月24日 08:01:56 +02:00

Update:

• Removed check-has-icon-tag: true and replaced it with new map grammar:
  +{icon_name} now does a special icon tag existence check. So it can be
  combined with other condition checks. See example config for what it looks
  like in use. I wanted to do @{icon_name} but it broke the yaml parser :(

Update: - Removed `check-has-icon-tag: true` and replaced it with new map grammar: `+{icon_name}` now does a special icon tag existence check. So it can be combined with other condition checks. See example config for what it looks like in use. I wanted to do `@{icon_name}` but it broke the yaml parser :(

bagnaram commented 2024年07月24日 14:42:07 +02:00

I found the issue on my end. The default theme in /usr/share/icons/default/index.theme doesn't contain a name field because it inherits the theme (in my case Adwaita.

[Icon Theme]
Inherits=Adwaita

I verified your recent commits makes this issue irrelevant and now successfully launches!

I found the issue on my end. The default theme in `/usr/share/icons/default/index.theme` doesn't contain a name field because it inherits the theme (in my case `Adwaita`. ``` [Icon Theme] Inherits=Adwaita ``` I verified your recent commits makes this issue irrelevant and now successfully launches!

snowytrees commented 2024年07月24日 19:49:46 +02:00

@bagnaram Awesome! That index file is actually ill formed as according to the spec Name is required.

@bagnaram Awesome! That index file is actually ill formed as according to the spec `Name` is required.

nemo commented 2024年08月10日 17:05:53 +02:00

-1
Thank you for your hard work... Tray is indeed a sorely missing feature, but i don't think systemd dependency is a desirable course for the project. Relying purely on dbus would be more appropriate, and I'm afraid half-measures like this one will only delay an actual implementation.

-1 Thank you for your hard work... Tray is indeed a sorely missing feature, but i don't think systemd dependency is a desirable course for the project. Relying purely on dbus would be more appropriate, and I'm afraid half-measures like this one will only delay an actual implementation.

snowytrees commented 2024年08月10日 17:47:14 +02:00

-1
Thank you for your hard work... Tray is indeed a sorely missing feature, but i don't think systemd dependency is a desirable course for the project. Relying purely on dbus would be more appropriate, and I'm afraid half-measures like this one will only delay an actual implementation.

Before a drive by -1 and denigrating it as a half measure it would be nice if you did some research into your comment. This uses the sd-dbus library and that is it. That library is the C dbus client library. It is also an optional dependency if you don’t want the tray. There is no other way to implement this.

> -1 > Thank you for your hard work... Tray is indeed a sorely missing feature, but i don't think systemd dependency is a desirable course for the project. Relying purely on dbus would be more appropriate, and I'm afraid half-measures like this one will only delay an actual implementation. Before a drive by -1 and denigrating it as a half measure it would be nice if you did some research into your comment. This uses the sd-dbus library and that is it. That library is *the* C dbus client library. It is also an optional dependency if you don’t want the tray. There is no other way to implement this.

dnkl commented 2024年08月10日 18:08:45 +02:00

That's not quite right. sd-bus is systemd's implementation of the d-bus protocol. It's neither the first, the reference or the d-bus implementation.

libdbus is.

That said, sd-bus is, API wise, the better option. But @nemo is right - it would be better if we used libdbus, and thereby were systemd free.

But I don't think that's something we can require, given that no one else has implemented tray support. And this has been a feature request for a long time.

As such, I'm willing to accept sd-bus as a dependency, as long as the tray support is optional.

Any one who wants it to use plain old libdbus is free to contribute a patch.

That's not quite right. sd-bus is systemd's implementation of the d-bus protocol. It's neither the first, the reference or _the_ d-bus implementation. libdbus is. That said, sd-bus is, API wise, the better option. But @nemo is right - it would be _better_ if we used libdbus, and thereby were systemd free. But I don't think that's something we can require, given that no one else has implemented tray support. And this has been a feature request for a long time. As such, I'm willing to accept sd-bus as a dependency, as long as the tray support is optional. Any one who wants it to use plain old libdbus is free to contribute a patch.

snowytrees commented 2024年08月10日 18:18:06 +02:00

I stand corrected, thanks @dnkl . When researching dbus libraries everything pointed back to sd-dbus. The implication I got was that @nemo charging I didn’t implement this as a minimal dbus only implementation (which I spent a good amount of time working on).

I stand corrected, thanks @dnkl . When researching dbus libraries everything pointed back to sd-dbus. The implication I got was that @nemo charging I didn’t implement this as a minimal dbus only implementation (which I spent a good amount of time working on).

snowytrees commented 2024年08月10日 18:27:05 +02:00

If someone wants to go down this path, here is the website of alternative dbus client libraries (what I used when deciding how to build this). They explicitly recommended using sd-dbus over libdbus but to each their own. https://www.freedesktop.org/wiki/Software/DBusBindings/

If someone wants to go down this path, here is the website of alternative dbus client libraries (what I used when deciding how to build this). They explicitly recommended using sd-dbus over libdbus but to each their own. https://www.freedesktop.org/wiki/Software/DBusBindings/

dnkl commented 2024年08月10日 18:34:28 +02:00

They explicitly recommended using sd-dbus over libdbus but to each their own.

That's understandable. As I said, it's the better choice API wise. And since most distributions use systemd, sd-bus becomes the obvious choice.

But yambar, like all my applications, are minimalistic, and I try to use as few dependencies as possible, to make my applications accessible on as many distributions as possible. Including non-systemd ones.

That's why I think libdbus would be the better choice in this particular case.

But again, tray support with sd-bus is better than no tray support.

> They explicitly recommended using sd-dbus over libdbus but to each their own. That's understandable. As I said, it's the better choice API wise. And since most distributions use systemd, sd-bus becomes the obvious choice. But yambar, like all my applications, are minimalistic, and I try to use as few dependencies as possible, to make my applications accessible on as many distributions as possible. Including non-systemd ones. That's why I think libdbus would be the better choice in this particular case. But again, tray support with sd-bus is better than no tray support.

nemo commented 2024年08月10日 19:39:13 +02:00

Sorry, I didn't mean to be rude; you are right about my lack of research and I'm frustrated that I cannot assist you in any way other than pointing out the 'lesser evil'. Again, thank you for this contribution and I hope it helps someone but me.

Sorry, I didn't mean to be rude; you are right about my lack of research and I'm frustrated that I cannot assist you in any way other than pointing out the 'lesser evil'. Again, thank you for this contribution and I hope it helps someone but me.

nemo commented 2024年08月23日 14:09:27 +02:00

Hi again!
It seems like there was another option that I wasn't aware of.

Basu - https://git.sr.ht/~emersion/basu

The sd-bus library, extracted from systemd.

Some projects rely on the sd-bus library for DBus support. However not all systems have systemd or elogind installed. This library provides just sd-bus (and the busctl utility).

On top of that, elogind also provides sd-bus library.

So all you need to do is to add these patches:

diff --git a/modules/meson.build b/modules/meson.build
index 276e5e4..82febb4 100644
--- a/modules/meson.build
+++ b/modules/meson.build
@@ -6,8 +6,8 @@ modules = []
 alsa = dependency('alsa', required: get_option('plugin-alsa'))
 plugin_alsa_enabled = alsa.found()
 
-systemd = dependency('libsystemd', required: get_option('plugin-tray'))
-plugin_tray_enabled = systemd.found()
+sdbus_library = dependency('libsystemd', 'basu', 'libelogind', required: get_option('plugin-tray'))
+plugin_tray_enabled = sdbus_library.found()
 
 udev_backlight = dependency('libudev', required: get_option('plugin-backlight'))
 plugin_backlight_enabled = udev_backlight.found()
@@ -125,7 +125,8 @@ if plugin_sway_xkb_enabled
 endif
 
 if plugin_tray_enabled
- mod_data += {'tray': [[], [m, dynlist, systemd]]}
+ sdbus = declare_dependency(compile_args: ['-DHAVE_' + sdbus_library.name().to_upper()], dependencies:[sdbus_library])
+ mod_data += {'tray': [[], [m, dynlist, sdbus]]}
 endif
 
 if plugin_xkb_enabled

diff --git a/modules/tray.c b/modules/tray.c
index d831498..1f93221 100644
--- a/modules/tray.c
+++ b/modules/tray.c
@@ -8,10 +8,18 @@
 #include <stdio.h>
 #include <string.h>
 #include <sys/stat.h>
-#include <systemd/sd-bus.h>
+#if defined(HAVE_LIBSYSTEMD)
+#include <systemd/sd-bus.h>
+#elif defined(HAVE_BASU)
+#include <basu/sd-bus.h>
+#include <unistd.h>
+#elif defined(HAVE_LIBELOGIND)
+#include <elogind/sd-bus.h>
+#endif
 #include <tllist.h>
 #include <wordexp.h>
 
 #define LOG_MODULE "tray"
 #define LOG_ENABLE_DBG 0
 #include "../log.h"

Tested it on Void linux with basu-devel. It works!

Note:
I couldn't compile modules/tray.c with basu: getpid() was not found and I had to add unistd.
Was that perhaps a wrong call? (with systemd/elogind it is not required).

Hi again! It seems like there was another option that I wasn't aware of. > **Basu** - https://git.sr.ht/~emersion/basu > > The sd-bus library, extracted from systemd. > > Some projects rely on the sd-bus library for DBus support. However not all systems have systemd or elogind installed. This library provides just sd-bus (and the busctl utility). On top of that, elogind also provides sd-bus library. So all you need to do is to add these patches: ```diff diff --git a/modules/meson.build b/modules/meson.build index 276e5e4..82febb4 100644 --- a/modules/meson.build +++ b/modules/meson.build @@ -6,8 +6,8 @@ modules = [] alsa = dependency('alsa', required: get_option('plugin-alsa')) plugin_alsa_enabled = alsa.found() -systemd = dependency('libsystemd', required: get_option('plugin-tray')) -plugin_tray_enabled = systemd.found() +sdbus_library = dependency('libsystemd', 'basu', 'libelogind', required: get_option('plugin-tray')) +plugin_tray_enabled = sdbus_library.found() udev_backlight = dependency('libudev', required: get_option('plugin-backlight')) plugin_backlight_enabled = udev_backlight.found() @@ -125,7 +125,8 @@ if plugin_sway_xkb_enabled endif if plugin_tray_enabled - mod_data += {'tray': [[], [m, dynlist, systemd]]} + sdbus = declare_dependency(compile_args: ['-DHAVE_' + sdbus_library.name().to_upper()], dependencies:[sdbus_library]) + mod_data += {'tray': [[], [m, dynlist, sdbus]]} endif if plugin_xkb_enabled ``` ```diff diff --git a/modules/tray.c b/modules/tray.c index d831498..1f93221 100644 --- a/modules/tray.c +++ b/modules/tray.c @@ -8,10 +8,18 @@ #include <stdio.h> #include <string.h> #include <sys/stat.h> -#include <systemd/sd-bus.h> +#if defined(HAVE_LIBSYSTEMD) +#include <systemd/sd-bus.h> +#elif defined(HAVE_BASU) +#include <basu/sd-bus.h> +#include <unistd.h> +#elif defined(HAVE_LIBELOGIND) +#include <elogind/sd-bus.h> +#endif #include <tllist.h> #include <wordexp.h> #define LOG_MODULE "tray" #define LOG_ENABLE_DBG 0 #include "../log.h" ``` Tested it on Void linux with basu-devel. It works! _Note:_ _I couldn't compile `modules/tray.c` with basu: getpid() was not found and I had to add unistd. Was that perhaps a wrong call? (with systemd/elogind it is not required)._

nemo commented 2024年08月23日 14:15:46 +02:00

One more thing:
I was testing it out on arch and gcc-14 kept throwing errors at me.
This fix helped:

diff --git a/3rd-party/nanosvg/src/nanosvg.h b/3rd-party/nanosvg/src/nanosvg.h
index 60a3238..33ec1de 100644
--- a/3rd-party/nanosvg/src/nanosvg.h
+++ b/3rd-party/nanosvg/src/nanosvg.h
@@ -1653,7 +1653,7 @@ static int nsvg__parseRotate(float* xform, const char* str)
 
 static void nsvg__parseTransform(float* xform, const char* str)
 {
-	float t[6];
+	float t[6] = { 0 };
 	int len;
 	nsvg__xformIdentity(xform);
 	while (*str)

This is nanosvg issue - see [https://github.com/libsdl-org/SDL_image/issues/452].

One more thing: I was testing it out on arch and gcc-14 kept throwing errors at me. This fix helped: ```diff diff --git a/3rd-party/nanosvg/src/nanosvg.h b/3rd-party/nanosvg/src/nanosvg.h index 60a3238..33ec1de 100644 --- a/3rd-party/nanosvg/src/nanosvg.h +++ b/3rd-party/nanosvg/src/nanosvg.h @@ -1653,7 +1653,7 @@ static int nsvg__parseRotate(float* xform, const char* str) static void nsvg__parseTransform(float* xform, const char* str) { - float t[6]; + float t[6] = { 0 }; int len; nsvg__xformIdentity(xform); while (*str) ``` This is nanosvg issue - see [https://github.com/libsdl-org/SDL_image/issues/452].

dnkl commented 2024年09月05日 09:16:54 +02:00

I started looking at this PR, and while I think the overall design and architecture is sound, it's such a large change set that it's difficult to get a "feel" for it.

I think I've seen enough that I can say I'm prepared to merge this. Given that, could you write the documentation for all the new things? Having that will make it easier for me to review it.

I started looking at this PR, and while I _think_ the overall design and architecture is sound, it's such a large change set that it's difficult to get a "feel" for it. I think I've seen enough that I can say I'm prepared to merge this. Given that, could you write the documentation for all the new things? Having that will make it easier for me to review it.

snowytrees commented 2024年09月15日 09:56:43 +02:00

@dnkl I'll put up some documentation hopefully sometime this week. Ya its a large unwieldy change sorry about that :(. Do you want more code comments as well or just public docs. If more code comments a pointer to areas you feel need them would be appreciated.

@nemo Thanks for the build patch & the nanosvg diff! Glad to see you found a minimal alternative to the full systemd library :). I'll incorporate it into my diff when I work on the docs.

@dnkl I'll put up some documentation hopefully sometime this week. Ya its a large unwieldy change sorry about that :(. Do you want more code comments as well or just public docs. If more code comments a pointer to areas you feel need them would be appreciated. @nemo Thanks for the build patch & the nanosvg diff! Glad to see you found a minimal alternative to the full systemd library :). I'll incorporate it into my diff when I work on the docs.

dnkl commented 2024年10月02日 08:22:38 +02:00

In the MPRIS PR (#402), we're using sd-bus from libsystemd if available, and otherwise fall back to basu. I think we should do the same here (in meson, it'll of course be the same code once both MRs are merged).

In the MPRIS PR (https://codeberg.org/dnkl/yambar/pulls/402), we're using sd-bus from libsystemd if available, and otherwise fall back to basu. I think we should do the same here (in meson, it'll of course be the _same_ code once both MRs are merged).

Pontoporeia commented 2024年12月19日 11:12:08 +01:00

Any progress on this PR ?

Any progress on this PR ?

mathstuf commented 2026年05月21日 03:57:01 +02:00

I'm not sure this is something I'd use personally, so I'm not going to review it deeply in its current conflicting state. However, if someone would like to submit it to maybar, I'll take a look at it there.

I'm not sure this is something I'd use personally, so I'm not going to review it deeply in its current conflicting state. However, if someone would like to submit it to [`maybar`](https://codeberg.org/mathstuf/maybar), I'll take a look at it there.

This pull request has changes conflicting with the target branch.