search

sailfishos / sailfish-office

Sailfish Office
GNU General Public License v2.0
73 stars 26 forks source link

[sailfish-office] Fix crash when adding pdf annotation below the document. Fixes JB#48393 #156

Closed pvuorela closed 4 years ago

pvuorela commented 4 years ago

On a very short document it's possible to try adding an annotation below the document area. Doing that made the app crash.

Now the operation fails silently on the UI side, but should be quite much better already.

@dcaliste @jpetrell

dcaliste commented 4 years ago

It's ok as you did. It's fixing the issue.

Looking at the code, I'm wondering if we should modify the attachOnce() routine to return false when failing, so callers can quick return and not call the failing addAnnotation routine. What do you think ?

pvuorela commented 4 years ago

Was also considering making some protection there, maybe more as "if (m_page >= 0) ...->addAnnotation(..)", but there were multiple places calling addAnnotation() so only a single check here seemed simpler.

dcaliste commented 4 years ago

but there were multiple places calling addAnnotation() so only a single check here seemed simpler.

Indeed, this LGTM as is.

In fact, it seems to me that all places addAnnotation() is called, a attachOnce call is done before, allowing to short-circuit by quick return if the latter returns false (in addition to the protection you already added).

pvuorela commented 4 years ago

I'll merge this now to get rid of a crash. In longer term we could perhaps consider do we want to either show some notification if an annotation fails, or in this case prevent the dialog from appearing when clicking outside the pdf content.