Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix trayIcons using custom iconTheme #86

Merged
merged 9 commits into from
Sep 10, 2023
Merged

fix trayIcons using custom iconTheme #86

merged 9 commits into from
Sep 10, 2023

Conversation

kotontrion
Copy link
Contributor

If an application uses a custom iconThemePath the icon could not be found and thereby the missing image icon was shown instead. This adds support for custom iconThemePaths.

@Aylur
Copy link
Owner

Aylur commented Sep 8, 2023

shouldn't this be moved to the proxy acquired callback? the path would be appended on every get icon call

@kotontrion
Copy link
Contributor Author

Yeah, you're right, i didn't think of that.
This fix just appends to the default path, which is used by every icon.

I think a better way to solve this is by adding an iconTheme property to the icon class. This would avoid messing with the default search paths.

I will be on vacation next week, but after that I could try to implement it this way.

@kotontrion
Copy link
Contributor Author

kotontrion commented Sep 9, 2023

added support for icon themes.
usage:

general:

Icon({iconThemePath: "/path/to/directory"})
Icon({iconThemePath: ["/path/to/directory1", "/path/to/directory2/"]})

systemtray:

const SystemTrayItem = item => Button({
  child: Icon({iconThemePath: item.iconThemePath}),
    onPrimaryClick: (_, event) => item.activate(event),
    onMiddleClick: (_, event) => item.secondaryActivate(event),
    onSecondaryClick: (_, event) => item.openMenu(event),
    onScrollUp: (_, event) => item.scroll(event),
    onScrollDown: (_, event) => item.scroll(event),
    connections: [[item, button => {
        button.child.icon = item.icon;
        button.tooltipMarkup = item.tooltipMarkup;
    }]],
})

@kotontrion kotontrion changed the title fixed missing tray icons for custom iconThemePaths support for theme paths for Icon widget Sep 9, 2023
@Aylur
Copy link
Owner

Aylur commented Sep 9, 2023

I am not sure if I like this, what would be the benefit over appending the path to the deafult IconTheme one?

@kotontrion
Copy link
Contributor Author

To make sure the correct icon is shown for the systemtray, the path should be prepended to the default iconTheme, to give the custom paths priority over icons installed in the system. But because changes to the default iconTheme are applied to all icons across the whole application, this could affect other unrelated icons. Even appending could lead to similar issues though unlikely.

IMO, the usage of iconThemePath of an tray icon should not have any affect on unrelated icons. This can't be guaranteed by modifying the default iconTheme.

If you don't like this approach another solution would be setting the iconTheme and loading the icon in the TrayIcon class, and don't change the icon class.
But then why shouldn't we give users access to this feature using the icon class?

@Aylur
Copy link
Owner

Aylur commented Sep 9, 2023

I would prefer hiding this from the user, so how about moving this to TrayItem?
Every TrayItem constructs a new Gtk.IconTheme and prepends proxy.IconThemePath if it exists, then in the icon getter we do this

// or using the default IconTheme if proxy.IconThemePath doesn't exist
const iconName = // icon name decided by status
const iconInfo = this._iconTheme.look_up_icon(iconName, 16, flags)

if (!iconInfo)
  // error which will never happen (probably)
  // return image-missing pixbuf
  
return iconInfo.load_icon() // always return a pixbuf and not an icon name

lets default to size 16, in gtk4 we wont need to specify a size so I wouldn't worry about it for now

@kotontrion kotontrion changed the title support for theme paths for Icon widget fix trayIcons using custom iconTheme Sep 10, 2023
@kotontrion
Copy link
Contributor Author

done

@Aylur Aylur merged commit 43ced1c into Aylur:main Sep 10, 2023
1 check passed
Aylur pushed a commit to AhmedSaadi0/ags that referenced this pull request Sep 10, 2023
Aylur pushed a commit that referenced this pull request Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants