Re: [Okular-devel] Review Request: Add undo/redo support for annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review24050 --- core/annotations.cpp http://git.reviewboard.kde.org/r/107442/#comment18342 I'm not sure it is legal to keep a QDomNode around after its owner QDomDocument has been destroyed. I remember doing some research when I wrote commit 86c92ffec26b0a8e9f6e40767df0a57743224279 and I came to the conclusion it isn't. But let's wait to hear from someone more knowledgeable. core/document.h http://git.reviewboard.kde.org/r/107442/#comment18343 Seems you don't actually use these three classes in document.h core/document.h http://git.reviewboard.kde.org/r/107442/#comment18344 Wondering if newProps is redundant. I mean, isn't newProps always the same as annotation-getAnnotationPropertiesDomNode() ? core/document.h http://git.reviewboard.kde.org/r/107442/#comment18345 Please document what completeDrag is for core/document.h http://git.reviewboard.kde.org/r/107442/#comment18346 Please document textEdit and commit too core/document.h http://git.reviewboard.kde.org/r/107442/#comment18347 s/undo/redo/ core/document.cpp http://git.reviewboard.kde.org/r/107442/#comment18348 We need to check that uc is a TranslateAnnotationCommand too, don't we? (see example at http://qt-project.org/doc/qt-4.8/qundocommand.html#mergeWith) core/document.cpp http://git.reviewboard.kde.org/r/107442/#comment18349 Yeah, uniqueName sounds like a good way to identify annotations, but actually there's no guarantee that these names are unique. Will it work if we just compare tuc-m_annotation != m_annotation instead? Okular-created annotations are always assigned random UUIDs and chances they collide are low, but according to PDF specifications such unique names are optional and, if they exist, they are unique only locally for a given page. core/page.cpp http://git.reviewboard.kde.org/r/107442/#comment18354 Any reason we have to this for each page? Can't we do this at the end of DocumentPrivate::loadDocumentInfo, or, even better, bypass the undo stack while loading saved annotations? part.rc http://git.reviewboard.kde.org/r/107442/#comment18339 Please increase version # part.rc http://git.reviewboard.kde.org/r/107442/#comment18340 In other programs (eg kwrite) I see that undo/redo are usually placed before copy/select stuff Sorry for the long delay, I've started to have a look at this new version. Apart from the minor issues I've noted, most code looks good to me. I'm not fully convinced by AnnotWindow changes, I've tested the code a bit and I've found that popup text changes easily go out of sync. For example: 1) Create ink annotation 2) Set its popup text to foo 3) Create ellipse annotation 4) Append bar to ink annotation's text 5) Edit - Undo: the ellipse is deleted, instead of reverting foobar to foo Or another example: 1) Create annotation 2) Set some popup text 3) Right click on the popup and press Undo from the context menu 4) Notice that the Document hasn't noticed that we've undone our text change (no redo is available, pressing undo doesn't delete the annotation) Seems like we're missing some editing events from QTextEdit. I've had a look at the Qt manual and unfortunately there seems to be no way to access QTextEdit's internal undo stack. I'm afraid we'll have to disable QtextEdit's internal undo stack and reinvent the wheel to handle QTextEdit text changes ourselves. About the other issue with Annotation holding references to QTextEdit, maybe we can keep track of each annotation's QTextEdit in UI code using a QHashAnntation*,QTextEdit* or something. Actually, there seems to be already an interesting QHash Okular::Annotation *, AnnotWindow * m_annowindows field in class PageViewPrivate (pageview.cpp). Let me know what you think about it, and sorry again :) - Fabio D'Urso On Dec. 1, 2012, 9:33 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated Dec. 1, 2012, 9:33 p.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
Re: [Okular-devel] Review Request: Add undo/redo support for annotations
On Dec. 27, 2012, 3:22 p.m., Fabio D'Urso wrote: core/document.cpp, line 2016 http://git.reviewboard.kde.org/r/107442/diff/2/?file=97032#file97032line2016 We need to check that uc is a TranslateAnnotationCommand too, don't we? (see example at http://qt-project.org/doc/qt-4.8/qundocommand.html#mergeWith) Sorry, just read the doc again. Even tough the example code seems to suggest this check is needed, it's not necessary. - Fabio --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review24050 --- On Dec. 1, 2012, 9:33 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated Dec. 1, 2012, 9:33 p.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/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 e5ec39b 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
Re: [Okular-devel] [Bug 312138] Unfolded table of contents folds in upon reloading of document
I would like to give it a try, please point me to the source. in the case where the toc is updated/changed, I guess that part should remain folded the rest should restore to it's previous state. What's your opinion on this ?? btw is this for the reload action only, or is it about to store it restore when the document is opened again? -Jaydeep On Thu, Dec 27, 2012 at 2:35 AM, Albert Astals Cid aa...@kde.org wrote: https://bugs.kde.org/show_bug.cgi?id=312138 Albert Astals Cid aa...@kde.org changed: What|Removed |Added Keywords||junior-jobs --- Comment #4 from Albert Astals Cid aa...@kde.org --- Marking this as a junior job, it should be pretty easy to store the whole tree of the TOC when we trigger the reload and check if the tree is the same after it has happened. Interested people in implementing the junior job please contact the okular-devel@kde.org mailing list if you decide to have a look at this. -- You are receiving this mail because: You are the assignee for the bug. ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] [Bug 312138] Unfolded table of contents folds in upon reloading of document
El Divendres, 28 de desembre de 2012, a les 01:48:55, Jaydeep Solanki va escriure: I would like to give it a try, please point me to the source. You know where the source is ;-) Have you tried searching for where the code that is responsible of this before asking? in the case where the toc is updated/changed, I guess that part should remain folded the rest should restore to it's previous state. What's your opinion on this ?? Let's go the easy way for now a) Keep the toc view in its present state if the toc hasn't changed at all. Otherwise fold it completely btw is this for the reload action only, or is it about to store it restore when the document is opened again? Reload only. Cheers, Albert -Jaydeep On Thu, Dec 27, 2012 at 2:35 AM, Albert Astals Cid aa...@kde.org wrote: https://bugs.kde.org/show_bug.cgi?id=312138 Albert Astals Cid aa...@kde.org changed: What|Removed |Added -- -- Keywords||junior-jobs --- Comment #4 from Albert Astals Cid aa...@kde.org --- Marking this as a junior job, it should be pretty easy to store the whole tree of the TOC when we trigger the reload and check if the tree is the same after it has happened. Interested people in implementing the junior job please contact the okular-devel@kde.org mailing list if you decide to have a look at this. -- You are receiving this mail because: You are the assignee for the bug. ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request: Add undo/redo support for annotations
On Dec. 27, 2012, 3:22 p.m., Jon Mease wrote: Sorry for the long delay, I've started to have a look at this new version. Apart from the minor issues I've noted, most code looks good to me. I'm not fully convinced by AnnotWindow changes, I've tested the code a bit and I've found that popup text changes easily go out of sync. For example: 1) Create ink annotation 2) Set its popup text to foo 3) Create ellipse annotation 4) Append bar to ink annotation's text 5) Edit - Undo: the ellipse is deleted, instead of reverting foobar to foo Or another example: 1) Create annotation 2) Set some popup text 3) Right click on the popup and press Undo from the context menu 4) Notice that the Document hasn't noticed that we've undone our text change (no redo is available, pressing undo doesn't delete the annotation) Seems like we're missing some editing events from QTextEdit. I've had a look at the Qt manual and unfortunately there seems to be no way to access QTextEdit's internal undo stack. I'm afraid we'll have to disable QtextEdit's internal undo stack and reinvent the wheel to handle QTextEdit text changes ourselves. About the other issue with Annotation holding references to QTextEdit, maybe we can keep track of each annotation's QTextEdit in UI code using a QHashAnntation*,QTextEdit* or something. Actually, there seems to be already an interesting QHash Okular::Annotation *, AnnotWindow * m_annowindows field in class PageViewPrivate (pageview.cpp). Let me know what you think about it, and sorry again :) Thanks for the feedback. Yeah, I agree that the current AnnotWindow approach isn't fully satisfactory. The issue, like you observed, is that we can't get full access to the QTextDocument's undo stack. The only information we have is from the undoCommandAdded() signal. So I agree that we'll have to disable our KTextEdit's undo stack and implement something from scratch. On the bright side this will solve the Core/GUI dependency issue :-) For my next version I think I'll try to implement a keystroke-by-keystroke undo/redo capability that properly restores cursor position on undo/redo. Once this is working I'll work out the logic for merging consecutive edits together appropriately. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review24050 --- On Dec. 1, 2012, 9:33 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated Dec. 1, 2012, 9:33 p.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/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 e5ec39b 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
