Created attachment 50825 [details] [review]
partial support for Movie actions

Some quick comments:
 * Please try to keep spacing consistant with the rest of the file, at least the ifs are not correctly spaced
 * Please give all the parameters names
 * You pass r and g and then never use them, why?
 * All the data should be in the private, so please remove the protected OperationType member and put it in the private

Thanks for the patch :-)

(In reply to comment #2)
> Some quick comments:
>  * Please try to keep spacing consistant with the rest of the file, at least
> the ifs are not correctly spaced

Have you noticed, you have at least two different styles across files? :)

>  * You pass r and g and then never use them, why?

An action can refer to a movie annotation either by title or by ref-gen pair. I've done the title, but I've skipped the ref-gen comparison for now, since MovieAnnotation does not store them (or at least I haven't found them). That's why I called the support "partial".

Created attachment 50836 [details] [review]
updated patch as per comments

OperationType type; is still a member of LinkMovie instead of LinkMoviePrivate (reading my comment i see that it was probably confusing in what i meant)

Created attachment 50838 [details] [review]
updated patch as per comments

I tried the patch with your patched okular and the test file given in the first comments of this okular bug https://bugs.kde.org/show_bug.cgi?id=136574.

Unfortunately, for me the movie on the second slide does not start automatically, even though it should.

I placed a few printfs in your code to see what's going on.  It appears that the new isTarget method you added to poppler-link.cc is never called for the test file.

That's very unfortunate. Can you debug it further?

If not, could you please provide the stderr output of okular?

If yes, the invokation of isTarget() is near generator_pdf.cpp:256

http://git.hades.name/cgit.cgi/okular.git/tree/generators/poppler/generator_pdf.cpp?h=movie&id=dbee5267271f2a634652e19827f2ba25e4345c98#n256

Could you check why is it not being invoked?

Could it by any chance be because of some kind of okular not linking to new okularGenerator_poppler.so ?

Created attachment 51551 [details]
Tex example, slightly modified to actually compile with pdflatex

Created attachment 51552 [details]
The pdf that results

I tested some more, and here is what I found.  The part about isTarget not being called was actually bogus.  I hadn't known about the kbuildsycoca4 call you have to make.

Now it does indeed work for the test file mentioned above.  Thanks!  However, it only works in presentation mode, and not in regular mode (but maybe that's intentional?).  Also, it does not work if you first go to a slide with an autostarting movie on it, and only then switch to presentation mode.  You need to be in presentation mode and then switch to the page with the movie.

Then, I wanted to try this with a pdf created by myself, using LaTeX and beamer as in the above example.  First I wanted to simply recompile the given example.  Strangely enough, that one cannot compile at all, because it references an eps file, which a) is not provided and b) pdflatex won't take eps files anyways.  So I patched that tex file a little bit, recompiled, and the movie does not autostart. 

Unfortunately I don't have a Windows machine near me to test whether at least there the movie autostarts.  I attached my patched tex file instead.

