Bug 47282

Created attachment 58376 [details] [review]
Patch (rebased)

Created attachment 58377 [details] [review]
Patch (rebased)

Created attachment 58380 [details] [review]
Patch (rebased)

Created attachment 58395 [details] [review]
Patch (working)

Working and tested with okular

Created attachment 58396 [details] [review]
Okular patch (for reference)

What does the loop in ScreenAnnotation::ScreenAnnotation( const QDomNode & node ) do?

What's the point in creating an empty screen elment in ScreenAnnotation::store?

 \since 0.10 needs to be  \since 0.20

Please pass the MediaRendition to LinkRenditionPrivate

poppler-media.h: qt interface to poppler in poppler-media.cc

MediaRendition has no documentation at all

MediaRendition misses some const in the getters (isValid, contentType, etc)

MediaRenditionPrivate *d_ptr; is duplicating what Q_DECLARE_PRIVATE( MediaRendition ) does?

Please use UnicodeParsedString instead of QString::fromLatin1

Please d-pointify ScreenObject

poppler-screen.cc says poppler-sound.cc: qt interface to poppler

What's the use of passing AnnotScreen *ann to ScreenObject::ScreenObject

What's the use of ScreenObject itself if only holds a LinkRendition ?

Q_DISABLE_COPY to StreamDevice ?

StreamDevice seems like it could to more to implement QIODevice functions like reset, etc, i understand you decided to say its isSequential to simplify the job, maybe you should rename StreamDevice to StreamSequentialDevice?

All the public new functions also need \since as well as enums and classes

(In reply to comment #6)

I would like to point out that I tried to stick as close as possible to current implementations, though I did also add a few of my usual blunders to keep things fresh. Enjoy!

> What does the loop in ScreenAnnotation::ScreenAnnotation( const QDomNode & node
> ) do?
> 
> What's the point in creating an empty screen elment in ScreenAnnotation::store?

  *Removed* (appeared in MovieAnnotation, left it in as a place holder)

>  \since 0.10 needs to be  \since 0.20

  *Replaced* (woops)

> Please pass the MediaRendition to LinkRenditionPrivate

  *Passed* (though it appeared as assignment in other existing implementations)

> poppler-media.h: qt interface to poppler in poppler-media.cc

  *Fixed*

> MediaRendition has no documentation at all

  *Documented*

> MediaRendition misses some const in the getters (isValid, contentType, etc)

  Sadly the original (wrapped) class didn't declare it's functions as constant, and I was a bit reluctant to change the original rendition class. Anyway, it's been done.

  *Constant-ed*
 
> MediaRenditionPrivate *d_ptr; is duplicating what Q_DECLARE_PRIVATE(
> MediaRendition ) does?

  Again, noticed that some already existing implementation where using Q_DECLARE_PRIVATE in order to use Q_D in member functions.
  As you know, d_ptr is not declared by the macro, so I had to add it manually.

#define Q_DECLARE_PRIVATE(Class) \                                                                                                
inline Class##Private* d_func() { return reinterpret_cast<Class##Private *>(qGetPtrHelper(d_ptr)); } \
inline const Class##Private* d_func() const { return reinterpret_cast<const Class##Private *>(qGetPtrHelper(d_ptr)); } \
friend class Class##Private;

#define Q_D(Class) Class##Private * const d = d_func()

  *Answered*

> Please use UnicodeParsedString instead of QString::fromLatin1

  *Replaced* (remnant from MovieAnnotation's current implementation)

> Please d-pointify ScreenObject

  *PIMPL'd*

> poppler-screen.cc says poppler-sound.cc: qt interface to poppler

  *Replaced*

> What's the use of passing AnnotScreen *ann to ScreenObject::ScreenObject

  *Removed* (sorry, left over from a previous attempt)

> What's the use of ScreenObject itself if only holds a LinkRendition ?

  For consistency, anyway the class is (was) no more heavy than a private class, not to mention it might come in handy later.

  * Consistency*

> Q_DISABLE_COPY to StreamDevice ?

  *Added*

> StreamDevice seems like it could to more to implement QIODevice functions like
> reset, etc, i understand you decided to say its isSequential to simplify the
> job, maybe you should rename StreamDevice to StreamSequentialDevice?

  Well, not quite, I first implemented it as a full blown non-sequential stream, but then I noticed that the Stream object getting sent to the wrapper was a FilterStream, so seeking was out of the question.

  *Renamed* (also added close for kicks)
StreamSequentialDevice

Created attachment 58408 [details] [review]
Patch (cleaned 1)

(In reply to comment #7)
> All the public new functions also need \since as well as enums and classes

*Sinced*

Created attachment 58409 [details] [review]
Patch (cleaned 2)

Created attachment 58460 [details] [review]
Patch (cleaned 3)

Pushed