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

Check pixbuf loader signals #76

Closed
wants to merge 2 commits into from
Closed

Conversation

caclark
Copy link
Contributor

@caclark caclark commented Dec 24, 2023

This test will detect the problem noted in #73.

This test will detect the problem noted in aruiz#73.
Copy link
Owner

@aruiz aruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but I'd like comments addressed before pushing.

g_error ("%s", error->message);
g_assert (error == NULL);

g_free(pixbuf_data);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use g_clear_pointer to set pixbuf_data to NULL

g_error ("%s", error->message);
g_assert (error == NULL);

g_object_unref(G_OBJECT(loader));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use g_clear_object


g_object_unref(G_OBJECT(loader));

g_strfreev (env);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use g_clear_pointer here too

@@ -0,0 +1,83 @@
#include <gdk-pixbuf/gdk-pixbuf.h>

gint area_prepared_cb_count = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please decleare a struct with these members, create a static instance in the main function and pass it as data to the callbacks.

Copy link
Owner

@aruiz aruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be the final changes before I squash and merge.

GError *error = NULL;
gsize pixbuf_size;
guchar *pixbuf_data;
SignalsCounters *counters = g_new0(SignalsCounters, 1);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually do SignalsCounter counters = { 0 }; and then pass it as &counters. No need to allocate from the heap.

g_assert_cmpint (counters->closed_cb_count, ==, 1);
g_assert_cmpint (counters->size_prepared_cb_count, ==, 1);

g_clear_pointer(&counters, g_free);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we use counters on the stack we don't need to free it anymore so we need to drop this line.

main (gint argc, gchar **argv)
{
gchar **env = g_get_environ ();
GdkPixbufLoader *loader;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of uninitialized pointers, please call gdk_pixbuf_loader_new() here or just declare the variable down below, we can use modern C patterns here.

GdkPixbufLoader *loader;
GError *error = NULL;
gsize pixbuf_size;
guchar *pixbuf_data;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set to NULL for safety

@aruiz aruiz closed this Feb 14, 2024
@aruiz
Copy link
Owner

aruiz commented Feb 14, 2024

I ammended this myself and pushed it to mainline. Thanks for working on this!

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