GNOME Bugzilla – Bug 764771
Label for page count with wrong width
Last modified: 2016-04-21 16:15:55 UTC
Created attachment 325577 [details] pdf with 2 pages for each page. Evince fails to calculate the right width for the label at the top left corner with the page count if the pdf has for each page 2 pages. In this case the label contains something like "24 (12 of 34)" where 24 is the current page number and 12 is the real page number (of the document). The problem is the label width is too short and I see "24 (12 of 3". The guilty seems to be the `ev_page_action_widget_update_max_width` function in *libmisc/ev-page-action-widget.c* In particular if you look at the function there is: ```c if (strcmp (max_page_label, max_page_numeric_label) != 0) { max_label = g_strdup_printf (_("(%d of %d)"), n_pages, n_pages); /* Do not take into account the parentheses for the size computation */ max_label_len = g_utf8_strlen (max_label, -1) - 2; } else { max_label = g_strdup_printf (_("of %d"), n_pages); max_label_len = g_utf8_strlen (max_label, -1); } ``` The `if` tries to distinguish the two cases but I put some prints and seems `max_page_label` and `max_page_numeric_label` have always the same value, so the `if` is useless. I tried to open different kind of document but they have always the same value. I didn't understand why this `if` is based on the `max_page_label`, but to fix this bug I think you need to find a better way to understand which kind of document evince has opened. A simple, but maybe not good, solution is: ```c gchar *label_text = gtk_entry_get_text(GTK_ENTRY (action_widget->label)); if ('(' == label_text[0]) { free(label_text); ``` In this example I used the text inside the label to understand which case is. I don't submit this as a patch, because this solution doesn't seem enough good and I don't know if other languages has '(' character. I attach a pdf as example. The pdf is empty, because I removed the content to avoid copyright problems, but each blank page is counted as double.
I tried to use markdown syntax but I see isn't supported, I'm sorry I didn't know
Created attachment 325578 [details] Screen of the label too short
Ok, I understood the logic. I report here the whole function: static void ev_page_action_widget_update_max_width (EvPageActionWidget *action_widget) { gchar *max_label; gint n_pages; gint max_label_len; gchar *max_page_label; gchar *max_page_numeric_label; n_pages = ev_document_get_n_pages (action_widget->document); max_page_label = ev_document_get_page_label (action_widget->document, n_pages - 1); max_page_numeric_label = g_strdup_printf ("%d", n_pages); if (strcmp (max_page_label, max_page_numeric_label) != 0) { max_label = g_strdup_printf (_("(%d of %d)"), n_pages, n_pages); /* Do not take into account the parentheses for the size computation */ max_label_len = g_utf8_strlen (max_label, -1) - 2; } else { max_label = g_strdup_printf (_("of %d"), n_pages); max_label_len = g_utf8_strlen (max_label, -1); } g_free (max_page_label); gtk_entry_set_width_chars (GTK_ENTRY (action_widget->label), max_label_len); g_free (max_label); max_label_len = ev_document_get_max_label_len (action_widget->document); gtk_entry_set_width_chars (GTK_ENTRY (action_widget->entry), CLAMP (max_label_len, strlen (max_page_numeric_label) + 1, 12)); g_free (max_page_numeric_label); } Ok, the 'if' tries to check if there is a difference between the label number displayed and the real number page for a known page (in this case is the last). Suppose to have a document of 12 pages, where each page is double. At the last page you have "24 (12 of 12)". The ev_document_get_n_pages function returns the number of pages in the document (i.e. 12). This value will be converted into a string and will be stored into max_page_numeric_label. The ev_document_get_page_label function returns the page number for a given index (n_pages - 1) as a string. So max_page_label will contain 24 for this example. Finally the strcmp checks if the two numbers (as strings) are equals or not. This function seems to work correctly. When I open the casetest.pdf with evince at the last page I see "9 of 9", while I'm expecting something like "18 (9 of 9)". This is why this function fails. Maybe this is a bug. There is also the possibility to have a document with some pages that contain 2 pages per page and other pages with just 1 page per page. I think also this kind of document could lead problems, but I can't imagine a document with this structure except for mistakes.
I see also the page count is wrong, I'm confused.. maybe the pdf is malformed, or the bug is more serious.
Ok, seems this pdf is malformed because also with others pdf viewer I got a wrong page count, but we can ignore this fact for the moment. My final consideration is the following. At the moment ev_page_action_widget_update_max_width tries to calculate the right width to give to the widget just the first time you open a document, but sometimes this function can fail. If you have a pdf with 2 pages per page and with the last page that contains only 1 page, the function will calculate the right width analyzing the last page only and it will fail. I think it's wrong trying to calculate the right width a priori, but it's better resize the width of the label dinamically every time its text changes.
Created attachment 325595 [details] [review] Patch to autoresize the width of the label with the page count every time its text changes
(In reply to devim from comment #5) > Ok, seems this pdf is malformed because also with others pdf viewer I got a > wrong page count, but we can ignore this fact for the moment. My final > consideration is the following. At the moment > ev_page_action_widget_update_max_width tries to calculate the right width to > give to the widget just the first time you open a document, but sometimes > this function can fail. If you have a pdf with 2 pages per page and with the > last page that contains only 1 page, the function will calculate the right > width analyzing the last page only and it will fail. > > I think it's wrong trying to calculate the right width a priori, but it's > better resize the width of the label dinamically every time its text changes. We try to get the maximum size the first time precisely to avoid resizes, since it looks weird and affects the position of other items in the toolbar.
Ok, but your calculation is based on the last page only. In my opinion this is wrong, infact with the pdf I've attached Evince fails to find the right width. I think it's hard to compute the maxmimum size just the first time. Also if you consider other pages (like the first, the last and one page in the middle) there is always the risk to fail because some page could be double (this will require a larger label). Maybe another solution could be iterate over all the pages of the document to calculate the maximum size. Using this approach you should be able to make the right calculus. If you think this solution is enough good, I can try to make a new patch.
Created attachment 325763 [details] [review] Patch to force Evince to calculate the right width iterating over all the pages.
you didn't upload a patch
Created attachment 325793 [details] [review] Patch (reuploaded) to force Evince to calculate the right width iterating over all the pages I'm sorry, I did a mistake.
This is a very weird example, the document uses page labels that don't make any sense. We check if we need to add the (n of M) when page labels don't match the numeric page, assuming that if it doesn't match for the last page it doesn't for all others and the the last one is always the longest. In this particular example, the only page where page label matches the numeric label is the last one, making our assumption wrong, but I'm not sure how realistic the example is, to be honest. If we really want to support this corner case, I would do this check in ev-document.c when the document is loaded, because it happens in a different thread and we already iterate and get the page labels, see ev_document_setup_cache. We do that to cache the page labels and calculate the max label length, that we use for the non numeric part of the page action widget. If all page labels match the numeric labels, then we don't event need to cache the labels, so we could free the page_labels array and set it to NULL so that ev_document_has_text_page_labels will return FALSE. And we could use that to decide how to calculate the max label length. If the document doesn't have text labels we can assume the (n of M) is not needed.
I found this issue with some slides (exported into pdf format) of a course of my university. This is a corner case, but it's enough annoying to not see how much pages the document has (because the label is too short). Since the solution shouldn't require too much time to be implemented and applied I suggest to consider to patch this issue.
(In reply to devim from comment #13) > I found this issue with some slides (exported into pdf format) of a course > of my university. This is a corner case, but it's enough annoying to not see > how much pages the document has (because the label is too short). Since the > solution shouldn't require too much time to be implemented and applied I > suggest to consider to patch this issue. I'm not opposed to fixing it, but not like the current patch does, iterating again all pages in the main thread.
Created attachment 326307 [details] [review] Path to manage properly documents with custom page labels calculating the right width for the label with the page count I tried to follow your suggestions, let me know what do you think.
I used a wrong indentation, but before to fix this issue I prefer to wait to know if there are other changes to do.
Review of attachment 326307 [details] [review]: Looks good to me, thanks! ::: libdocument/ev-document.c @@ +265,3 @@ priv->page_labels = g_new0 (gchar *, priv->n_pages); + + real_page_label = g_strdup_printf ("%d", i + 1); You are leaking this, it should be freed after the comparison. @@ +266,3 @@ + + real_page_label = g_strdup_printf ("%d", i + 1); + custom_page_labels = custom_page_labels || g_strcmp0 (real_page_label, page_label) != 0; If we know there are custom page labels already we can skip the memory allocation too, not only the strcmp. So, I would use an if like: if (!custom_page_labels) { gchar *real_page_label; real_page_label = g_strdup_printf ("%d", i + 1); custom_page_labels = g_strcmp0 (real_page_label, page_label) != 0; g_free (real_page_label); }
Created attachment 326349 [details] [review] [Updated] Patch to manage properly documents with custom page labels calculating the right width for the label with the page count You're right, I edited the patch following your tips. Thank you too for your support and your work :)
For priv->info = _ev_document_get_info (document); I fixed the indentation.
Created attachment 326366 [details] [review] [Updated 2] Patch to manage properly documents with custom page labels calculating the right width for the label with the page count I fixed a memory leak due to an incorrect free of the page_labels array.
(In reply to devim from comment #20) > Created attachment 326366 [details] [review] [review] > [Updated 2] Patch to manage properly documents with custom page labels > calculating the right width for the label with the page count > > I fixed a memory leak due to an incorrect free of the page_labels array. Right!, a would way to avoid this in the future could be to add a helper method to free the page labels instead of duplicating the code.
Created attachment 326471 [details] [review] [Updated 3] Patch to manage properly documents with custom page labels calculating the right width for the label with the page count Ok, I improved the quality of code using the g_strfreev built-in function. This function could be used also in other areas of the code (e.g. in ev_document_finalize), but this isn't the goal of the current patch. I think now the patch is good.
Review of attachment 326471 [details] [review]: I've pushed the patch with my comments fixed and some trailing whitespace issues fixed too. Thanks! ::: libdocument/ev-document.c @@ +281,3 @@ + + /* g_strfreev requires a NULL-terminated array */ + priv->page_labels[i] = NULL; This is not really needed, since g_new0 already set this to NULL. @@ +285,3 @@ + if (!custom_page_labels) { + g_strfreev (priv->page_labels); + priv->page_labels = NULL; g_clear_pointer() does this for you.
(In reply to devim from comment #22) > Created attachment 326471 [details] [review] [review] > [Updated 3] Patch to manage properly documents with custom page labels > calculating the right width for the label with the page count > > Ok, I improved the quality of code using the g_strfreev built-in function. > This function could be used also in other areas of the code (e.g. in > ev_document_finalize), but this isn't the goal of the current patch. I think > now the patch is good. I disagree, if you are changing the way it's allocated in this patch you should also change the way it's freed. Already done it before pushing.
I saw, thank you for your help :)