GNOME Bugzilla – Bug 787070
PDF Annotation can't be clicked when overlapping
Last modified: 2018-03-31 01:45:30 UTC
Hi all, When I use evince to open a PDF that I annotated with the Acrobat Reader App (iPad), I am not able to read all the annotations in evince afterwards. I debugged this problem and it looks like that evince is not sorting the annotations that overlap correctly. In my specific case, I created a highlight annotation and a popup annotation (not 100% sure about the name) on top of it on my iPad. In evince the highlight annotation is then put above the popup annotation and leaves the user no way to show the contents of the popup annotation.
Created attachment 358845 [details] [review] Proposed fix by ordering the annotations by bounding box area size
Created attachment 358858 [details] Faulty PDF Created a PDF that I annotated on my iPad in the way I did it on the original one...
Review of attachment 358845 [details] [review]: Looks good for a first pass. Please also set proper authorship (that's the From: in the commit message), and add a prefix to the commit subject (do a "git log" on the file you modified to see what was used in the past) ::: libdocument/ev-mapping-list.c @@ +117,3 @@ GList *list; + g_return_val_if_fail (mapping_list != NULL, NULL); This line changed for no reason. @@ +119,3 @@ + g_return_val_if_fail (mapping_list != NULL, NULL); + + EvMapping *found = NULL; Declarations should be at the top of the block (the top of the function in this case). @@ +128,3 @@ (y <= mapping->area.y2)) { + + if(!found) { No need for braces for a one-line conditional. @@ +135,3 @@ + * In this way we allow most of the elements to be clickable + * by the user */ + if(found && (u_int64_t)(found->area.x2 - found->area.x1) * Please split this up in multiple calls, it's unreadable. You'll also want to use guint64, rather that stdint types, and possibly g_uint64_checked_mul() to do your multiplications (an overflow would probably have unintended consequences).
Created attachment 359422 [details] [review] Updated Patch Fixing the Problem I reworked my patch now. I agree that the second if-statement was quite unreadable, I moved the area size calculation now into a static function. After that I was able to collapse both if-statements into one. Apparently the indentation of the g_return_... line was wrong beforehand, checked how it is indented now and it looks good (but creates this nasty extra line in the diff). Hopefully it's more readable for you now, feel free to make further improvement suggestions!
Just as additional information: I also tested the "Faulty PDF" with Okular, Foxit Reader and the embedded PDF reader in Firefox and all of them got this right. It seems that only evince has this problem...
Review of attachment 359422 [details] [review]: This looks good to me too. I have only a couple of minor comments ::: libdocument/ev-mapping-list.c @@ +101,3 @@ +static guint64 +get_mapping_area (EvMapping *mapping) We use area in several places in the code to refer to the rectangle, so here I would use a different name, maybe get_mapping_area_size? @@ +141,3 @@ + * by the user */ + if(!found || (get_mapping_area(mapping) < + get_mapping_area(found))) I would use a single line here, it won't be much longer than the comment above and it reads better I think.
Review of attachment 359422 [details] [review]: ::: libdocument/ev-mapping-list.c @@ +103,3 @@ +get_mapping_area (EvMapping *mapping) +{ + return (mapping->area.x2 - mapping->area.x1) * This needs to check for multiplication overflow. I would move the comparison inside the function here, returning > 0, < 0, and == 0, similarly to strcmp. @@ +127,1 @@ + g_return_val_if_fail (mapping_list != NULL, NULL); You still have unneeded whitespace changes. @@ +135,3 @@ (y <= mapping->area.y2)) { + + /* In case of only one match choose that. Otherwise The indentation was incorrect in the original code, and is also incorrect here. Should be 8-character tabs. @@ +140,3 @@ + * In this way we allow most of the elements to be selectable + * by the user */ + if(!found || (get_mapping_area(mapping) < if (!found || (get_mapping_area... Also, space before parenthesis. @@ +141,3 @@ + * by the user */ + if(!found || (get_mapping_area(mapping) < + get_mapping_area(found))) You will need braces here because the conditional is multi-line.
Created attachment 359525 [details] [review] libdocument: Fixing display of overlapping annotations If two PDF annotations overlap, one might "shadow" the other, making it imposible for the user to click on of them if the smaller one is below the larger one. This fix calculates the area of bounding box and always puts the smaller sized annotations on top of the larger ones. With help from Bastien Nocera <hadess@hadess.net>
Sorry, but this doesn't look good to me. Please note that the mappings save the coordinates as doubles if I looked it up correctly. Any reason why we don't change my code to use doubles instead of guint64_t?
Created attachment 359528 [details] [review] libdocument: Fixing display of overlapping annotations If two PDF annotations overlap, one might "shadow" the other, making it imposible for the user to click on of them if the smaller one is below the larger one. This fix calculates the area of bounding box and always puts the smaller sized annotations on top of the larger ones. With help from Bastien Nocera <hadess@hadess.net>
Review of attachment 359525 [details] [review]: ::: libdocument/ev-mapping-list.c @@ +127,3 @@ + + /* If one overflowed, then it's bigger than the other */ + if (!ret_a && !ret_b) Here you end up in an approximation again, as with the original code... If you are really afraid of this kind of issues you could compute area_1 = log(wa) + log(ha) instead, calculate area_2 accordingly and compare these both values (you don't have to reverse the operation because of the monotonic properties of the log function and it will give you accurate results for far larger numbers... However at the moment I think, that you should be fine with just using gdouble instead of guint64 as it's used in EvMapping anyway... I had done this myself, but I expected the members to be ints not floating point numbers (hopefully I haven't messed up the math while thinking about that this quick).
Created attachment 359529 [details] [review] Updated patch @Carlos: I modified my original patch according to your suggestions. Unfortunatly Bastien and I have different opinions about checked multiplication. As mentioned earlier, using the overflow flag for something else than error detection is pointless. Even more as there are mathematically better options. However to address his somehow valid criticism about overflowing numbers I changed the datatype to gdouble. I believe that this is the best option here in order to minimize speed and code complexity penalties... Also because the x1,x2 members of the EvMapping apprently are already gdoubles and not gints as I assumed earlier, so the loss of precision should not hurt that much. If two objects are really close in their size, the algorithm will most likely fail anyways...
I like both patches, to be honest. I like the idea of using a cmp function, but I agree we shouldn't be converting the doubles to unsigned integers. I think it's quite unlikely that the multiplication overflows using doubles, we would have annotations that don't fit on the screen anyway. So, in the worst case we will end up returning a wrong value, but I don't think the annotation will be working in that case either. So, I'm not sure it's worth checking the overflow. So, unless I'm wrong, I think we could take Bastien's approach of avoiding multiplications when possible, and then just do the multiplications without any overflow check. I would also renames cmp_mapping_area as cmp_mapping_area_size, since again we are not comparing rectangles, but their sizes.
I wonder, how different is this bug with respect to Bug 769133. That also have some patches that need some work.
I took a look at this and on the first glance the patch proposed in Bug 769133 doesn't seem to fix this bug.
As José clarified on the other bug (thanks!), these bugs are somehow complementary fixes for selection process of annotations. Bug 769133 cares about two not truly overlapping annotations (quadrilaterals don't overlap / bounding boxes overlap; can only be the case for markup annotations) and mine about two truly overlapping annotations and tries to make the selection possible by putting the smallest (in bounding box area sense) to the front. Sadly, as far as I can see, it's not easily possible to merge them both together as they modify the same code sections...
Fabian, I will rework my patches for bug 769133 in top of the patch here.
Sorry, I think I committed Fabian's patch by mistake when I released 3.27.91. So I fixed this by using Bastian's approach while following what Carlos said in comment 13, that is avoid multiplication but don't check for the overflow (as Fabian is right, the width and height are already doubles). The commit is 4416eb17a. Thus, I think this bug can be closed. Feel free to reopen if there is something wrong or missing.