> On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote: > > core/document.cpp, line 3282 > > <http://git.reviewboard.kde.org/r/107442/diff/6/?file=119425#file119425line3282> > > > > I am wondering why do we need this map for the prev contents, as far as > > i can see annotwindow does not modify the annotation anymore, so the > > annotation * you get here should have the old values still no? > > Jon Mease wrote: > Yes, we could assume that the old contents are still in the annotation > but this makes me a little nervous because there's nothing stopping a user of > Okular core from modifying the annotation's contents as well which would then > break the undo stack. But we could do it this way and just add API > documentation specifying that the annotation's contents should not be > updated. > > If we do go this route I have a question. The AnnotationWindow class > uses the function GuiUtils::contents rather than Annotation::contents when > retrieving the contents of an annotation. Would the Document class need to > use this GuiUtils version as well in order to grab the old contents (rather > than Annotation::contents?). I'm not really clear from the source what > conditions the GuiUtils version is handling. If so we'll need to move this > function out of GuiUtils (because of the GUI dependency) and into some other > core class. Let me know if you have a preference of where to move it to. > > Albert Astals Cid wrote: > "there's nothing stopping a user of Okular core from modifying the > annotation's contents" > Sure, there's nothing stopping people from doing int *a = 0; *a = 33; > either :D We have to document stuff properly and do the best we can to review > new stuff so that it does not break > > About the second part, yes, it seems we'd need that logic somewhere, as > you are already doing some special casing in > DocumentPrivate::performSetAnnotationContents, honestly i can't really say > what the GuiUtils code does either :D About a name, I guess we could always > go with Document::annotationContents(Annotation *);
About the first check in GuiUtils::contents ( m_annot->window().text() ): it is correct in principle, because under some circumstances the popup windows's text overrides the actual annotation contents. However, such circumstances are currently not even detectable outside Poppler (*). Furthermore, Poppler-qt4 never sets popup windows' text and always returns an empty string. So I say let's keep it simple and remove it. We can solve this issue at a lower level in Poppler itself in the future. * = [unimportant detail] this happens if the /Popup annotation has *no* optional /Parent field (see PDF 32000-1:2008, table 183) About the second check: in Poppler, inplaceText is a synonym for contents. It used to be a separate thing but now it only exists for source compatibility purposes. So I think we can kill it in Okular too and always use annotation->contents(). Of course it needs to be done in a backward-compatible way (we don't want users losing their saved annotations) tldr: You can remove TextAnnotation's inplaceText/setInplaceText, ignore all conditions listed in GuiUtils::contents, kill GuiUtils::contents and always use plain Annotation::contents/setContents :D - Fabio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review29227 ----------------------------------------------------------- On March 12, 2013, 1:26 a.m., Jon Mease wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/107442/ > ----------------------------------------------------------- > > (Updated March 12, 2013, 1:26 a.m.) > > > Review request for Okular. > > > Description > ------- > > This patch adds undo/redo support to Okular annotation manipulation commands. > > 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 include support for undoing and redoing editing actions > on forms. > > > > > 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 6ff6536 > core/document.cpp 5ab759e > core/document_p.h fb3aec6 > core/page.cpp 1db2763 > part.rc 39c1571 > ui/annotationpropertiesdialog.cpp 4b02258 > ui/annotwindow.h f7df9f6 > ui/annotwindow.cpp c1bafb9 > ui/pageview.cpp b018dfe > > 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. > > > Thanks, > > Jon Mease > >
_______________________________________________ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
