Summary

Add icon-name, icon-search-path, and icon-resource-path to PurpleProtocol

Review Request #790 — Created July 8, 2021 and submitted Sept. 3, 2021, 2:10 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

Depends On

Commit

9713338010e9

Reviewers

Groups

pidgin

People

Description

These are now used by pidgin_create_icon_from_protocol which will attempt to
load the icon for the protocol from the default GtkIconTheme if the protocol
has a non NULL value for icon-name.

pidgin_ui_init was also updated to add icon-search-path and icon-resource-path
to the default GtkIconTheme when a protocol is registered. It also scans all of
the protocols that have been already been registered as that happens before the
ui is initialized.

Testing Done

Removed old local icon and then ran with an inverted icon to make sure pidgin was displaying the icon from the resource.

Issues

Description	From	Last Updated
Maybe it should return a GIcon (implemented as a GLoadableIcon or something), that can return multiple sizes?	QuLogic	Sept. 2, 2021, 7:40 a.m.
This is not a gresource?	QuLogic	Sept. 3, 2021, 1:50 a.m.
embedded	QuLogic	Sept. 3, 2021, 1:51 a.m.
Dashes instead of underscore?	QuLogic	Sept. 3, 2021, 1:51 a.m.
Ditto.	QuLogic	Sept. 3, 2021, 1:51 a.m.
Ditto.	QuLogic	Sept. 3, 2021, 1:51 a.m.
Ditto.	QuLogic	Sept. 3, 2021, 1:51 a.m.
its	QuLogic	Sept. 3, 2021, 1:51 a.m.

Ship it!

```
Ship It!
```

Admittedly I did not review this super in-depth, but I didn't see anything that jumped out at me in a cursory review.

Show all issues

Maybe it should return a GIcon (implemented as a GLoadableIcon or something), that can return multiple sizes?

grim July 23, 2021, 9:01 a.m.

We could just make it a GIcon and then the prpls can decide what they want to do. I assume most would use embeded resources to create the icon via g_bytes_icon_new and then gtk can resize it and stuff, although I'm not sure how difficult it'll be to use that in another ui, but it looks like that's where GLoadableIcon comes in so maybe that's the way to go?

grim July 24, 2021, 11:30 p.m.

Part of my thinking was to pass a size enum or something along so that protocols who's icon's don't scale well for smaller sizes could use another icon or something.

QuLogic July 26, 2021, 5:45 a.m.

Right, that's what the GLoadableIcon interface provides.

grim July 26, 2021, 6:16 a.m.

After digging into that a bit more it looks like protocol plugins would need to implement that to control file for the sizes. However, that also means a whole bunch of resource shennigans...

However, we could create a PurpleIcon that would take a GResource and a path prefix to off load all of that. I'd opt for the PurpleIcon name so it can be reused for plugins and stuff too.

Thoughts?

grim Aug. 7, 2021, 3:22 a.m.

I just submitted RR 885 and realized that if we leave the icon name and instead of loading the file from disk, we can just look it up in the icon theme that the protocol plugins can provide via their resources. I'll update this RR to do that now.

grim Aug. 7, 2021, 3:28 a.m.

After some more thought, we'll need to have the protocol plugin export the resource path that we'll pass into gtk_icon_theme_add_resource_path as well as the name. Then, Pidgin will need to connect to the ProtocolManager's registered signal to add the resource path from the protocol via gtk_icon_theme_add_resource_path. However, there is no way to unregister an icon path, which should be okay as an unloaded plugin is still in memory and not actually closed.

Change Summary:

Updated to work with the gtk_icon_theme_api

Summary:

-	Add an icon method to PurpleProtocolClass and port irc to it
+	Add icon-name, icon-search-path, and icon-resource-path to PurpleProtocol

Description:

~		This updates the pidgin_create_icon_from_protocol to check the new method first and then fall back to the existing list_icon method.
	~	These are now used by pidgin_create_icon_from_protocol which will attempt to
	+	load the icon for the protocol from the default GtkIconTheme if the protocol
	+	has a non NULL value for icon-name.

~		I opted for a 128x128px icon just kind of on a whim but figured higher resolution is better. We could pass a size parameter to icon function and then avoid some scaling, as the protocol could specify different resources, but then that means a new enum in libpurple that's very ui specific, but maybe that's fine.
~
~		I didn't move the svg because I removed some additional shadows that were in the original svg. If we want I can go back and do the remove but it was just easier for me at the moment.
	~	pidgin_ui_init was also updated to add icon-search-path and icon-resource-path
	~	to the default GtkIconTheme when a protocol is registered. It also scans all of
	~	the protocols that have been already been registered as that happens before the
	+	ui is initialized.

Testing Done:

~		Ran locally with an inverted icon to make sure pidgin was displaying the icon from the resource.
	~	Removed old local icon and then ran with an inverted icon to make sure pidgin was displaying the icon from the resource.

Commit:

-	c9a3f984cf3c
+	28468ab07ff9

Diff:

Revision 2 (+309 -5)

Show changes

	libpurple/protocol.h
	libpurple/protocol.c
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/meson.build
	libpurple/protocols/irc/resources/irc.gresource.xml
	pidgin/data/icons/hicolor/16x16/apps/im-irc.png
	pidgin/data/icons/hicolor/16x16/apps/scalable/im-irc.svg
	pidgin/data/icons/hicolor/22x22/apps/im-irc.png
	5 more

libpurple/protocol.h (Diff revision 2)

Show all issues

This is not a gresource?

grim Sept. 2, 2021, 7:47 a.m.

Yeah copy paste error. The icon_search_path support is for protocols that aren't using glib-compile-resource.

libpurple/protocol.h (Diff revision 2)
Show all issues
```
embedded
```
libpurple/protocol.c (Diff revision 2)
Show all issues
```
Dashes instead of underscore?
```
libpurple/protocol.c (Diff revision 2)
Show all issues
```
Ditto.
```
libpurple/protocol.c (Diff revision 2)
Show all issues
```
Ditto.
```
libpurple/protocol.c (Diff revision 2)
Show all issues
```
Ditto.
```
pidgin/libpidgin.c (Diff revision 2)
Show all issues
```
its
```

Commit:

-	28468ab07ff9
+	4e7a90c391af

Diff:

Revision 3 (+309 -5)

Show changes

	libpurple/protocol.h
	libpurple/protocol.c
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/meson.build
	libpurple/protocols/irc/resources/irc.gresource.xml
	pidgin/data/icons/hicolor/16x16/apps/im-irc.png
	pidgin/data/icons/hicolor/16x16/apps/scalable/im-irc.svg
	pidgin/data/icons/hicolor/22x22/apps/im-irc.png
	5 more

Ship it!

```
Ship It!
```

Change Summary:

Make the docs actually build

Commit:

-	4e7a90c391af
+	9713338010e9

Diff:

Revision 4 (+309 -5)

Show changes

	libpurple/protocol.h
	libpurple/protocol.c
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/meson.build
	libpurple/protocols/irc/resources/irc.gresource.xml
	pidgin/data/icons/hicolor/16x16/apps/im-irc.png
	pidgin/data/icons/hicolor/16x16/apps/scalable/im-irc.svg
	pidgin/data/icons/hicolor/22x22/apps/im-irc.png
	5 more

Review Board 6.0.2

Add icon-name, icon-search-path, and icon-resource-path to PurpleProtocol

Information

Reviewers

Change Summary:

Summary:

Description:

Testing Done:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)