The new icontheme API needs improvement
And because I don't want to be too picky, I'll review all of the API.
typedef enum
{
GTK_ICON_LOOKUP_NO_SVG = 1 << 0,
GTK_ICON_LOOKUP_FORCE_SVG = 1 << 1,
GTK_ICON_LOOKUP_USE_BUILTIN = 1 << 2,
GTK_ICON_LOOKUP_GENERIC_FALLBACK = 1 << 3,
GTK_ICON_LOOKUP_FORCE_SIZE = 1 << 4,
GTK_ICON_LOOKUP_FORCE_REGULAR = 1 << 5,
GTK_ICON_LOOKUP_FORCE_SYMBOLIC = 1 << 6,
GTK_ICON_LOOKUP_DIR_LTR = 1 << 7,
GTK_ICON_LOOKUP_DIR_RTL = 1 << 8
} GtkIconLookupFlags;GTK_ICON_LOOKUP_NO_SVG and GTK_ICON_LOOKUP_FORCE_SVG should go away. GtkIconTheme loads icons how it sees fit.
GTK_ICON_LOOKUP_USE_BUILTIN should go away. It's not even used anymore by the icon theme, but if it was, GTK should always use builtins as a last resort.
GTK_ICON_LOOKUP_GENERIC_FALLBACK should go away. Getting generic fallbacks can be done via gtk_icon_theme_lookup_icon() (where it should always be used) and not getting generic fallbacks can be done via gtk_icon_theme_lookup_icon_with_fallback() (see below).
GTK_ICON_LOOKUP_FORCE_SIZE should be the default. If code loads an icon at 37x37, it should get an icon at 37x37. It's up to the icontheme what it renders in that case.
If developers want a different size, they can use gtk_icon_theme_get_icon_sizes() and apply their chosen scale themselves.
The other options are fine with me.
And see below for the potential addition of GTK_ICON_LOOKUP_FORCE_TEXTURE.
GDK_AVAILABLE_IN_ALL
GtkIconTheme *gtk_icon_theme_new (void);
GDK_AVAILABLE_IN_ALL
GtkIconTheme *gtk_icon_theme_get_for_display (GdkDisplay *display);
GDK_AVAILABLE_IN_ALL
gboolean gtk_icon_theme_has_icon (GtkIconTheme *self,
const gchar *icon_name);
GDK_AVAILABLE_IN_ALL
gint *gtk_icon_theme_get_icon_sizes (GtkIconTheme *self,
const gchar *icon_name);I have no complaints about these APIs.
GDK_AVAILABLE_IN_ALL
GtkIconTheme *gtk_icon_theme_get_default (void);This one should go away.
People use it as a shortcut for the next one and then they just end up using the wrong display.
In fact, the only user inside GTK does this wrong.
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_set_display (GtkIconTheme *self,
GdkDisplay *display);This should be private.
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_set_search_path (GtkIconTheme *self,
const gchar *path[],
gint n_elements);
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_get_search_path (GtkIconTheme *self,
gchar **path[],
gint *n_elements);This should be a G_TYPE_STRV property and the API should mirror that and operate with null-terminated char**.
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_append_search_path (GtkIconTheme *self,
const gchar *path);
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_prepend_search_path (GtkIconTheme *self,
const gchar *path);I would drop this in favor of the API below.
But if we keep it, it should be implemented by calling gtk_icon_theme_set_search_path().
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_add_resource_path (GtkIconTheme *self,
const gchar *path);This should also have a G_TYPE_STRV property.
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_set_custom_theme (GtkIconTheme *self,
const gchar *theme_name);I'd replace this with a char * GtkIconTheme::theme-name property with corresponding getters and setters, defaulting to "hicolor" or "gnome" or whatever the current default is.
The display's theme should report the same value as the GtkSettings object.
GDK_AVAILABLE_IN_ALL
GtkIcon * gtk_icon_theme_lookup_icon (GtkIconTheme *self,
const gchar *icon_name,
gint size,
gint scale,
GtkIconLookupFlags flags);
GDK_AVAILABLE_IN_ALL
GtkIcon * gtk_icon_theme_choose_icon (GtkIconTheme *self,
const gchar *icon_names[],
gint size,
gint scale,
GtkIconLookupFlags flags);
GDK_AVAILABLE_IN_ALL
GtkIcon * gtk_icon_theme_lookup_by_gicon (GtkIconTheme *self,
GIcon *icon,
gint size,
gint scale,
GtkIconLookupFlags flags);There are way too many APIs to pick icons and it is unclear to me which one people should use. The use of different verbs for the same operation also doesn't help.
I would drop the GIcon version - turning GIcons into paintables is not the icon theme's job. It's only the icon theme's job if the icon is indeed a GThemedIcon, but that's a special case that should be handled by other code. (I'll elaborate on this more below).
I would also rename gtk_icon_theme_choose_icon() to gtk_icon_theme_lookup_icon_with_fallbacks().
And this API needs to properly document how the lookup for fallbacks is happening. With this new addition of scale, we can now fall back to
-
size
-
scale
-
name
-
theme
-
symbolicness
-
direction
And it is unclear to me which icon will be chosen if 6 different ones exist that match in 5 categories each but not the 6th - or 30 icons exist that match in 4 categories each but not in the 2 others.
I will again note very loudly that I think adding the new scale dimension is the wrong thing to do because people weren't even able to properly understand icon lookup in a 5-dimensional world or create all the necessary icons for it, and adding more dimensions will only make it worse.
GDK_AVAILABLE_IN_ALL
void gtk_icon_theme_choose_icon_async (GtkIconTheme *self,
const gchar *icon_names[],
gint size,
gint scale,
GtkIconLookupFlags flags,
int priority,
GCancellable *cancellable,
GAsyncReadyCallback callback,
gpointer user_data);
GDK_AVAILABLE_IN_ALL
GtkIcon * gtk_icon_theme_choose_icon_finish (GtkIconTheme *self,
GAsyncResult *result,
GError **error);Async lookups should go away. People will just use them wrong.
The only user inside GTK already uses them wrong.
Instead, the sync version should always do an async load and provide a way to enforce the load.
This also avoid the need for a custom GError and allows knowing if an icon exists sync (because of the NULL return) while still doing the actual load async.
GDK_AVAILABLE_IN_ALL
GList * gtk_icon_theme_list_icons (GtkIconTheme *self,
const gchar *context);This should be a G_TYPE_STRV read-only property and then this should be const char ** gtk_icon_theme_get_icons().
GDK_AVAILABLE_IN_ALL
gboolean gtk_icon_theme_rescan_if_needed (GtkIconTheme *self);This needs to go away. If rescans are needed the icon theme should know that and just rescan. If rescans are not needed, this function doesn't need to be called.
GtkIcon
I don't like the name GtkIcon because now when I see the term "icon" in GTK's chat, API documentation or here in gitlab, I always have to wonder if they mean icon-name, GtkIcon, GIcon or the file loaded for them.
However, I have no better solution, so I can't suggest one.
If we keep the name "icon" for this thing, the API docs should be reviewed so that we are sure that whenever we use the term, it refers to the same thing, and they otherwise use the term GtkIcon, GIcon or "icon name".
Finally, GtkIcon should go into its own file and its own documentation section, because otherwise people will mix things up about it.
GDK_AVAILABLE_IN_ALL
gint gtk_icon_get_base_size (GtkIcon *self);
GDK_AVAILABLE_IN_ALL
gint gtk_icon_get_base_scale (GtkIcon *self);Remove this. Nobody knows what this is.
GDK_AVAILABLE_IN_ALL
const gchar * gtk_icon_get_filename (GtkIcon *self);I'd remove this. Icons don't even have filenames when they are loaded from a resource.
If anyone wants to keep this, it should be a GFile GtkIcon::file property.
GDK_AVAILABLE_IN_ALL
gboolean gtk_icon_is_symbolic (GtkIcon *self);Needs a property. Alternatively it could be removed, but it might be useful.
GDK_AVAILABLE_IN_ALL
GdkTexture * gtk_icon_download_texture (GtkIcon *self,
GError **error);
GDK_AVAILABLE_IN_ALL
GdkTexture * gtk_icon_download_colored_texture (GtkIcon *self,
const GdkRGBA *foreground_color,
const GdkRGBA *success_color,
const GdkRGBA *warning_color,
const GdkRGBA *error_color,
GError **error);
I don't want icons to be textures. We might load SVGs directly or support animations at some point, and then this API won't work.
I also do not like the GError there - none of the callers inside GTK even check that error, so I think it's equally likely that nobody else will. And then if we indeed stop returning textures, those apps will all start crashing.
So converting an icon into a texture should require more code. Making people create a snapshot and rendering it there or having them lookup the filename and load the file themselves seem like the right amount of work to me.
The one alternative that could be useful is having a GTK_ICON_LOOKUP_FORCE_TEXTURE flag that ensures that a given icon will not be returned unless GTK can guarantee that it will return a texture and only support the download_texture() call in that case. But I would insist that that only works for non-symbolic icons, because symbolic icons aren't textures, they're masks and colors.
GDK_AVAILABLE_IN_ALL
void gtk_icon_snapshot_with_colors (GtkIcon *icon,
GtkSnapshot *snapshot,
double width,
double height,
const GdkRGBA *foreground_color,
const GdkRGBA *success_color,
const GdkRGBA *warning_color,
const GdkRGBA *error_color);I don't like this as public API, because it has 8 arguments and nobody knows where to get them from.
missing APIs
GDK_AVAILABLE_IN_ALL
GtkIcon * gtk_icon_new_for_file (GFile *file,
int size,
int scale);This should be public API (the function exists internally). People can then use it to load a GFileIcon or any other icon they'd like to explicitly use. gtk_image_set_from_file() should use this, too.
Once called, it should immediately start an async load, just like the creation of any other GtkIcon does. We could then even ponder using this to ensure icons get loaded async and at the right scale when using <object class="GtkImage"><property name="???">/path/to/icon.svg</property></object> in GtkBuilder.
GDK_AVAILABLE_IN_ALL
GtkIcon * gtk_icon_new_for_gicon (GIcon *icon,
GtkIconTheme *icon_theme,
int size,
int scale);I don't think we need this API.
But if people think we want to support GIcons more, this is how I would do it.
final thoughts
I don't think the public API should be aware of icons being loaded async. They just are (or aren't) and it happens behind the developers' back. This is how GTK's APIs work (all GDK calls to Wayland are async after all) and I don't think any widget cares.
Also, I would expect icon loads to never fail. Icons are part of the system either via the icon theme or via resources and as such it's something that a developer can assume will never fail. Even if it does, a developer cannot do anything about it anyway.
Of course, if an async load fails, GTK should not crash. GTK is able to do something about it and can just load the next fallback icon - or in the worst case just display the missing-icon symbol.