(In reply to comment #11)

Thanks for testing!

The autostart indeed works only in presentation mode. This is because in normal mode Okular does not activate Actions associated with page opening. I am not sure if this is intended by Okular developers or a bug (which I haven't yet investigated), but in any case it's not a Poppler issue.

Regarding your PDF file, I am not sure where is the error in .tex file, but the resulting PDF does not contain the necessary code to autostart video (the movie action "46 0" is not referenced in page's actions). I am sure (although haven't checked) that it won't work in Windows also.

> The autostart indeed works only in presentation mode. This is because in normal
> mode Okular does not activate Actions associated with page opening. I am not
> sure if this is intended by Okular developers or a bug (which I haven't yet
> investigated), but in any case it's not a Poppler issue.

I am not sure what behavior to expect here, either.  I guess that is on Albert to decide.

> 
> Regarding your PDF file, I am not sure where is the error in .tex file, but the
> resulting PDF does not contain the necessary code to autostart video (the movie
> action "46 0" is not referenced in page's actions). I am sure (although haven't
> checked) that it won't work in Windows also.

I was afraid you were going to say this.  The tex-file requests autostarting of the movie in the way described by the 'beamer' latex package manual (by simply adding the 'autostart' option).  If this does not lead to a correct pdf does this mean it is a beamer bug?  And how did the original poster generate his demo pdf? I have no clue.

What about the issue that a movie will not autostart if you switch to presentation mode will showing the corresponding page?  I guess that could be annoying if you have an autostarting movie on your first slide.

Cheers,
Oliver

(In reply to comment #13)
> I am not sure what behavior to expect here, either.  I guess that is on Albert
> to decide.

Personally i find it a bit agressive to have a video autostart if i am just browsing it (i.e. not in presentation mode)

> I was afraid you were going to say this.  The tex-file requests autostarting of
> the movie in the way described by the 'beamer' latex package manual (by simply
> adding the 'autostart' option).  If this does not lead to a correct pdf does
> this mean it is a beamer bug?  
Yes

> What about the issue that a movie will not autostart if you switch to
> presentation mode will showing the corresponding page?  

I'd probably call this a bug, but don't have a strong opinion

> Personally i find it a bit agressive to have a video autostart if i am just
> browsing it (i.e. not in presentation mode)

I tend to see this the same way.
 
> > What about the issue that a movie will not autostart if you switch to
> > presentation mode will showing the corresponding page?  
> 
> I'd probably call this a bug, but don't have a strong opinion

Even with this 'bug' the patch is a definite improvement.  Can we have it in master, please?

I've already spoken with edward and he knows of that the patch needs to be improved before i commit it.

okay, thanks guys!

Edward did you have time to improve the suggestions we talked about back in 2011-10-04 as said in comment #16 ?

Created attachment 58057 [details] [review]
Updated patch for implementing support for LinkMovie

The LinkMovie takes the title and the reference (new class) of the associated annotation now and provides static convenience methods to check whether a LinkMovie object or title/reference pair matches a MovieAnnotation.

There's broken indentation in 
QString LinkMovie::annotationTitle() const
and
ObjectReference LinkMovie::annotationReference() const

bool LinkMovie::isReferencedAnnotation( const MovieAnnotation *annotation, const LinkMovie *link )
should not be static and use "this" instead of link

Why would you ever need
bool LinkMovie::isReferencedAnnotation( const MovieAnnotation *annotation, const QString &annotationTitle, const ObjectReference &annotationReference ) ?

Why someone would want to know the 
ObjectReference annotationReference() const; ?

Created attachment 58118 [details] [review]
Integrated review

This patch removes the isReferencedAnnotation() method that took title and reference as parameter. The intent was to not keep the Poppler::LinkMovie object around in Okular, but that's of course a design problem in Okular and shouldn't be reflected in Poppler API.
So LinkMovie provides a const member method now and hides the title/reference from public API.

You should pass the other values to the LinkMoviePrivate constructor, this way you avoid the akward thing of having LinkMoviePrivate initialize operation to LinkMovie::Play and then do d->operation = operation;

Also i think it'd be a nice idea if you don't expose
void setPdfObjectReference(const ObjectReference &reference);
since i don't see a reason a user would use it, and simply go behind the scenes and update d->pdfObjectReference

The since should be 0.20, not 0.8 and are missing in the new functions

And last i remember we were trying to use QSharedDataPointers in stuff like ObjectReferencePrivate * const d; but i'll accept if you say "nah" on it.

Created attachment 58161 [details] [review]
Integrated review comments from Albert

Since the objectreference is never passed anymore to the user, i think it'd make sense to turn the .h into a _p.h and not install it. What do you think?

(In reply to comment #24)
Hej Albert,

> Since the objectreference is never passed anymore to the user, i think it'd
> make sense to turn the .h into a _p.h and not install it. What do you think?
It is still passed to the LinkMovie ctor. Should this be made private as well?

It is ok, it is a const & so just do a forward declare and it will work. Maybe document the constrcutor is private so people don't get crazy trying to create one :D

Created attachment 58264 [details] [review]
Next interation of integrating review comments

Make poppler-objectreference.h a private header now and document the LinkMovie ctor as being private.

I've commited your patch with a minor change in which two invalid objectreferences are equal, having

ObjectReference a;
ObjectReference b;
bool c = a == b;

with c being false was too weird