Crylia / crylia-theme

A theme for AwesomeWM
516 stars 26 forks source link

Better icon handler #15

Closed Shorakie closed 2 years ago

Shorakie commented 2 years ago

This PR aims to make the icon_handler better. This PR also formats the indents of some lua files.

I wrote the logic based on the implementation of the Albert Launcher. can check the source code here I tried testing it against my applications which i could get an icon almost all of the times. If you intend on merging this PR please test it against your applications as well, any feedbacks are welcome for improving it.

Crylia commented 2 years ago

First of all thanks for improving this, the function I wrote was horrible. I was only able to fly over it and I'm gonna need to take a bit more time to fully understand the code. One note for now, I see many io.open which is blocking Awesome. It should be possible to make it async using lgi but as long as there is no notable "lag" it shouldn't matter for now.

Shorakie commented 2 years ago

yeah i guess that can be a concern for low-end systems, all of the io.open are meant solely for checking the existance of files/directories.

ill lookup the lgi.

Crylia commented 2 years ago

Alright I'm ready for the first round and I found some minor problems.

  1. Alacritty (and some other programs) doesn't seem to get an icon when using icon_finder.from_client. This seems to be because of name:lower(), using only name it will work. Maybe only use name since I haven't found any application that does require lower().

  2. Arandr doesn't seem to work at all(uses default_icon). I don't think my handler does either and that might be because it uses an icon for a category or something like that? Either way its nothing to worry about and can be fixed later after this merged.

  3. Zoom has no icon at all (Not even the fallback one (not using the flatpak version))

  4. Steam Games have no icon, but that's expected since they work really weird. I might do something about it in the future.

  5. local default_icon = "/usr/share/icons/Papirus-Dark/128x128/apps/application-default-icon.svg" The default Icons should be either looked up based on the theme or something we include in the config.

Since I'm currently changing some file structures again, could you please put the LIP.lua inside src/lib/LIP/LIP.lua? This is to differentiate between my code and third party ones.


To break it down real quick

    • [ ] Needs a fix
    • [ ] Can be fixed but not needed for merge
    • [ ] Needs a fix
    • [ ] Something for the future
    • [x] Needs a fix

This is a rock solid PR and the code is looking really good. If you need any assistance please let me know, I'm happy to merge it once this is resolved :)

Shorakie commented 2 years ago

i fixed the 5th issue. the first 3 issues i guess we need to investigate more into it and the main problem is mapping a client to an icon, which can be done using its name, iclass, gclass, etc. for steam icons there are some apps like Steam icon fix or SIF in short, yet it is not complete. Im also open to any helps from you or anyother person on this PR.

one side note: i tried to rewrite the icon_handler based on the rofi launcher which was hard for me to read/understand due to it being written in c.

Crylia commented 2 years ago

The steam issue is something I'll do after I'm done with the notification-center I'm working on. It should be rather easy and since I already wrote a function to fetch album images from spotify's server it should be rather easy. https://steamdb.info/app/1245620/info/ As you can see its rather easy to get the icon we need.

I will look into 1 and 2 next week when I got more time for it.

problem is mapping a client to an icon, which can be done using its name, iclass, gclass, etc

I always used xprop WM_CLASS WM_NAME etc to get the clients name. Sadly as it seems to be the norm these days man applications violate xorg specs and do what they want so we need to check for the WM_CLASS WM_NAME etc until we get lucky.

one side note: i tried to rewrite the icon_handler based on the rofi launcher which was hard for me to read/understand due to it being written in c.

If you need any help with C just hit me up, I also had the idea to copy rofi, gnome or any launcher really but never got the time to read myself into it. But I'm willing to help where I can.

One side note, Zoom was a huge pain for me to get working since they decided to use uppercase and special character :roll_eyes: . If Zoom doesn't work, honestly fuck them at this point.

I also just remembered that when there is no icon at all, it might be a function crashing, might be related to the zoom naming issue

Crylia commented 2 years ago

https://github.com/sclu1034/lgi-async-extra

With this the blocking IO problem should be gone. I'm working on the steam icon fix and maybe on this after if you don't already.

Crylia commented 2 years ago

I'm also gonna drop this here https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html I will try get this working in lua so that we can do a proper xdg icon lookup.

Crylia commented 2 years ago

Alright I have some good and "bad" news. The first is that I figured out all the problems from above. I got steam games working and basically anything that uses xdg-icons (so everything lol). The bad new is that I had to rewrite everything for it to work according to the specs.

I will not commit the code into this PR since it has nothing to do with the old code.

The only "problem" that remains is that Its not possible (at least for my knowledge) to get the xdg-icon-name from any running applications or the .desktop file from that application(which would contain that information) so I'm still forced to do multiple lookup tries with lowercase, client.class, client.name etc.

I'm still gonna thank you for your work and will list you in the readme once I'm done with all the stuff that I want to merge into the main branch.

Crylia commented 2 years ago

I added the xdg based icon lookup in the develop branch. It probably takes a bit more optimization and I need to fix the inherit lookup. I'm gonna close this for now since it wont be needed/merged.