Re: [Okular-devel] Review Request 118171: Fix for bug 118171 - crash when clicking in a text form that is not editable
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118171/ --- (Updated May 20, 2014, 9:25 p.m.) Status -- This change has been marked as submitted. Review request for Okular. Bugs: 334611 http://bugs.kde.org/show_bug.cgi?id=334611 Repository: okular Description --- Fix for bug 334611 - Crash when clicking in a text form that is not editable. Solution is to not connect the undo/redo support signals for read-only form fields. Diffs - ui/formwidgets.cpp 023a25f Diff: https://git.reviewboard.kde.org/r/118171/diff/ Testing --- Okular no longer crashes when selecting text from the read only text field in the PDF supplied with the bug report Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Review Request 118171: Fix for bug 118171 - crash when clicking in a text form that is not editable
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118171/ --- Review request for Okular. Bugs: 334611 http://bugs.kde.org/show_bug.cgi?id=334611 Repository: okular Description --- Fix for bug 334611 - Crash when clicking in a text form that is not editable. Solution is to not connect the undo/redo support signals for read-only form fields. Diffs - ui/formwidgets.cpp 023a25f Diff: https://git.reviewboard.kde.org/r/118171/diff/ Testing --- Okular no longer crashes when selecting text from the read only text field in the PDF supplied with the bug report Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] [okular] [Bug 334611] Crash when clicking in a text form that is not editable
https://bugs.kde.org/show_bug.cgi?id=334611 --- Comment #3 from Jon Mease jon.me...@gmail.com --- Sure, I'll try to take a look by this weekend. -- 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
Re: [Okular-devel] Review Request 114153: An Idea: Render PDF annotations internally while they are being moved
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114153/ --- (Updated Dec. 30, 2013, 12:53 p.m.) Review request for Okular. Changes --- Added a status update to the description. Repository: okular Description (updated) --- Update: The plan now is to only perform the internal rendering described below for Line, Ink, and Geometric annotations. As I've worked with the internal rendering for these annotation types I've found several small bugs in the internal annotation rendering code that cause visual differences between Okular's rendering and Poppler's. My next steps are to open a series of bug reports and small review requests for these individual rendering bugs. This review request can be considered to be on-hold until I've had time to document and fix these rendering bugs. Unlike in other document formats, the annotations on PDF documents are rendered by the Poppler back-end along with the document itself. Because this document rendering step is expensive we don't render annotations on PDF documents while the annotations are being moved (With Ctrl+Left click drag). Instead of rendering the annotation itself during the move, we render a dashed outline of the annotation. For non-PDF document types the annotations are rendered by Okular on top of the document, and so there is no large performance penalty in rendering the annotation smoothly as it is moved. I find the aesthetic experience of moving annotations on non-PDF to be much more pleasing. In this small patch updates the paintCroppedPageOnPainter() function draw external annotations using the internal annotation drawing logic while the annotation is being moved. It also removes the dashed annotation outline during the move. With this small change the experience of moving an annotation on a PDF now matches that of moving an annotation on the other document formats. Two small oddities: The rendering of the popup note icon differs between the Poppler back-end and Okular's internal rendering so the icon changes form while being moved and then changes back after being dropped. The rendering of fonts on inline notes between the Poppler back-end and Okular's internal rendering seems to differ in some cases so as you move an inline note the font changes. Thoughts? Diffs - ui/pagepainter.cpp d5d9c3e Diff: https://git.reviewboard.kde.org/r/114153/diff/ Testing --- Tested drawing and moving each of the annotation types on a PDF document and on a DVI document. The behavior on the DVI document is unchanged. I find the behavior on the PDF document to be more natural. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114060: Proposed viewport transition refinements for Find and Undo/Redo actions
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114060/ --- (Updated Dec. 29, 2013, 10:28 p.m.) Status -- This change has been marked as submitted. Review request for Okular. Repository: okular Description --- This patch introduces viewport transitions for undo/redo actions on annotations and forms. When an annotation/form action is undone/redone but the associated annotation/form is not currently visible, the viewport is updated to center on the undo/redo action. If the annotation/form is visible, the viewport is not updated. The viewport transitions for the Find action have also been updated to this same algorithm. Previously the viewport was moved to center on each matching search term even if the search term was already visible in the viewport. This lead to unnecessary viewport transitions if the search term matched several items in a single paragraph for example. These proposed changes to the viewport transition behavior are consistent with the find and undo behavior of many existing applications including Kate, Open Office, and Foxit PDF Reader. Diffs - core/document.h fe296e0 core/document.cpp 265ee09 core/document_p.h 3a257de core/documentcommands.cpp 7799bb0 core/documentcommands_p.h fe1c577 core/page.cpp 0bafa99 core/utils.cpp 5dd8448 core/utils_p.h df82fe1 Diff: https://git.reviewboard.kde.org/r/114060/diff/ Testing --- Manual testing of the viewport behavior for find and undo/redo actions on several documents. I also tested that the desired behavior is maintained when documents are rotated. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114153: An Idea: Render PDF annotations internally while they are being moved
On Nov. 27, 2013, 7:49 p.m., Fabio D'Urso wrote: Rendering differences (that I judged ugly) were the reason why I chose to go the dashed outline route. Jon Mease wrote: Yeah, that makes sense. How do you feel about my idea above of only using this approach for the geometric annotations? To my eye the rendering looks almost identical. Fabio D'Urso wrote: It works for me. IIRC, however, there are some differences with straight lines having non-null leader lines (ie those optional perpendicular segments at the endings), maybe you can change Okular's rendering to match Poppler's. BTW, the long-term fix I have in mind is to patch Poppler to render annotations separately to different pixmaps than the rest of the page, so we can really paint them on top of the page inexpensively. But this is lots of work and I have no time to do that at the moment :( so yeah, it works for me! :D Jon Mease wrote: Thanks for the feedback. I'll give this a shot and update the patch accordingly. I'll also see if I can generate some annotations with leader lines in Foxit and take a look at Okular's rendering. BTW, the larger project I'm working towards right now is to be able to write on PDFs in Okular with a Wacom tablet and be able to bulk-select words (collections of ink strokes) and move them around like in Xournal. This update will really improve the appearance of this bulk translation of annotations. I like the sound of this Poppler patch, but it certainly does sound like a lot of work. Fabio D'Urso wrote: You don't need FoxIt at all :) Just create a straight line in Okular and set its Leader Line and Leader Line Extension Length properties, then compare Poppler's and PagePainter's renderings. If they look the same, then I was not remembering correctly :P Thanks for feedback and for the tip. Yes, you're right that Okular's internal rendering of line annotations with leader lines doesn't match Poppler's. In addition, the leader lines aren't always drawn at a perfectly right angle to the main line using Okular's rendering. I've started updating Okular's drawing logic to match poppler, but I'm going to need to update the hit test for line annotations as well. I'll update this review request when I finish this part. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/#review44612 --- On Nov. 27, 2013, 3:22 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/ --- (Updated Nov. 27, 2013, 3:22 p.m.) Review request for Okular. Repository: okular Description --- Unlike in other document formats, the annotations on PDF documents are rendered by the Poppler back-end along with the document itself. Because this document rendering step is expensive we don't render annotations on PDF documents while the annotations are being moved (With Ctrl+Left click drag). Instead of rendering the annotation itself during the move, we render a dashed outline of the annotation. For non-PDF document types the annotations are rendered by Okular on top of the document, and so there is no large performance penalty in rendering the annotation smoothly as it is moved. I find the aesthetic experience of moving annotations on non-PDF to be much more pleasing. In this small patch updates the paintCroppedPageOnPainter() function draw external annotations using the internal annotation drawing logic while the annotation is being moved. It also removes the dashed annotation outline during the move. With this small change the experience of moving an annotation on a PDF now matches that of moving an annotation on the other document formats. Two small oddities: The rendering of the popup note icon differs between the Poppler back-end and Okular's internal rendering so the icon changes form while being moved and then changes back after being dropped. The rendering of fonts on inline notes between the Poppler back-end and Okular's internal rendering seems to differ in some cases so as you move an inline note the font changes. Thoughts? Diffs - ui/pagepainter.cpp d5d9c3e Diff: http://git.reviewboard.kde.org/r/114153/diff/ Testing --- Tested drawing and moving each of the annotation types on a PDF document and on a DVI document. The behavior on the DVI document is unchanged. I find the behavior on the PDF document to be more natural. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org
[Okular-devel] Review Request 114153: An Idea: Render PDF annotations internally while they are being moved
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/ --- Review request for Okular. Repository: okular Description --- Unlike in other document formats, the annotations on PDF documents are rendered by the Poppler back-end along with the document itself. Because this document rendering step is expensive we don't render annotations on PDF documents while the annotations are being moved (With Ctrl+Left click drag). Instead of rendering the annotation itself during the move, we render a dashed outline of the annotation. For non-PDF document types the annotations are rendered by Okular on top of the document, and so there is no large performance penalty in rendering the annotation smoothly as it is moved. I find the aesthetic experience of moving annotations on non-PDF to be much more pleasing. In this small patch updates the paintCroppedPageOnPainter() function draw external annotations using the internal annotation drawing logic while the annotation is being moved. It also removes the dashed annotation outline during the move. With this small change the experience of moving an annotation on a PDF now matches that of moving an annotation on the other document formats. Two small oddities: The rendering of the popup note icon differs between the Poppler back-end and Okular's internal rendering so the icon changes form while being moved and then changes back after being dropped. The rendering of fonts on inline notes between the Poppler back-end and Okular's internal rendering seems to differ in some cases so as you move an inline note the font changes. Thoughts? Diffs - ui/pagepainter.cpp d5d9c3e Diff: http://git.reviewboard.kde.org/r/114153/diff/ Testing --- Tested drawing and moving each of the annotation types on a PDF document and on a DVI document. The behavior on the DVI document is unchanged. I find the behavior on the PDF document to be more natural. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114153: An Idea: Render PDF annotations internally while they are being moved
On Nov. 27, 2013, 7:44 p.m., Albert Astals Cid wrote: Don't have an opinion to be honest. It is true that for some cases it looks better, but for some others it looks weird since you switch between text rendered by poppler and text rendered by us. I'm not oposed to either. Fabio? A compromise would be to only use this approach for the geometric shapes (Ink stroke, line, polygon, and ellipse) and keep that past behavior for everything else. The geometric shapes seem to me to be rendered nearly identically in Okular as in Poppler. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/#review44611 --- On Nov. 27, 2013, 3:22 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/ --- (Updated Nov. 27, 2013, 3:22 p.m.) Review request for Okular. Repository: okular Description --- Unlike in other document formats, the annotations on PDF documents are rendered by the Poppler back-end along with the document itself. Because this document rendering step is expensive we don't render annotations on PDF documents while the annotations are being moved (With Ctrl+Left click drag). Instead of rendering the annotation itself during the move, we render a dashed outline of the annotation. For non-PDF document types the annotations are rendered by Okular on top of the document, and so there is no large performance penalty in rendering the annotation smoothly as it is moved. I find the aesthetic experience of moving annotations on non-PDF to be much more pleasing. In this small patch updates the paintCroppedPageOnPainter() function draw external annotations using the internal annotation drawing logic while the annotation is being moved. It also removes the dashed annotation outline during the move. With this small change the experience of moving an annotation on a PDF now matches that of moving an annotation on the other document formats. Two small oddities: The rendering of the popup note icon differs between the Poppler back-end and Okular's internal rendering so the icon changes form while being moved and then changes back after being dropped. The rendering of fonts on inline notes between the Poppler back-end and Okular's internal rendering seems to differ in some cases so as you move an inline note the font changes. Thoughts? Diffs - ui/pagepainter.cpp d5d9c3e Diff: http://git.reviewboard.kde.org/r/114153/diff/ Testing --- Tested drawing and moving each of the annotation types on a PDF document and on a DVI document. The behavior on the DVI document is unchanged. I find the behavior on the PDF document to be more natural. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114153: An Idea: Render PDF annotations internally while they are being moved
On Nov. 27, 2013, 7:49 p.m., Fabio D'Urso wrote: Rendering differences (that I judged ugly) were the reason why I chose to go the dashed outline route. Yeah, that makes sense. How do you feel about my idea above of only using this approach for the geometric annotations? To my eye the rendering looks almost identical. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/#review44612 --- On Nov. 27, 2013, 3:22 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/ --- (Updated Nov. 27, 2013, 3:22 p.m.) Review request for Okular. Repository: okular Description --- Unlike in other document formats, the annotations on PDF documents are rendered by the Poppler back-end along with the document itself. Because this document rendering step is expensive we don't render annotations on PDF documents while the annotations are being moved (With Ctrl+Left click drag). Instead of rendering the annotation itself during the move, we render a dashed outline of the annotation. For non-PDF document types the annotations are rendered by Okular on top of the document, and so there is no large performance penalty in rendering the annotation smoothly as it is moved. I find the aesthetic experience of moving annotations on non-PDF to be much more pleasing. In this small patch updates the paintCroppedPageOnPainter() function draw external annotations using the internal annotation drawing logic while the annotation is being moved. It also removes the dashed annotation outline during the move. With this small change the experience of moving an annotation on a PDF now matches that of moving an annotation on the other document formats. Two small oddities: The rendering of the popup note icon differs between the Poppler back-end and Okular's internal rendering so the icon changes form while being moved and then changes back after being dropped. The rendering of fonts on inline notes between the Poppler back-end and Okular's internal rendering seems to differ in some cases so as you move an inline note the font changes. Thoughts? Diffs - ui/pagepainter.cpp d5d9c3e Diff: http://git.reviewboard.kde.org/r/114153/diff/ Testing --- Tested drawing and moving each of the annotation types on a PDF document and on a DVI document. The behavior on the DVI document is unchanged. I find the behavior on the PDF document to be more natural. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114153: An Idea: Render PDF annotations internally while they are being moved
On Nov. 27, 2013, 7:49 p.m., Fabio D'Urso wrote: Rendering differences (that I judged ugly) were the reason why I chose to go the dashed outline route. Jon Mease wrote: Yeah, that makes sense. How do you feel about my idea above of only using this approach for the geometric annotations? To my eye the rendering looks almost identical. Fabio D'Urso wrote: It works for me. IIRC, however, there are some differences with straight lines having non-null leader lines (ie those optional perpendicular segments at the endings), maybe you can change Okular's rendering to match Poppler's. BTW, the long-term fix I have in mind is to patch Poppler to render annotations separately to different pixmaps than the rest of the page, so we can really paint them on top of the page inexpensively. But this is lots of work and I have no time to do that at the moment :( so yeah, it works for me! :D Thanks for the feedback. I'll give this a shot and update the patch accordingly. I'll also see if I can generate some annotations with leader lines in Foxit and take a look at Okular's rendering. BTW, the larger project I'm working towards right now is to be able to write on PDFs in Okular with a Wacom tablet and be able to bulk-select words (collections of ink strokes) and move them around like in Xournal. This update will really improve the appearance of this bulk translation of annotations. I like the sound of this Poppler patch, but it certainly does sound like a lot of work. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/#review44612 --- On Nov. 27, 2013, 3:22 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114153/ --- (Updated Nov. 27, 2013, 3:22 p.m.) Review request for Okular. Repository: okular Description --- Unlike in other document formats, the annotations on PDF documents are rendered by the Poppler back-end along with the document itself. Because this document rendering step is expensive we don't render annotations on PDF documents while the annotations are being moved (With Ctrl+Left click drag). Instead of rendering the annotation itself during the move, we render a dashed outline of the annotation. For non-PDF document types the annotations are rendered by Okular on top of the document, and so there is no large performance penalty in rendering the annotation smoothly as it is moved. I find the aesthetic experience of moving annotations on non-PDF to be much more pleasing. In this small patch updates the paintCroppedPageOnPainter() function draw external annotations using the internal annotation drawing logic while the annotation is being moved. It also removes the dashed annotation outline during the move. With this small change the experience of moving an annotation on a PDF now matches that of moving an annotation on the other document formats. Two small oddities: The rendering of the popup note icon differs between the Poppler back-end and Okular's internal rendering so the icon changes form while being moved and then changes back after being dropped. The rendering of fonts on inline notes between the Poppler back-end and Okular's internal rendering seems to differ in some cases so as you move an inline note the font changes. Thoughts? Diffs - ui/pagepainter.cpp d5d9c3e Diff: http://git.reviewboard.kde.org/r/114153/diff/ Testing --- Tested drawing and moving each of the annotation types on a PDF document and on a DVI document. The behavior on the DVI document is unchanged. I find the behavior on the PDF document to be more natural. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114060: Proposed viewport transition refinements for Find and Undo/Redo actions
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114060/ --- (Updated Nov. 28, 2013, 12:47 a.m.) Review request for Okular. Changes --- Updates based on Albert's feedback Repository: okular Description --- This patch introduces viewport transitions for undo/redo actions on annotations and forms. When an annotation/form action is undone/redone but the associated annotation/form is not currently visible, the viewport is updated to center on the undo/redo action. If the annotation/form is visible, the viewport is not updated. The viewport transitions for the Find action have also been updated to this same algorithm. Previously the viewport was moved to center on each matching search term even if the search term was already visible in the viewport. This lead to unnecessary viewport transitions if the search term matched several items in a single paragraph for example. These proposed changes to the viewport transition behavior are consistent with the find and undo behavior of many existing applications including Kate, Open Office, and Foxit PDF Reader. Diffs (updated) - core/document.h fe296e0 core/document.cpp 265ee09 core/document_p.h 3a257de core/documentcommands.cpp 7799bb0 core/documentcommands_p.h fe1c577 core/page.cpp 0bafa99 core/utils.cpp 5dd8448 core/utils_p.h df82fe1 Diff: http://git.reviewboard.kde.org/r/114060/diff/ Testing --- Manual testing of the viewport behavior for find and undo/redo actions on several documents. I also tested that the desired behavior is maintained when documents are rotated. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114060: Proposed viewport transition refinements for Find and Undo/Redo actions
On Nov. 27, 2013, 11:16 p.m., Albert Astals Cid wrote: core/utils.h, line 78 http://git.reviewboard.kde.org/r/114060/diff/1/?file=219403#file219403line78 Same question as before, since this method seems to be used only in core/* can you see if you can put it in some _p.h so we don't expose it to the rest of the world? Moved to util_p.h On Nov. 27, 2013, 11:16 p.m., Albert Astals Cid wrote: core/document.h, line 728 http://git.reviewboard.kde.org/r/114060/diff/1/?file=219398#file219398line728 const for NormalizedRect? Also since this is just used by core/* stufff do we really to make it public? Can it go to some _p.h? Moved to DocumentPrivate class - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114060/#review44637 --- On Nov. 28, 2013, 12:47 a.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114060/ --- (Updated Nov. 28, 2013, 12:47 a.m.) Review request for Okular. Repository: okular Description --- This patch introduces viewport transitions for undo/redo actions on annotations and forms. When an annotation/form action is undone/redone but the associated annotation/form is not currently visible, the viewport is updated to center on the undo/redo action. If the annotation/form is visible, the viewport is not updated. The viewport transitions for the Find action have also been updated to this same algorithm. Previously the viewport was moved to center on each matching search term even if the search term was already visible in the viewport. This lead to unnecessary viewport transitions if the search term matched several items in a single paragraph for example. These proposed changes to the viewport transition behavior are consistent with the find and undo behavior of many existing applications including Kate, Open Office, and Foxit PDF Reader. Diffs - core/document.h fe296e0 core/document.cpp 265ee09 core/document_p.h 3a257de core/documentcommands.cpp 7799bb0 core/documentcommands_p.h fe1c577 core/page.cpp 0bafa99 core/utils.cpp 5dd8448 core/utils_p.h df82fe1 Diff: http://git.reviewboard.kde.org/r/114060/diff/ Testing --- Manual testing of the viewport behavior for find and undo/redo actions on several documents. I also tested that the desired behavior is maintained when documents are rotated. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114049: Add unit tests for editing PDF forms
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114049/ --- (Updated Nov. 24, 2013, 3:37 p.m.) Status -- This change has been marked as submitted. Review request for Okular. Repository: okular Description --- Add a set of unit tests on the editing of PDF form data. These tests also test the undo/redo actions on forms. These tests require the attached formSamples.pdf file be included in the tests/data directory. These tests should apply to KDE/4.11 and onward, but I developed them against 4.12. Diffs - tests/CMakeLists.txt 800a2a7 tests/data/formSamples.pdf PRE-CREATION tests/editformstest.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/114049/diff/ Testing --- This is testing :-) File Attachments Form Samples PDF http://git.reviewboard.kde.org/media/uploaded/files/2013/11/22/0358848d-d534-4bea-b7cd-4f77dfdbfa11__formSamples.pdf Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Review Request 114060: Proposed viewport transition refinements for Find and Undo/Redo actions
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114060/ --- Review request for Okular. Repository: okular Description --- Find Diffs - core/document.h fe296e0 core/document.cpp 265ee09 core/documentcommands.cpp 7799bb0 core/documentcommands_p.h fe1c577 core/page.cpp 0bafa99 core/utils.h 8d5d5fc core/utils.cpp 5dd8448 Diff: http://git.reviewboard.kde.org/r/114060/diff/ Testing --- Manual testing of the viewport behavior for find and undo/redo actions on several documents. I also tested that the desired behavior is maintained when documents are rotated. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114060: Proposed viewport transition refinements for Find and Undo/Redo actions
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114060/ --- (Updated Nov. 24, 2013, 2:23 a.m.) Review request for Okular. Repository: okular Description (updated) --- This patch introduces viewport transitions for undo/redo actions on annotations and forms. When an annotation/form action is undone/redone but the associated annotation/form is not currently visible, the viewport is updated to center on the undo/redo action. If the annotation/form is visible, the viewport is not updated. The viewport transitions for the Find action have also been updated to this same algorithm. Previously the viewport was moved to center on each matching search term even if the search term was already visible in the viewport. This lead to unnecessary viewport transitions if the search term matched several items in a single paragraph for example. These proposed changes to the viewport transition behavior are consistent with the find and undo behavior of many existing applications including Kate, Open Office, and Foxit PDF Reader. Diffs - core/document.h fe296e0 core/document.cpp 265ee09 core/documentcommands.cpp 7799bb0 core/documentcommands_p.h fe1c577 core/page.cpp 0bafa99 core/utils.h 8d5d5fc core/utils.cpp 5dd8448 Diff: http://git.reviewboard.kde.org/r/114060/diff/ Testing --- Manual testing of the viewport behavior for find and undo/redo actions on several documents. I also tested that the desired behavior is maintained when documents are rotated. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 114049: Add unit tests for editing PDF forms
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/114049/ --- (Updated Nov. 22, 2013, 11:29 p.m.) Review request for Okular. Repository: okular Description --- Add a set of unit tests on the editing of PDF form data. These tests also test the undo/redo actions on forms. These tests require the attached formSamples.pdf file be included in the tests/data directory. These tests should apply to KDE/4.11 and onward, but I developed them against 4.12. Diffs - tests/CMakeLists.txt 800a2a7 tests/data/formSamples.pdf PRE-CREATION tests/editformstest.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/114049/diff/ Testing --- This is testing :-) File Attachments Form Samples PDF http://git.reviewboard.kde.org/media/uploaded/files/2013/11/22/0358848d-d534-4bea-b7cd-4f77dfdbfa11__formSamples.pdf Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110589: Undo support for PDF forms
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110589/ --- (Updated June 3, 2013, 1:28 a.m.) Review request for Okular. Changes --- Updates based on Albert's comments. Document::editFormList and Document::editFormCombo no longer require the old values, they are extracted from the form itself. Hence, ComboEdit no longer needs the m_prevText member variable to keep track of previous text. Description --- Add undo / redo support for forms. Along with the previous annotation undo support I believe this completes the implementation of undo/redo for all document editing actions in Okular (Bug 177501). This review request corresponds to the Undo/Redo support in PDF forms feature in the 4.11 feature plan (http://techbase.kde.org/Schedules/KDE4/4.11_Feature_Plan) Potential issue: If the last form or annotation that was modified is outside of the document viewport we are not currently moving the viewport to the form or annotation and so it can be unclear what has been undone. I would appreciate suggestions on whether we should add this viewport shifting and, if so, how best to go about implementing it. This addresses bug 177501. http://bugs.kde.org/show_bug.cgi?id=177501 Diffs (updated) - core/document.h d443917 core/document.cpp 2732441 core/documentcommands.cpp 5fcc195 core/documentcommands_p.h a9775a6 ui/formwidgets.h 24108b8 ui/formwidgets.cpp 57ecceb ui/pageview.h 5e839f2 ui/pageview.cpp 6e093ef Diff: http://git.reviewboard.kde.org/r/110589/diff/ Testing --- Manual testing on a variety of PDFs including forms. I've attached three such documents below. File Attachments Mixed forms 1 http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/forms-scribus.pdf http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/formSamples.pdf Exclusive checkboxes http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/stripped-doc.pdf Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110589: Undo support for PDF forms
On June 1, 2013, 3:40 p.m., Albert Astals Cid wrote: Looks good for me besides one small thing, in editFormList and editFormCombo you are passing the old values while in editFormText and editFormButtons you are not (you get them from the forms themselves) is it possible not to pass the old values to editFormList and editFormCombo too? Jon Mease wrote: This is the approach I originally attempted. However, I couldn't figure out how to reliably get the current text out of a FormFieldChoice object across different versions of Poppler. One complicating factor is that the PopplerFormFieldChoice::editChoice and setEditChoice methods are only meaningfully implemented for versions 0.22 of Poppler so getting this right would require some ifdefs. That said, I'm open to implementation suggestions (and fine if you want to change anything along these lines after committing). Thanks! Albert Astals Cid wrote: I don't understand why you need to interact with Poppler* at all? I mean the ui/* files don't so why do you need to? Isn't FormFieldChoice::currentChoices what you need? Sorry for the confusion. I looked at the code again it wasn't as complicated as it seemed late at night when I was trying things for the first time. And you're right, Poppler doesn't enter in. I believe v3 of the patch addresses your concern. Thanks - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110589/#review33561 --- On June 3, 2013, 1:28 a.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110589/ --- (Updated June 3, 2013, 1:28 a.m.) Review request for Okular. Description --- Add undo / redo support for forms. Along with the previous annotation undo support I believe this completes the implementation of undo/redo for all document editing actions in Okular (Bug 177501). This review request corresponds to the Undo/Redo support in PDF forms feature in the 4.11 feature plan (http://techbase.kde.org/Schedules/KDE4/4.11_Feature_Plan) Potential issue: If the last form or annotation that was modified is outside of the document viewport we are not currently moving the viewport to the form or annotation and so it can be unclear what has been undone. I would appreciate suggestions on whether we should add this viewport shifting and, if so, how best to go about implementing it. This addresses bug 177501. http://bugs.kde.org/show_bug.cgi?id=177501 Diffs - core/document.h d443917 core/document.cpp 2732441 core/documentcommands.cpp 5fcc195 core/documentcommands_p.h a9775a6 ui/formwidgets.h 24108b8 ui/formwidgets.cpp 57ecceb ui/pageview.h 5e839f2 ui/pageview.cpp 6e093ef Diff: http://git.reviewboard.kde.org/r/110589/diff/ Testing --- Manual testing on a variety of PDFs including forms. I've attached three such documents below. File Attachments Mixed forms 1 http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/forms-scribus.pdf http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/formSamples.pdf Exclusive checkboxes http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/stripped-doc.pdf Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Review Request 110589: Undo support for PDF forms
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110589/ --- Review request for Okular. Description --- Add undo / redo support for forms. Along with the previous annotation undo support I believe this completes the implementation of undo/redo for all document editing actions in Okular (Bug 177501). This review request corresponds to the Undo/Redo support in PDF forms feature in the 4.11 feature plan (http://techbase.kde.org/Schedules/KDE4/4.11_Feature_Plan) Potential issue: If the last form or annotation that was modified is outside of the document viewport we are not currently moving the viewport to the form or annotation and so it can be unclear what has been undone. I would appreciate suggestions on whether we should add this viewport shifting and, if so, how best to go about implementing it. This addresses bug 177501. http://bugs.kde.org/show_bug.cgi?id=177501 Diffs - core/document.h d443917 core/document.cpp 2732441 core/documentcommands.cpp 5fcc195 core/documentcommands_p.h a9775a6 ui/formwidgets.h 24108b8 ui/formwidgets.cpp 57ecceb ui/pageview.h 5e839f2 ui/pageview.cpp 6e093ef Diff: http://git.reviewboard.kde.org/r/110589/diff/ Testing --- Manual testing on a variety of PDFs including forms. I've attached three such documents below. File Attachments Mixed forms 1 http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/forms-scribus.pdf http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/formSamples.pdf Exclusive checkboxes http://git.reviewboard.kde.org/media/uploaded/files/2013/05/22/stripped-doc.pdf Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Undo for forms
Greetings, Wanted to let you all know that I'm working on adding undo support to form editing actions in Okular. I already have undo support added for TextAreaEdit and FormLineEdit and I believe I'm close to having ListEdit working as well. I ran into an issue with CheckBoxEdit and RadioButtonEdit. There seems to be handling in the code for exclusive groups of check boxes or radio buttons (Where only one in the group may be checked at a time). However, I've been unable to find a PDF in which this exclusive group behavior actually works in Okular. See, for example, the radio buttons in the PDF at http://kb.nitropdf.com/attachments/GroupingRadioButtons-GUIDd4da0dd6fe1942b1a7f1f775f1b7852b.pdf. The top set of radio buttons act exclusively in Google Chrome and Acrobat but not in Okular. Is this because Okular doesn't support javascript yet? Or do we expect this situation to be handled by Okular's button grouping logic? Are there any other PDFs where this exclusive behavior works in Okular that I could use for undo testing? Or is this grouping an old broken feature that should be removed? I'm hoping to get a patch up onto the review board this weekend including undo for all of the other form types. If I can manage this might there be time to try to get it in for 4.11? Thanks! -Jon . ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110391: Fix for bug 319442
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110391/ --- (Updated May 13, 2013, 11:06 p.m.) Review request for Okular. Changes --- Updates based on Fabio's comments Description --- Fixed bug 319442 by removing the annotation's inplaceText and window text attributes and replacing them both with the unified usage of the contents attribute. See discussion with Fabio in bug report and https://git.reviewboard.kde.org/r/107442/. The inplaceText and window text properties will still be read from saved XML files for backwards compatibility, however they are now used to set the annotation's contents property. This addresses bug 319442. http://bugs.kde.org/show_bug.cgi?id=319442 Diffs (updated) - core/annotations.h 4e9082e core/annotations.cpp 72ca8b5 core/document.cpp 2732441 generators/djvu/generator_djvu.cpp bc83ed7 generators/poppler/annots.cpp b7fb9f7 ui/pagepainter.cpp 950be03 ui/pageviewannotator.cpp 4615d1c Diff: http://git.reviewboard.kde.org/r/110391/diff/ Testing --- Tested creating inline note annotations and the bug no longer occurs. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110391: Fix for bug 319442
On May 13, 2013, 6:35 p.m., Fabio D'Urso wrote: Hi Jon! The patch looks good to me. Here are a few minor notes Thanks Fabio! - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110391/#review32452 --- On May 13, 2013, 11:06 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110391/ --- (Updated May 13, 2013, 11:06 p.m.) Review request for Okular. Description --- Fixed bug 319442 by removing the annotation's inplaceText and window text attributes and replacing them both with the unified usage of the contents attribute. See discussion with Fabio in bug report and https://git.reviewboard.kde.org/r/107442/. The inplaceText and window text properties will still be read from saved XML files for backwards compatibility, however they are now used to set the annotation's contents property. This addresses bug 319442. http://bugs.kde.org/show_bug.cgi?id=319442 Diffs - core/annotations.h 4e9082e core/annotations.cpp 72ca8b5 core/document.cpp 2732441 generators/djvu/generator_djvu.cpp bc83ed7 generators/poppler/annots.cpp b7fb9f7 ui/pagepainter.cpp 950be03 ui/pageviewannotator.cpp 4615d1c Diff: http://git.reviewboard.kde.org/r/110391/diff/ Testing --- Tested creating inline note annotations and the bug no longer occurs. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Review Request 110391: Fix for bug 319442
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110391/ --- Review request for Okular. Description --- Fixed bug 319442 by removing the annotation's inplaceText and window text attributes and replacing them both with the unified usage of the contents attribute. See discussion with Fabio in bug report and https://git.reviewboard.kde.org/r/107442/. The inplaceText and window text properties will still be read from saved XML files for backwards compatibility, however they are now used to set the annotation's contents property. This addresses bug 319442. http://bugs.kde.org/show_bug.cgi?id=319442 Diffs - core/annotations.h 4e9082e core/annotations.cpp 72ca8b5 core/document.cpp 2732441 generators/djvu/generator_djvu.cpp bc83ed7 generators/poppler/annots.cpp b7fb9f7 ui/pagepainter.cpp 950be03 ui/pageviewannotator.cpp 4615d1c Diff: http://git.reviewboard.kde.org/r/110391/diff/ Testing --- Tested creating inline note annotations and the bug no longer occurs. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110391: Fix for bug 319442
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110391/ --- (Updated May 11, 2013, 7:48 p.m.) Review request for Okular. Description --- Fixed bug 319442 by removing the annotation's inplaceText and window text attributes and replacing them both with the unified usage of the contents attribute. See discussion with Fabio in bug report and https://git.reviewboard.kde.org/r/107442/. The inplaceText and window text properties will still be read from saved XML files for backwards compatibility, however they are now used to set the annotation's contents property. This addresses bug 319442. http://bugs.kde.org/show_bug.cgi?id=319442 Diffs - core/annotations.h 4e9082e core/annotations.cpp 72ca8b5 core/document.cpp 2732441 generators/djvu/generator_djvu.cpp bc83ed7 generators/poppler/annots.cpp b7fb9f7 ui/pagepainter.cpp 950be03 ui/pageviewannotator.cpp 4615d1c Diff: http://git.reviewboard.kde.org/r/110391/diff/ Testing --- Tested creating inline note annotations and the bug no longer occurs. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] [okular] [Bug 319442] Initial contents of Inline Note annotation discarded
https://bugs.kde.org/show_bug.cgi?id=319442 --- Comment #4 from Jon Mease jon.me...@gmail.com --- Thanks for your thoughts Fabio. Proposed fix at https://git.reviewboard.kde.org/r/110391/ I removed the inplaceText and window text attributes (along with getters and setters) and replaced all usages with (get/set)contents. -- 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] [okular] [Bug 319442] New: Initial contents of Inline Note annotation discarded
https://bugs.kde.org/show_bug.cgi?id=319442 Bug ID: 319442 Summary: Initial contents of Inline Note annotation discarded Classification: Unclassified Product: okular Version: 0.16.60 Platform: Compiled Sources OS: Linux Status: UNCONFIRMED Severity: normal Priority: NOR Component: general Assignee: okular-devel@kde.org Reporter: jon.me...@gmail.com The initial contents of an Inline Note annotation are not preserved when the note 's contents are later edited. Reproducible: Always Steps to Reproduce: 1. Create an inline annotation 2. Enter some initial text in the popup window (The inline note will be created with these contents). 3. Double click the newly created inline note 4. Observe what text the popup annotation window is populated with Actual Results: The popup window's text is empty Expected Results: The popup window should be populated with the initial text entered in step (2) above It looks like the problem is that when the inline note annotation is created the initial text is set using Annotation::setInplaceText (Reference 1), however the annotation popup window is populated using the annotation's contents (Reference 2). When the undo/redo functionality was added the intention was to do away with the distinction between an annotation's contents and inplaceText (See discussion in https://git.reviewboard.kde.org/r/107442/), but this case was not addressed. Proposal: Remove m_inplaceText member from annotationPrivate but remap calls to Annotation::(set)inPlaceText to calls to Annotation::(set)contents for backwards compatibility. While investigating I noticed that the function in Reference 3 should also be cleaned up when this is fixed. References: 1) PickPointEngine::end in pageviewannotator.cpp 2) AnnotWindow::AnnotWindow 3) DocumentPrivate::performSetAnnotationContents in document_p.cpp -- 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] [okular] [Bug 319442] Initial contents of Inline Note annotation discarded
https://bugs.kde.org/show_bug.cgi?id=319442 Jon Mease jon.me...@gmail.com changed: What|Removed |Added CC||fabiodu...@hotmail.it, ||jon.me...@gmail.com -- 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] [okular] [Bug 319442] Initial contents of Inline Note annotation discarded
https://bugs.kde.org/show_bug.cgi?id=319442 --- Comment #1 from Jon Mease jon.me...@gmail.com --- Fabio, what do you think of the proposal above? -- 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] Review Request 110229: Must rotate annotation to match page when setting annotation's properties (Bug 318828)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110229/ --- Review request for Okular. Description --- Proposed fix for bug 318828 along with associated unit test This addresses bug 318828. http://bugs.kde.org/show_bug.cgi?id=318828 Diffs - core/annotations.cpp d749bd7 tests/modifyannotationpropertiestest.cpp af35c64 Diff: http://git.reviewboard.kde.org/r/110229/diff/ Testing --- Added unit test ModifyAnnotationPropertiesTest::testModifyAnnotationPropertiesWithRotation_Bug318828 Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] [okular] [Bug 318828] Wrong ObjectRect after modifying annotation properties in a rotated document
https://bugs.kde.org/show_bug.cgi?id=318828 --- Comment #5 from Jon Mease jon.me...@gmail.com --- Proposed fix and unit test submitted for review https://git.reviewboard.kde.org/r/110229/ -- 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] [okular] [Bug 318828] Wrong ObjectRect after modifying annotation properties in a rotated document
https://bugs.kde.org/show_bug.cgi?id=318828 --- Comment #4 from Jon Mease jon.me...@gmail.com --- Sure thing. May be a few days but I'll take a look soon. -- 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] Review Request 109989: Fixes bug 318091 and adds undo unit tests
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109989/ --- Review request for Okular. Description --- Fixes bug 318091 (Undo / Redo of annotation creation doesn't properly handle page rotation) and adds associated unit test. Also adds a plethora of unit tests covering all of the new annotation undo commands (https://git.reviewboard.kde.org/r/107442/) This addresses bug 318091. http://bugs.kde.org/show_bug.cgi?id=318091 Diffs - tests/modifyannotationpropertiestest.cpp PRE-CREATION tests/editannotationcontentstest.cpp PRE-CREATION tests/addremoveannotationtest.cpp PRE-CREATION core/page.cpp bb9ae83 tests/CMakeLists.txt 5d1ce2d core/document.cpp 3302b76 tests/testingutils.h PRE-CREATION tests/testingutils.cpp PRE-CREATION tests/translateannotationtest.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/109989/diff/ Testing --- This is testing :-) Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Undo/Redo for annotations merged
Thanks Albert, I think I just missed a couple steps from http://techbase.kde.org/Getting_Started/Build/Environment#Environment_Configuration I set things up from scratch again and everything is working. No excuses left, time to start on some unit tests :-) -Jon I am able to cmake, and make, and make install okular itself but perhaps I'm missing some cmake environment variables for the tests? Nah, that should be enough. From there the Invalid plugin factory for okularGenerator_poppler Looks weird, can you verify that you can open PDF files with that okular you just compiled? Cheers, Albert ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Undo/Redo for annotations merged
Thanks Albert (and Fabio) for all of your help along the way on this. I appreciate it. I do plan to tackle both a) and b) below. Not sure yet which order. I could use a little help getting started with the a). I'm having trouble figuring out how to run okular's existing unit tests. When I run (on master) cmake make buildtests make test I get failures for both parttest and searchtest. When I cd into the tests directory and run parttest on its own here is the output I receive. * Start testing of Okular::PartTest * Config: Using QTest library 4.8.3, Qt 4.8.3 PASS : Okular::PartTest::initTestCase() QDEBUG : Okular::PartTest::testReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testReload() qttest(6457) KXMLGUIClient::setXMLFile: cannot find .rc file part.rc for component qttest QDEBUG : Okular::PartTest::testReload() qttest(6457)/okular (app) Okular::DocumentPrivate::loadGeneratorLibrary: Invalid plugin factory for okularGenerator_poppler! PASS : Okular::PartTest::testReload() QDEBUG : Okular::PartTest::testTOCReload() qttest(6457) Okular::Settings::instance: Settings::instance called after the first use - ignoring QDEBUG : Okular::PartTest::testTOCReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testTOCReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testTOCReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testTOCReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testTOCReload() qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testTOCReload() qttest(6457) KXMLGUIClient::setXMLFile: cannot find .rc file part.rc for component qttest QDEBUG : Okular::PartTest::testTOCReload() qttest(6457)/okular (app) Okular::DocumentPrivate::loadGeneratorLibrary: Invalid plugin factory for okularGenerator_poppler! FAIL! : Okular::PartTest::testTOCReload() Compared values are not the same Actual (part.m_toc-expandedNodes().count()): 0 Expected (3): 3 Loc: [/home/measejm1/Programming/kdeProjects/okular/tests/parttest.cpp(50)] QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457) Okular::Settings::instance: Settings::instance called after the first use - ignoring QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457) KXMLGUIClient::setXMLFile: cannot find .rc file part.rc for component qttest QDEBUG : Okular::PartTest::testFowardPDF(non-utf8) qttest(6457)/okular (app) Okular::DocumentPrivate::loadGeneratorLibrary: Invalid plugin factory for okularGenerator_poppler! FAIL! : Okular::PartTest::testFowardPDF(non-utf8) Compared values are not the same Actual (part.m_document-currentPage()): 4294967295 Expected (0u): 0 Loc: [/home/measejm1/Programming/kdeProjects/okular/tests/parttest.cpp(79)] QDEBUG : Okular::PartTest::testFowardPDF(utf8) qttest(6457) Okular::Settings::instance: Settings::instance called after the first use - ignoring QDEBUG : Okular::PartTest::testFowardPDF(utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(utf8) qttest(6457)/kdecore (KConfigSkeleton) KCoreConfigSkeleton::writeConfig: QDEBUG : Okular::PartTest::testFowardPDF(utf8) qttest(6457) KXMLGUIClient::setXMLFile: cannot find .rc file part.rc for component qttest QDEBUG :
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
On April 3, 2013, 9:16 p.m., Albert Astals Cid wrote: I was about to commit and then i saw we still have the m_prevAnnotTextCursorPos and m_prevAnnotTextAnchorPos maps. Do we really need them? As far as I can see they only get used to fill the commands for the AnnotWindow, can't we cache old those values to feed the command in AnnotWindow itself? Jon Mease wrote: Do you mean have the annotWindow keep track of its own previous cursor and anchor positions and then pass them in as arguments along with the annotation's new contents? If so I believe I already implemented this approach in a previous version of the patch so it won't be hard to dig it back up again. Thanks Albert Astals Cid wrote: Yes, sorry if I made you go back and forth, but the less things we have as caches in document the better, they will get rotten eventually. No problem, I'll make the update in a day or two. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review30337 --- On March 27, 2013, 12:10 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated March 27, 2013, 12:10 p.m.) Review request for Okular. Description --- This patch adds undo/redo support to Okular's 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/guiutils.h 2ae4ab3 ui/guiutils.cpp 1d67d3a 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated April 5, 2013, 12:29 a.m.) Review request for Okular. Changes --- I have removed the QMaps from Document that were used to cache the previous cursor and anchor positions for the contents of all annotations. This responsibility now lies with the annotWindow class. Document::editPageAnnotationContentsOrCursor has been renamed to Document::editPageAnnotationContents as it should now only be called if the contents themselves have changed. The previous cursor and anchor positions have been added to this method signature. I also made a few small updates to EditTextCommand to prevent consecutive edits involving the insertion or deletiion newlines from being merged together into a single undo command. This is to be consistent with the undo behavior of other text editors like Kate. Description --- This patch adds undo/redo support to Okular's 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 (updated) - 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/guiutils.h 2ae4ab3 ui/guiutils.cpp 1d67d3a 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
On April 3, 2013, 9:16 p.m., Albert Astals Cid wrote: I was about to commit and then i saw we still have the m_prevAnnotTextCursorPos and m_prevAnnotTextAnchorPos maps. Do we really need them? As far as I can see they only get used to fill the commands for the AnnotWindow, can't we cache old those values to feed the command in AnnotWindow itself? Do you mean have the annotWindow keep track of its own previous cursor and anchor positions and then pass them in as arguments along with the annotation's new contents? If so I believe I already implemented this approach in a previous version of the patch so it won't be hard to dig it back up again. Thanks - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review30337 --- On March 27, 2013, 12:10 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated March 27, 2013, 12:10 p.m.) Review request for Okular. Description --- This patch adds undo/redo support to Okular's 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/guiutils.h 2ae4ab3 ui/guiutils.cpp 1d67d3a 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated March 27, 2013, 12:10 p.m.) Review request for Okular. Changes --- Error conditions are now checked in both Document::modifyPageAnnotationProperties and Document::prepareToModifyAnnotationProperties. Error message is printed with kError and failure with a Q_ASSERT. I also fixed the handling of latex fragments in annotWindow. Description --- This patch adds undo/redo support to Okular's 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 (updated) - 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/guiutils.h 2ae4ab3 ui/guiutils.cpp 1d67d3a 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
On March 26, 2013, 10:54 p.m., Albert Astals Cid wrote: core/document.cpp, line 3239 http://git.reviewboard.kde.org/r/107442/diff/7/?file=121230#file121230line3239 wondering if we should assert here that d-m_prevPropsOfAnnotBeingModified is null/empty/wathever Jon Mease wrote: Sure, that makes sense. Is a C++ assert statement sufficient? Or would you prefer another failure mechanism? Albert Astals Cid wrote: Q_ASSERT (that will only assert in debug builds, otherwise it's a noop) + if + kWarning like you do in modifyPageAnnotationProperties, also might make sense adding the Q_ASSERT to modifyPageAnnotationProperties Ok, sounds good. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review29929 --- On March 27, 2013, 12:10 p.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated March 27, 2013, 12:10 p.m.) Review request for Okular. Description --- This patch adds undo/redo support to Okular's 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/guiutils.h 2ae4ab3 ui/guiutils.cpp 1d67d3a 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
On March 26, 2013, 10:54 p.m., Albert Astals Cid wrote: core/document.cpp, line 3239 http://git.reviewboard.kde.org/r/107442/diff/7/?file=121230#file121230line3239 wondering if we should assert here that d-m_prevPropsOfAnnotBeingModified is null/empty/wathever Sure, that makes sense. Is a C++ assert statement sufficient? Or would you prefer another failure mechanism? - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review29929 --- On March 24, 2013, 12:40 a.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated March 24, 2013, 12:40 a.m.) Review request for Okular. Description --- This patch adds undo/redo support to Okular's 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/guiutils.h 2ae4ab3 ui/guiutils.cpp 1d67d3a 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated March 24, 2013, 12:40 a.m.) Review request for Okular. Changes --- Updates based on Albert's comments on v6 Description (updated) --- This patch adds undo/redo support to Okular's 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 (updated) - 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/guiutils.h 2ae4ab3 ui/guiutils.cpp 1d67d3a 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
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 *); Fabio D'Urso wrote: 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 Ok, I removed GuiUtils::contents and replaced all usages with plain Annotation::contents. I didn't remove the inplaceText methods and member because these are used in the QDomNode code to store and retrieve annotation properties and I'm not comfortable enough to change these in a backwards compatible way. With this change I was able to get rid of the previous contents map and instead grab the previous contents from the annotation itself. Feel free to re-open this issue if you have any other concerns On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote: ui/annotationpropertiesdialog.cpp, line 209 http://git.reviewboard.kde.org/r/107442/diff/6/?file=119429#file119429line209 Maybe we could not modify the annot in ::slotapply but modify it in Document::modifyPageAnnotationProperties? And then you wouldn't need the map in there either because you would get the old annotation as parameter? Jon Mease wrote: The difficulty here is that most of the work in modifying the annotation's properties is actually performed in the applyChanges() method of the AnnotationWidget (m_annotWidget). And we can't pass the AnnotationWidget into the Document::modifyPageAnnotationProperties method because of the GUI dependency. So I'm not really sure how this could be done (but I'm open to suggestions). Albert Astals Cid wrote: Hmmm, i see, maybe we could go to a two step process? Like m_document-startAnnotModification(); annot-setStuff(); m_document-endAnnotModification(); It's not ultra nice, but I don't really like maintainging caches of stuff, they will eventually get out
Re: [Okular-devel] Review Request 109632: Annotation eraser
On March 22, 2013, 8:19 p.m., Albert Astals Cid wrote: So yeah, besides what Fabio comments on it may well be a bit slow (no way to speed it up unless you change poppler to do two pass rendering that might even be possible) code looks ok *but* it will stop working when the undo/redo annotations code gets merged. I'd suggest you have a look at it and try to find out how you'd apply your changes on top of it. Peter Grasch wrote: I'll hold off until that is committed then. No reason to start complicating things, this is not that urgent. I don't think it will be difficult to rebase your changes onto the new undo functionality (once it's merged) but let me know if you have any questions. - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109632/#review29738 --- On March 21, 2013, 1:26 a.m., Peter Grasch wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109632/ --- (Updated March 21, 2013, 1:26 a.m.) Review request for Okular. Description --- Prerequisites: Please read till the end! This introduces a new annotation tool: Eraser. The eraser primarily erases other annotations that it comes into contact with (shapes, lines, highlights, etc.). However, ink annotations are treated more like a real eraser: Existing paths are split and unaffected parts are preserved. This is what it looks like: http://bedahr.org/eraser.ogv Example tool configuration for your tools.xml (not included in patch): tool id=15 name=Eraser pixmap=tool-eraser-okular tooltipEraser/tooltip engine type=Eraser / shortcut7/shortcut /tool The eraser builds on the work for the outline selection proposed in review request #109627. Please apply that patch before this one. Diffs - ui/pageviewannotator.cpp 7bd7496 Diff: http://git.reviewboard.kde.org/r/109632/diff/ Testing --- While it worked fine for the few PDFs I threw at it on this relatively powerful machine, it was pointed out to me in #okular, that calls to modifyPageAnnotation are very expensive as poppler has to re-draw the pdf (with the annotations) for every change. I hope we can resume the discussion about possible improvements here. Thanks, Peter Grasch ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 109632: Annotation eraser
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109632/#review29779 --- Comments on the functionality (not the implementation): 1) I'm really excited to have this functionality in Okular (My work on the undo functionality was also primarily motivated by the desire to replace Xournal with Okular for tablet PC ink document annotation). 2) How difficult would it be to add something like an erase ink strokes option to the tools.xml. Xournal has this option and it controls whether an ink stroke is fully deleted by the eraser (as you do for the other annotation types) or erased incrementally (your current implementation for ink annotations). I've found both of these settings to be quite helpful under different circumstances. - Jon Mease On March 21, 2013, 1:26 a.m., Peter Grasch wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109632/ --- (Updated March 21, 2013, 1:26 a.m.) Review request for Okular. Description --- Prerequisites: Please read till the end! This introduces a new annotation tool: Eraser. The eraser primarily erases other annotations that it comes into contact with (shapes, lines, highlights, etc.). However, ink annotations are treated more like a real eraser: Existing paths are split and unaffected parts are preserved. This is what it looks like: http://bedahr.org/eraser.ogv Example tool configuration for your tools.xml (not included in patch): tool id=15 name=Eraser pixmap=tool-eraser-okular tooltipEraser/tooltip engine type=Eraser / shortcut7/shortcut /tool The eraser builds on the work for the outline selection proposed in review request #109627. Please apply that patch before this one. Diffs - ui/pageviewannotator.cpp 7bd7496 Diff: http://git.reviewboard.kde.org/r/109632/diff/ Testing --- While it worked fine for the few PDFs I threw at it on this relatively powerful machine, it was pointed out to me in #okular, that calls to modifyPageAnnotation are very expensive as poppler has to re-draw the pdf (with the annotations) for every change. I hope we can resume the discussion about possible improvements here. Thanks, Peter Grasch ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote: Thanks for the feedback. I have a couple questions inline below On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote: ui/annotationpropertiesdialog.cpp, line 209 http://git.reviewboard.kde.org/r/107442/diff/6/?file=119429#file119429line209 Maybe we could not modify the annot in ::slotapply but modify it in Document::modifyPageAnnotationProperties? And then you wouldn't need the map in there either because you would get the old annotation as parameter? The difficulty here is that most of the work in modifying the annotation's properties is actually performed in the applyChanges() method of the AnnotationWidget (m_annotWidget). And we can't pass the AnnotationWidget into the Document::modifyPageAnnotationProperties method because of the GUI dependency. So I'm not really sure how this could be done (but I'm open to suggestions). 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? 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. On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote: core/document.h, line 394 http://git.reviewboard.kde.org/r/107442/diff/6/?file=119424#file119424line394 Do we need the bool? Seems it's only called once with true, no? Yes, it looks like we're only ever setting it to true now. I will get rid of it. On March 14, 2013, 7:43 p.m., Albert Astals Cid wrote: ui/annotwindow.h, line 38 http://git.reviewboard.kde.org/r/107442/diff/6/?file=119430#file119430line38 We don't seem to use it anymore, or maybe we didn't already, but can you kill the annotation() method? Will do - Jon --- 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
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated March 12, 2013, 1:24 a.m.) Review request for Okular. Changes --- Updates based on Albert's review of version 5 of the patch. Description (updated) --- 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 (updated) - 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. 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] Review Request 107442: Add undo/redo support for annotations
On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote: Tried it looks pretty cool :) Some minor comments inline You will need to rebase your patch, there's a small conflict with a change i did in document.cp Thanks a lot for your feedback! These issues will be addressed in version 6 of the patch On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote: core/annotations.h, line 676 http://git.reviewboard.kde.org/r/107442/diff/5/?file=111627#file111627line676 Can you please try moving all these protected/private methods to the private classes instead of to the public ones? Good idea, this actually cleaned up the design a bit On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote: core/document.h, line 405 http://git.reviewboard.kde.org/r/107442/diff/5/?file=111629#file111629line405 I'd prefer if we passed the newprops instead of the old ones in here so if you can fix editPageAnnotationContents to only need the new ones too, the three functions will be getting the new data we want to apply. Making it easier if someone (like the Okular Active guys) wants to use these functions I modified this method to not require the old properties by adding a map to the DocumentPrivate class to keep track of the old properties. The new properties are extracted from the annotation passed to the method. On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote: core/document.h, line 428 http://git.reviewboard.kde.org/r/107442/diff/5/?file=111629#file111629line428 Do we really need to pass new and old data here? Shouldn't we be able to get one of the two from annotation? Modified this method to only input the new contents, cursor position, and anchor position by adding corresponding maps to the DocumentPrivate class to keep track of the old quantities. On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote: core/document.cpp, line 2248 http://git.reviewboard.kde.org/r/107442/diff/5/?file=111630#file111630line2248 You can just do emit m_docPriv-m_parent-annotationContentsBlaBla, no need for the intermediate function call Thanks for the tip. Looks like emitting another class's signal is equivalent to calling a protected method on the class so I needed to make EditAnnotationContentsCommand a friend of Document to make this work. On March 7, 2013, 3:59 p.m., Albert Astals Cid wrote: core/document.cpp, line 2126 http://git.reviewboard.kde.org/r/107442/diff/5/?file=111630#file111630line2126 nitpick, extra space at the end (or missing at the beginning :D not sure what is the file style) Also did some general parenthesis spacing cleanup of this section - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/#review28765 --- On March 12, 2013, 1:24 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:24 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. 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] Review Request 107442: Add undo/redo support for annotations
--- 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. Changes --- Removed old known issue from the Testing Done description 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 (updated) --- 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
[Okular-devel] Undo / Redo questions
Greetings, For the past several months I've been (slowly) working on adding undo/redo support to Okular. I believe I have everything working to undo/redo all actions related to annotations (See https://git.reviewboard.kde.org/r/107442/). My first question is whether this functionality is sufficient for inclusion in Okular or whether we should wait until I have time to add undo support for forms as well. I do hope I'll have time to work on adding form support in the not so distant future. Does anyone have a good sample PDF file that includes all of the form controls supported by Okular for me to test with? Thanks! -Jon ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 107442: Add undo/redo support for annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107442/ --- (Updated Feb. 2, 2013, 6:58 p.m.) Review request for Okular. Changes --- This update utilizes the KTextEdit::aboutToShowContextMenu signal to replace the default undo and redo actions in the context menu of the AnnotWindow with the central undo and redo actions on the document. This resolves the final issue that I am aware of related to the undo/redo functionality on annotations. There is still no support for undo/redo on forms 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 (updated) - ui/pageview.cpp 60a273d ui/annotwindow.cpp c1bafb9 ui/annotwindow.h f7df9f6 part.rc 39c1571 ui/annotationpropertiesdialog.cpp 4b02258 core/page.cpp 4df58e0 core/document.cpp 372af56 core/document_p.h 57a3bee core/document.h 1d825e1 core/annotations.h 72abdff core/annotations.cpp 49ab5bd 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] 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
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/ --- (Updated Dec. 1, 2012, 9:33 p.m.) Review request for Okular. Changes --- Update rebased to master and incorporating most of Fabio's suggestions. Outstanding Issues: - Annotation still has a reference to a QTextEdit*. Right now the EditAnnotationContentsCommand holds on to a QTextEdit and delegates undo/redo commands to the QTextEdit. I took this approach in order to leverage QTextEdit's ability to group text editing commands together (so that we're not always undoing the insertion and deletion of individual characters). This approach also preserves the proper cursor position in the kTextEdit when a user calls undo while editing in the AnnotWindow. However, I now see that this approach may be a no-go as it requires a GUI QTextEdit widget in order to edit and undo the contents of an annotation. I would appreciate suggestions on the matter. - I have not done any further work on the .pdf issue described previously. - No undo support for forms 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 (updated) - 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] Review Request: Add undo/redo support for annotations
On Nov. 28, 2012, 12:04 a.m., Fabio D'Urso wrote:
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.
Thanks a lot for your helpful feedback! I'll fix the merge conflict and
incorporate your comments soon.
- I understand now why Annotation can't have a reference to KTextEdit but I
need somewhere to store an instance of KTextEdit for the life of an annotation
(so that the undo stack of the KTextEdit instance persists after its
annotWindow is closed). Do you have any recommendations for where this
KTextEdit reference could live?
- The reason I clear the undo stack in page.cpp is so that when opening a
document with existing saved annotations, it's not possible to undo the act of
adding those saved annotations to the document. Does that make sense?
Thanks also for the overview of how PDF annotations are handled differently.
I'll keep this in mind as I take another shot at figuring out the source of my
rendering issue.
- Jon
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107442/#review22677
---
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
Re: [Okular-devel] An outline and some questions regarding undo framework
Thanks for the feedback Albert. I think I will proceed with the plan of utilizing the XML code to store/load annotation state information. And I'll plan on working to get this in for 4.11. Please forgive my ignorance of the development process, but when would be a good time to submit a review request for 4.11? I totally agree that form data should be included in the undo framework, it just hadn't crossed my mind. Are there any other okular operations that should be integrated as well? -Jon ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] An outline and some questions regarding undo framework
Greetings, So I've spent a good bit of time studying the code, planning, and prototyping ideas for the undo framework and I wanted to lay them out here for general discussion. Location of the QUndoStack: My current thinking is that the undo stack should be a member of DocumentPrivate. Methods on Document that currently directly alter the state of the annotations in the document should be moved from Document to DocumentPrivate. These methods include addPageAnnotation, removePageAnnotation(s), and modifyPageAnnotation. New methods on Document should be added (corresponding to those listed above) that instantiate appropriate QUndoCommands and push them to the document's undo stack. The QUndoCommands, in turn, will make use of the private methods that actually alter the documents annotations. Three QUndoCommands need to be written: addPageAnnotationCommand, removePageAnnotationCommand, and modifyPageAnnotationCommand. The first two have turned out to be fairly simple (Based on the approach Albert suggested in my now discarded review request from a few days ago) and I have fully functional prototypes written of both. The trickiest part has been working out how to implement the modifyPageAnnotationCommand. We have lots of styles of annotations each with lots of individual properties and so it seems pretty impractical to me to create individual modification undo commands for each annotation type and each property. What would be ideal would be to have some kind of annotation property descriptor object that would capture all of the state of a particular annotation. This way we could call getPropertyDescriptor on the annotation before and after modifying it and have the modification undo command hold on to these two descriptors. Then the undo and redo actions would simply call setPropertyDescriptor() on the annotation object with the appropriate property descriptor object to toggle its state back and forth. But then it occurred to me that storing and loading an annotation's state in xml is a very similar problem. We currently have a store() method on the Annotation class that stores the annotations state to a QDomNode. We also have constructors for each Annotation subclass that input a QDomNode and use it to initialize their state. In a sense, these QDomNodes could be thought of as an annotation's property descriptor object as described above. It's straight forward to write a little getPropertyDescriptorDom method on the Annotation class that uses store() to return a QDomNode containing the annotation's state. Using this method the modifyPageAnnotation undo command could hold onto two QDomNodes representing the before and after states of an annotation. The undo and redo actions would then use the appropriate QDomNode to update the annotations internal state. How does everyone feel about this general approach of leveraging the xml storage logic and using QDomNodes to represent an annotation's internal state? I really like that the undo framework would not be dependent upon all of the individual annotation subclasses and their properties. Instead it would only be dependent upon a working xml storage/loading capability. If this general approach makes sense (of using QDomNodes to represent an annotation's state) then the last remaining challenge is to figure out how to use a QDomNode to update the state of an existing annotation. Currently the logic for parsing the QDomNode and setting up an annotation's state is located in one of the constructors of each annotation type. We could refactor this logic (for each annotation type) out of the constructor and into a standard method called something like setAnnotationStateFromDomNode(). This way the logic to set an annotation's state based on a QDomNode is available to both the constructor and to the modifyPageAnnotation undo command. I've implemented a rough version of this approach and it works pretty well overall. The only issue is that the QDomNode parsing code in the constructors of some of the annotation types assume that their working with freshly initialized instantiations of the private member variables. For example, the InkAnnotationPrivate class has a couple of lists as members that are added to by the InkAnnotation constructor without being cleared first. So in order to be able to use a setAnnotationStateFromDomNode() method we'd have to make some of that initialization explicit. One other question I have is where should the Undo and Redo Actions live? Right now for prototyping I've added them to the PageView's action collection so that they can be added to the toolbar for testing purposes. Should Undo and Redo show up in Edit menu? If so, any thoughts on how to get them there? Ok, that's enough for now. I would appreciate feedback on all of the above. I would also appreciate feedback on whether it would be feasible to try to get this in for 4.10. Depending the feedback I get I may be able to have something ready to review
[Okular-devel] Review Request: Manage annotation objects with QSharedPointers to support future Undo/Redo framework
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106923/ --- Review request for Okular. Description --- The purpose of this patch is to shift the responsibility for explicitly deleting annotations away from the Page class. Prior to this patch the Page class would explicitly delete an annotations object that was removed from the page. However, in order to eventually support Undoing and Redoing the creation and removal of annotations from a page it is important that the annotation objects are not automatically deleted upon removal. The Page class now stores QSharedPointers to annotations rather than standard pointers. Page::addAnnotation and Document::addPageAnnotation now require a QSharedPointer rather than a standard pointer. The implication is that the deletion of annotation objects is now managed by the QSharedPointer class rather than the Page class. Currently the only QSharedPointer referencing a particular annotation will be the one help by the page class and so the removal of the annotaion from a page will automatically result in the deletion of the annotation object. However, in the future several QUndoCommands may also have QSharedPointer references to the annotation and in this case the annotation will not be deleted until all of those QUndoCommands have been been destroyed. Diffs - core/annotations.cpp 21114af core/document.h d47acec core/document.cpp bddef39 core/page.h a8f2761 core/page.cpp d746382 core/textdocumentgenerator.cpp f370ded generators/djvu/generator_djvu.cpp bc83ed7 generators/poppler/generator_pdf.cpp fcc8dc4 ui/annotationtools.h fdcae1b ui/annotationtools.cpp f2db355 ui/pagepainter.cpp 916a0ab ui/pageviewannotator.cpp 4488639 Diff: http://git.reviewboard.kde.org/r/106923/diff/ Testing --- Added a kDebug() statement to the destructor of the Annotation class. For both annotations loaded from disk and annotations created during the current session the removal of the annotation from a page still results in the destruction of the annotation object (no annotation memory leak). This is the same behavior as before, only now it is accomplished through the use of QSharedPointers rather than an explicit delete call. Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] An Undo / Redo framework for annotations
So I started working on the Undo/Redo framework and the first hurdle to work out is memory management. Right now when an annotation is removed from a page it is automatically deleted (see Page::removeAnnotation). However if we want to be able to undo this removal we obviously don't want to delete the annotation object right away. The solution that jumped out at me was to have the page class use QSharedPointers to reference annotations. This way the page class just needs to make sure its shared pointer goes out of scope when an annotation is removed and the QSharedPointer class is responsible for actually deleting the annotation when it's no longer being referenced my any shared pointer. This way we can soon have a bunch of QUndoCommands on the undo stack that reference the removed annotation and the annotation won't be deleted until the annotation is both removed from a page and all of these undo commands are deleted from the undo stack. I've submitted a review request that contains only this memory management change (https://git.reviewboard.kde.org/r/106923/) but I wanted to explain a little more of my reasoning here. Let me know what you think! -Jon On Sun, Oct 14, 2012 at 12:16 PM, Fabio D'Urso fabiodu...@hotmail.it wrote: On Saturday, October 13, 2012 11:43:38 PM Jon Mease wrote: Hello all, Hi, My next goal for enhancing Okular's annotation framework is to add undo / redo support when creating/editing/deleting annotations. Looking back at the list archives it seems like this is something that has been asked for and mentioned in bug reports for several years now. Before starting in I wanted to ask whether anyone has already started work on this. It's still unexplored land :D No one has worked on this yet I have some experience with Qt's Undo/Redo framework and this is what I'm planning on using. At the highest level my thought is to write QUndoCommands for each of the following annotation actions 1) Creating a new annotation with a particular set of properties 2) Deleting an existing annotation 3) Editing an annotation's properties (color, opacity, etc) 4) Moving an annotation's position 5) Editing text in a text annotation (this would rely on the undoAvailable signal of QTextEdit) Sounds mostly correct to me. You can join #okular on freenode to discuss your ideas live with other developers. Does anyone have any thoughts on the subject? Does an existing undo/redo branch or patch exists somewhere? If not, I'll probably get started on one this coming week. Great! Keep us informed ;) Thank you, Fabio ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request: Support high precision Wacom input for creating annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106816/ --- (Updated Oct. 15, 2012, 12:31 a.m.) Review request for Okular. Changes --- 1) Cleaned up patch based on Albert's comments. 2) Reworked PageViewAnnotator::routeTabletEvent slightly. Now all tablet events over the annotations toolbar are set to ignore so that the toolbar will receive the corresponding mouse event. However, the annotation code still handles TabletMove and TabletRelease events in case an annotation has been drawn onto the toolbar itself. Description --- This patch adds processing of QTabletEvents to the PageView class. Basically a graphics tablet will behave exactly like a mouse except while creating an annotation. When creating an annotation, the higher precision position of the QTabletEvent is used and this results in smoother free-hand annotations (See the attached image for a before and after comparison). With this patch in place it's possible to create a nice looking signature in an Okular document using a graphics tablet. Diffs (updated) - ui/annotationtools.h 7107042 ui/annotationtools.cpp 40fa6fe ui/pageview.h c1b36f4 ui/pageview.cpp 2ef10a1 ui/pageviewannotator.h a2ef90d ui/pageviewannotator.cpp 9754b5e Diff: http://git.reviewboard.kde.org/r/106816/diff/ Testing --- * The creation of annotations using the wacom pen (smooth freehand annotations) * The creation of annotations using the mouse (no change) * The operation of the annotations menu using both the mouse and the wacom pen (pen behaves just like the mouse). Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request: Support high precision Wacom input for creating annotations
On Oct. 14, 2012, 5:09 p.m., Albert Astals Cid wrote: ui/pageview.cpp, line 1728 http://git.reviewboard.kde.org/r/106816/diff/1/?file=89319#file89319line1728 statics are evil, do you really need penDown to be static? penDown is now member of PageViewPrivate class - Jon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106816/#review20318 --- On Oct. 15, 2012, 12:31 a.m., Jon Mease wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106816/ --- (Updated Oct. 15, 2012, 12:31 a.m.) Review request for Okular. Description --- This patch adds processing of QTabletEvents to the PageView class. Basically a graphics tablet will behave exactly like a mouse except while creating an annotation. When creating an annotation, the higher precision position of the QTabletEvent is used and this results in smoother free-hand annotations (See the attached image for a before and after comparison). With this patch in place it's possible to create a nice looking signature in an Okular document using a graphics tablet. Diffs - ui/annotationtools.h 7107042 ui/annotationtools.cpp 40fa6fe ui/pageview.h c1b36f4 ui/pageview.cpp 2ef10a1 ui/pageviewannotator.h a2ef90d ui/pageviewannotator.cpp 9754b5e Diff: http://git.reviewboard.kde.org/r/106816/diff/ Testing --- * The creation of annotations using the wacom pen (smooth freehand annotations) * The creation of annotations using the mouse (no change) * The operation of the annotations menu using both the mouse and the wacom pen (pen behaves just like the mouse). Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] An Undo / Redo framework for annotations
Hello all, My next goal for enhancing Okular's annotation framework is to add undo / redo support when creating/editing/deleting annotations. Looking back at the list archives it seems like this is something that has been asked for and mentioned in bug reports for several years now. Before starting in I wanted to ask whether anyone has already started work on this. I have some experience with Qt's Undo/Redo framework and this is what I'm planning on using. At the highest level my thought is to write QUndoCommands for each of the following annotation actions 1) Creating a new annotation with a particular set of properties 2) Deleting an existing annotation 3) Editing an annotation's properties (color, opacity, etc) 4) Moving an annotation's position 5) Editing text in a text annotation (this would rely on the undoAvailable signal of QTextEdit) Does anyone have any thoughts on the subject? Does an existing undo/redo branch or patch exists somewhere? If not, I'll probably get started on one this coming week. Thanks, -Jon ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
[Okular-devel] Review Request: Support high precision Wacom input for creating annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106816/ --- Review request for Okular. Description --- This patch adds processing of QTabletEvents to the PageView class. Basically a graphics tablet will behave exactly like a mouse except while creating an annotation. When creating an annotation, the higher precision position of the QTabletEvent is used and this results in smoother free-hand annotations (See the attached image for a before and after comparison). With this patch in place it's possible to create a nice looking signature in an Okular document using a graphics tablet. Diffs - ui/annotationtools.h 7107042 ui/annotationtools.cpp 40fa6fe ui/pageview.h c1b36f4 ui/pageview.cpp 2ef10a1 ui/pageviewannotator.h a2ef90d ui/pageviewannotator.cpp 9754b5e Diff: http://git.reviewboard.kde.org/r/106816/diff/ Testing --- * The creation of annotations using the wacom pen (smooth freehand annotations) * The creation of annotations using the mouse (no change) * The operation of the annotations menu using both the mouse and the wacom pen (pen behaves just like the mouse). Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request: Support high precision Wacom input for creating annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106816/ --- (Updated Oct. 12, 2012, 10:03 p.m.) Review request for Okular. Description --- This patch adds processing of QTabletEvents to the PageView class. Basically a graphics tablet will behave exactly like a mouse except while creating an annotation. When creating an annotation, the higher precision position of the QTabletEvent is used and this results in smoother free-hand annotations (See the attached image for a before and after comparison). With this patch in place it's possible to create a nice looking signature in an Okular document using a graphics tablet. Diffs - ui/annotationtools.h 7107042 ui/annotationtools.cpp 40fa6fe ui/pageview.h c1b36f4 ui/pageview.cpp 2ef10a1 ui/pageviewannotator.h a2ef90d ui/pageviewannotator.cpp 9754b5e Diff: http://git.reviewboard.kde.org/r/106816/diff/ Testing --- * The creation of annotations using the wacom pen (smooth freehand annotations) * The creation of annotations using the mouse (no change) * The operation of the annotations menu using both the mouse and the wacom pen (pen behaves just like the mouse). Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request: Support high precision Wacom input for creating annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106816/ --- (Updated Oct. 12, 2012, 10:03 p.m.) Review request for Okular. Description --- This patch adds processing of QTabletEvents to the PageView class. Basically a graphics tablet will behave exactly like a mouse except while creating an annotation. When creating an annotation, the higher precision position of the QTabletEvent is used and this results in smoother free-hand annotations (See the attached image for a before and after comparison). With this patch in place it's possible to create a nice looking signature in an Okular document using a graphics tablet. Diffs - ui/annotationtools.h 7107042 ui/annotationtools.cpp 40fa6fe ui/pageview.h c1b36f4 ui/pageview.cpp 2ef10a1 ui/pageviewannotator.h a2ef90d ui/pageviewannotator.cpp 9754b5e Diff: http://git.reviewboard.kde.org/r/106816/diff/ Testing --- * The creation of annotations using the wacom pen (smooth freehand annotations) * The creation of annotations using the mouse (no change) * The operation of the annotations menu using both the mouse and the wacom pen (pen behaves just like the mouse). Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request: Support high precision Wacom input for creating annotations
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106816/ --- (Updated Oct. 12, 2012, 10:03 p.m.) Review request for Okular. Description --- This patch adds processing of QTabletEvents to the PageView class. Basically a graphics tablet will behave exactly like a mouse except while creating an annotation. When creating an annotation, the higher precision position of the QTabletEvent is used and this results in smoother free-hand annotations (See the attached image for a before and after comparison). With this patch in place it's possible to create a nice looking signature in an Okular document using a graphics tablet. Diffs - ui/annotationtools.h 7107042 ui/annotationtools.cpp 40fa6fe ui/pageview.h c1b36f4 ui/pageview.cpp 2ef10a1 ui/pageviewannotator.h a2ef90d ui/pageviewannotator.cpp 9754b5e Diff: http://git.reviewboard.kde.org/r/106816/diff/ Testing --- * The creation of annotations using the wacom pen (smooth freehand annotations) * The creation of annotations using the mouse (no change) * The operation of the annotations menu using both the mouse and the wacom pen (pen behaves just like the mouse). Thanks, Jon Mease ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
