-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107442/#review22677
-----------------------------------------------------------
Hi :)
It seems that the patch doesn't apply any more and needs to be rebased.
I've tried to reconstruct it and have a quick look, but I couldn't test it as
it doesn't compile.
Some comments in random order:
- DocumentPrivate::perform{Add|Remove}PageAnnotations method are not used
anywhere
- You've removed Document::removePageAnnotations. This needs to be discussed
with Albert as it breaks the API. This patch changes the signature of other
public methods too (this is just a reminder for a later discussion)
- You can't do the Annotation::m_textEdit thing because the Annotation class is
part of okularcore and cannot contain stuff that belongs to the UI (which lives
in okularpart). [remember that okularcore is a library that can be used by
different clients, not by okularpart only]
- Document::updateUndoRedoAvailable() may signal "spurious" changes (for
example if you undo twice there's no need to emit canRedoChanged(true) the
second time).
Maybe you can just connect signal-to-signal and let Qt do the rest, something
like:
connect( d->undoStack, SIGNAL(canUndoChanged(bool)), this,
SIGNAL(canUndoChanged(bool)))
- Do we really need these new public methods? Keep in mind that once we add
them they become part of the API/ABI, so it's better to avoid them at first if
possible. In general, auxiliary methods that are not meant to be called outside
okularcore should be put in the various ***Private classes (I'm referring
to the three methods in Annotation and the m_textEdit member)
- Why Annotation::getNewPrivateAnnotation? Wasn't "new
BlablablaAnnotationPrivate()" ok? Please also note that calling virtual methods
within the constructor of the same object is generally not recommended because
the class hasn't been fully initialized yet.
- In annotations_p.h you've forward-declared classes QTextEdit and QMatrix but
I don't see them being used. If you want to use QMatrix, please have a look at
commit aa6ed8afc0f595db1d372b81dc8db2593e7055e1
- kDebug(4700) << "new edit contents"; → kDebug(OkularDebug) << "new edit
contents";
- In document.h you're referencing class AnnotWindow. You can't, because that's
part of okularpart.
- Do the *AnnotationCommand classes need to be exported? If not, please remove
the OKULAR_EXPORT tag and move them to some not-exported blabla_p.h file (such
as document_p.h or a dedicated undocommands_p.h)
- I don't understand "// setup actions shouldn't be undoable \
m_doc->undoStack->clear();" in page.cpp. Can you explain it please? Thank you
Undo/redo support is one of the most requested features about annotations, so
thank you very much for your work!
P.S.: About the issue with PDF files, you have to be careful with them as they
are "special": in non-PDF files annotations are entirely handled by Okular
(rendering included), wheres in PDF files it's Poppler who draws them and saves
them to PDF. (Poppler is the library we use to render and save PDF files)
When you use annotations in PDF files you need to make sure that poppler stays
informed about them via the AnnotationProxy interface (so that it knows both
what to draw and what to save if the user presses Save As), and you also need
to preserve the Annotation::ExternallyDrawn flag, which basically tells
Okular's PagePainter class not to redraw annotations above the ones drawn by
Poppler. Note that this is just a hint, I haven't checked if you are already
doing this.
- Fabio D'Urso
On Nov. 24, 2012, 3:09 a.m., Jon Mease wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107442/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2012, 3:09 a.m.)
>
>
> Review request for Okular.
>
>
> Description
> -------
>
> This patch is a first cut at adding undo/redo support to Okular. This patch
> is not yet complete, however it is far enough along that I would like to
> begin incorporating feedback from the community.
>
> Functionality:
> The following actions can be undone and redone: creation and removal of
> annotations, editing arbitrary annotation properties, relocating annotations
> with Ctrl+drag, and editing the text contents of an annotation.
>
> This patch does not yet include support for undoing and redoing editing
> actions on forms. I plan to implement this form undo functionality before
> the functionality of this patch is included in Okular.
>
> Known Issue:
> When editing an annotation's properties in a .dvi file the annotation is
> altered and the action can be undone as expected. However, when editing an
> annotation's properties in a .pdf file the image of the original annotation
> is not removed from the document when the altered annotation appears. I
> would appreciate any possible leads on this issue.
>
>
>
> This addresses bug 177501.
> http://bugs.kde.org/show_bug.cgi?id=177501
>
>
> Diffs
> -----
>
> core/annotations.h 72abdff
> core/annotations.cpp 49ab5bd
> core/annotations_p.h 221572d
> core/document.h 1d825e1
> core/document.cpp 3e4e21a
> core/document_p.h 4a20561
> core/page.cpp 4df58e0
> part.rc 39c1571
> ui/annotationpropertiesdialog.cpp 4b02258
> ui/annotwindow.h f7df9f6
> ui/annotwindow.cpp c1bafb9
> ui/pageview.cpp bf8b546
>
> Diff: http://git.reviewboard.kde.org/r/107442/diff/
>
>
> Testing
> -------
>
> I have tested the undoing and redoing of the specified annotation actions
> using .dvi and .pdf documents. The only known issue is the one described
> above when using .pdf files.
>
>
> Thanks,
>
> Jon Mease
>
>
_______________________________________________
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel
