Bug 92040

We do not care for binary compatibility on the poppler/ level, only in the frontends.

Please also mark in int variants in the qt frontends as deprecated.

And float duration; should be a double?

Created attachment 118362 [details] [review]
The patch

(In reply to Albert Astals Cid from comment #1)
> Please also mark in int variants in the qt frontends as deprecated.
Done.

> And float duration; should be a double?
I only wrote float due to having binary compatibility in mind (same size as int, so the structure size doesn't change). Now that it doesn't matter, I've changed to double.

What do you make of the modifications to the glib frontend, where the binary compatibility can only be preserved by replicating the whole class?

I've pushed the core and Qt parts, someone with knowledge of the glib side needs to think if your patch is good or not.

(In reply to Albert Astals Cid from comment #3)
> I've pushed the core and Qt parts, someone with knowledge of the glib side
> needs to think if your patch is good or not.

Thank you! Sorry for missing that Q_DECL_DEPRECATED. A patch for okular is coming :)

Comment on attachment 118362 [details] [review]
The patch

Review of attachment 118362 [details] [review]:
-----------------------------------------------------------------

::: glib/poppler-page.h
@@ +341,5 @@
> +  gdouble duration;
> +  gint angle;
> +  gdouble scale;
> +  gboolean rectangular;
> +};

Thanks for the patch. What does Ex stand for? I don't think we need a new boxed type for this. I think that as long as we don't change the order of the struct members, we can append a new one without breaking api/abi. So, I would just add gdouble duration_float to the existing struct.

Created attachment 120275 [details] [review]
Non-integer transition durations for poppler-glib

(In reply to Carlos Garcia Campos from comment #5)

> Thanks for the patch. What does Ex stand for? I don't think we need a new
> boxed type for this. I think that as long as we don't change the order of
> the struct members, we can append a new one without breaking api/abi. So, I
> would just add gdouble duration_float to the existing struct.

Right, people are not supposed to create instances of the structure by direct malloc, are they. This is what happens when a KDE user starts hacking on GObject stuff :) Here's a new patch (I called in duration_real to be consistent with the already existing counterpart in the Qt wrapper, and also because "float" might be confusing whet the type is double).

(In reply to Arseniy Lartsev from comment #6)
> Created attachment 120275 [details] [review] [review]
> Non-integer transition durations for poppler-glib
> 
> (In reply to Carlos Garcia Campos from comment #5)
> 
> > Thanks for the patch. What does Ex stand for? I don't think we need a new
> > boxed type for this. I think that as long as we don't change the order of
> > the struct members, we can append a new one without breaking api/abi. So, I
> > would just add gdouble duration_float to the existing struct.
> 
> Right, people are not supposed to create instances of the structure by
> direct malloc, are they.

Right, those are always allocated by poppler. It's unfortunate that those struct are public, but still.

> This is what happens when a KDE user starts hacking
> on GObject stuff :) 

:-)

> Here's a new patch (I called in duration_real to be
> consistent with the already existing counterpart in the Qt wrapper, and also
> because "float" might be confusing whet the type is double).

OK, sounds good to me. Thanks.

Pushed, thanks!