GNOME Bugzilla – Bug 610141
gtk_assistant_get_nth_page() function fails to deliver last page when passed -1 as page number.
Last modified: 2010-02-23 16:25:50 UTC
Documentation on gtk_assistant_get_nth_page() states that if one passes -1 as page number, last page will be returned. Currently, passing -1 as page number will return NULL.
Created attachment 153928 [details] Simple test application that demonstrates the problem.
Created attachment 153929 [details] [review] Patch that fixes this problem.
I think you should specifically check for -1 here. After all, the docs also state that you'll get NULL for numbers which are out-of-bounds.
Created attachment 153936 [details] [review] Updated patch Updated according to recommendations from Matthias.
Just to explain why my first patch filtered all negative values and not only -1. If only -1 is filtered, it's still possible to pass negative value to g_list_nth(), which is bad, since this function expects unsigned integer. Luckily, negative signed integers, when passed to function that expects unsigned one, produce very large numbers, which are most of the time out of bounds, but I'm not sure if this is proper way of doing it. Second solution would be to filter page number on like this: page_num < -1 => return NULL page_num == -1 => return last page page_num > -1 && page_num < total_pages => return requested page page_num >= total_pages => return NULL
I would recommend adding a g_return_val_if_fail statement to check the out-of-bounds conditions.
Created attachment 153945 [details] [review] Added check for out-of-bounds situations.
My last patch is bad. g_return_val_if_fail (page_num < -1, NULL) should be g_return_val_if_fail (page_num >= -1, NULL). I'll re-create the patch if everything else is OK.
Actually, loooking at g_list_nth, it already returns NULL if the given position if out of bounds, so we may not need any extra checks, strictly speaking.
But no, it only takes a guint, so better to check explicitly. Looks alright then, with the fixes sense of the test.
Created attachment 153954 [details] [review] Reversed sense of g_return_val_if_failt() test that has been broken in last patch.
Comment on attachment 153954 [details] [review] Reversed sense of g_return_val_if_failt() test that has been broken in last patch. Looks good now, thanks. Please commit if you have access. Otherwise, I'll take care of it later.
I don't have access, so please commit this at your convenience.
> + g_return_val_if_fail (page_num >= -1, NULL); That is wrong according to the documentation: "The child widget, or NULL if page_num is out of bounds."
Would you rather replace this with with plain if clause: if (page_num < -1) return NULL;
Christian, it also says that it returns the last page for -1. So, what is wrong, in your opinion ?
I was thinking, if the documentation states 'out of bounds' returns NULL, I assume that < -1 shouldn't raise a critical. But on second thought, passing -42 and expecting to get NULL is probably not useful in any way. So I guess you should ignore my concern. Incidentally gtk_notebook_get_nth_page should then get a g_return_val_if_fail as well for < -1.