Re: [Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color
On July 25, 2013, 12:59 p.m., Jaydeep Solanki wrote: Can you please confirm if the links are shown properly Albert Astals Cid wrote: Christoph? Oh, I had the impression the question was directed to you (I looked at the To: field in the mail) The link color now works as intended, but I only have this one test file (see Testing above). - Christoph --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/#review36480 --- On July 25, 2013, noon, Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/ --- (Updated July 25, 2013, noon) Review request for Okular and Albert Astals Cid. Description --- As indicated in bug 322547, some documents do not specify a text color, and probably assume the default text color to be black. QTextDocument, however, defaults to using the system text color. This patch changes the default text color to Qt::black. It should affect epub, fb2, odt, and plain text generators. I think it is better to use this approach instead of changing the paper color to use the system background color (see bug 253583), because 1) the document might specify a text color in some places, 2) the user is able to change the fg/bg colors anyway using Okular's Accessibility options, and those probably expect black on white. This addresses bugs 253583 and 322547. http://bugs.kde.org/show_bug.cgi?id=253583 http://bugs.kde.org/show_bug.cgi?id=322547 Diffs - core/textdocumentgenerator.cpp b260b3f Diff: http://git.reviewboard.kde.org/r/111681/diff/ Testing --- I tested the document from bug 322547 comment #3. Thanks, Christoph Feck ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Dt. 9th August - status
How it occurs : 1) Open an ePub 2) As soon as it opens, hold Right arrow key, till it reaches the end of document or crashes another way would be 2) Hold PgUp/ PgDn key, till it reaches the end of document or crashes. *Note:* it doesn't always crash. In fact the probability of it crashing is very low, thus it gets difficult to debug. On Tue, Aug 13, 2013 at 3:17 AM, Albert Astals Cid aa...@kde.org wrote: El Dissabte, 10 d'agost de 2013, a les 01:19:14, Jaydeep Solanki va escriure: Hi, I have tested yesterday's patch (without lock) with about 15 different epubs, and it crashed only once. I tried the same epub that made it crash once a few more times, but it didn't crash. Thus, crash doesn't depend on the epub. I observed a few things, it crashes only when I'm scrolling the pages at super fast speed (like pressing PgDn or right arrow key), this tells that generating pixmaps fast makes it break, so I set the performance to Greedy, which makes Okular generate all the pixmaps as soon as possible, but it never crashes there! The error that it gave me when crash occurred was memory corrupted, so I tried valgrind, but it didn't detect any invalid memory. May be whenever you get time give it a try. Let's see if it crashes with you. Can you please exactly tell me what you are doing and describe the crash you are getting? Cheers, Albert Cheers, Jaydeep ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110003: Best-fit zoom
On Aug. 14, 2013, 9:55 p.m., Albert Astals Cid wrote: Second, I am trying to add Auto Fit as one of the default zoom settings in the configuration dialog. Although I was quite sure I did not miss a spot and the option turns up in the settings dialog, choosing Auto Fit does not get activated when restarting Okular. Any hints what goes wrong? You aware this setting only applies for files you've never opened? Is this your problem? Indeed. I tested it with another file I had never opened before and AutoFit was active as expected. So, is there anything left or is my patch good to be accepted? - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110003/#review37805 --- On July 7, 2013, 11:04 p.m., Thomas Fischer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110003/ --- (Updated July 7, 2013, 11:04 p.m.) Review request for Okular. Description --- Attached patch implements best-fit zoom for Okular. It is a refined version of the patch submitted in bug report 249364, attachment 51069. The refinement addresses the scrollbar issues as observed in continuous view mode. This addresses bug 249364. http://bugs.kde.org/show_bug.cgi?id=249364 Diffs - conf/dlggeneralbase.ui f2c9efd conf/okular.kcfg 1e23d61 part.rc 64aeffb ui/pageview.h 5484cc5 ui/pageview.cpp 16b00ab Diff: http://git.reviewboard.kde.org/r/110003/diff/ Testing --- Thanks, Thomas Fischer ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Dt. 9th August - status
El Divendres, 16 d'agost de 2013, a les 22:29:38, Jaydeep Solanki va escriure: How it occurs : 1) Open an ePub 2) As soon as it opens, hold Right arrow key, till it reaches the end of document or crashes another way would be 2) Hold PgUp/ PgDn key, till it reaches the end of document or crashes. *Note:* it doesn't always crash. In fact the probability of it crashing is very low, thus it gets difficult to debug. Have you tried with the helgrind tool of valgrind? Cheers, Albert On Tue, Aug 13, 2013 at 3:17 AM, Albert Astals Cid aa...@kde.org wrote: El Dissabte, 10 d'agost de 2013, a les 01:19:14, Jaydeep Solanki va escriure: Hi, I have tested yesterday's patch (without lock) with about 15 different epubs, and it crashed only once. I tried the same epub that made it crash once a few more times, but it didn't crash. Thus, crash doesn't depend on the epub. I observed a few things, it crashes only when I'm scrolling the pages at super fast speed (like pressing PgDn or right arrow key), this tells that generating pixmaps fast makes it break, so I set the performance to Greedy, which makes Okular generate all the pixmaps as soon as possible, but it never crashes there! The error that it gave me when crash occurred was memory corrupted, so I tried valgrind, but it didn't detect any invalid memory. May be whenever you get time give it a try. Let's see if it crashes with you. Can you please exactly tell me what you are doing and describe the crash you are getting? Cheers, Albert Cheers, Jaydeep ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110003: Best-fit zoom
On Aug. 14, 2013, 9:55 p.m., Albert Astals Cid wrote: Second, I am trying to add Auto Fit as one of the default zoom settings in the configuration dialog. Although I was quite sure I did not miss a spot and the option turns up in the settings dialog, choosing Auto Fit does not get activated when restarting Okular. Any hints what goes wrong? You aware this setting only applies for files you've never opened? Is this your problem? Thomas Fischer wrote: Indeed. I tested it with another file I had never opened before and AutoFit was active as expected. So, is there anything left or is my patch good to be accepted? Looks ok from the code POV (well you need to update part.rc version and adding a few const to variables that never change like uiAspect/pageAspect/etc would be cool) What I would like you is editing the docbook manual to explain the new zoom mode. Can you do that? - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110003/#review37805 --- On July 7, 2013, 11:04 p.m., Thomas Fischer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110003/ --- (Updated July 7, 2013, 11:04 p.m.) Review request for Okular. Description --- Attached patch implements best-fit zoom for Okular. It is a refined version of the patch submitted in bug report 249364, attachment 51069. The refinement addresses the scrollbar issues as observed in continuous view mode. This addresses bug 249364. http://bugs.kde.org/show_bug.cgi?id=249364 Diffs - conf/dlggeneralbase.ui f2c9efd conf/okular.kcfg 1e23d61 part.rc 64aeffb ui/pageview.h 5484cc5 ui/pageview.cpp 16b00ab Diff: http://git.reviewboard.kde.org/r/110003/diff/ Testing --- Thanks, Thomas Fischer ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/ --- (Updated Aug. 16, 2013, 8:58 p.m.) Review request for Okular. Description --- As indicated in bug 322547, some documents do not specify a text color, and probably assume the default text color to be black. QTextDocument, however, defaults to using the system text color. This patch changes the default text color to Qt::black. It should affect epub, fb2, odt, and plain text generators. I think it is better to use this approach instead of changing the paper color to use the system background color (see bug 253583), because 1) the document might specify a text color in some places, 2) the user is able to change the fg/bg colors anyway using Okular's Accessibility options, and those probably expect black on white. This addresses bugs 253583 and 322547. http://bugs.kde.org/show_bug.cgi?id=253583 http://bugs.kde.org/show_bug.cgi?id=322547 Diffs - core/textdocumentgenerator.cpp b260b3f Diff: http://git.reviewboard.kde.org/r/111681/diff/ Testing --- I tested the document from bug 322547 comment #3. Thanks, Christoph Feck ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/#review37994 --- To be honest i'm a bit confused by all the different patches trying to fix the same thing, there's this one, the other one that tries to use kcolorscheme, the other one that tries to let the user choose. And what I don't understand is why we need these patches. I would understand that if an epub sets the background color it will also set the text color (so all is fine, none of our colors is used) and if no color is set, i would undertand that the color scheme by the user provides acceptable colors. So where is the problem? - Albert Astals Cid On July 25, 2013, noon, Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/ --- (Updated July 25, 2013, noon) Review request for Okular and Albert Astals Cid. Description --- As indicated in bug 322547, some documents do not specify a text color, and probably assume the default text color to be black. QTextDocument, however, defaults to using the system text color. This patch changes the default text color to Qt::black. It should affect epub, fb2, odt, and plain text generators. I think it is better to use this approach instead of changing the paper color to use the system background color (see bug 253583), because 1) the document might specify a text color in some places, 2) the user is able to change the fg/bg colors anyway using Okular's Accessibility options, and those probably expect black on white. This addresses bugs 253583 and 322547. http://bugs.kde.org/show_bug.cgi?id=253583 http://bugs.kde.org/show_bug.cgi?id=322547 Diffs - core/textdocumentgenerator.cpp b260b3f Diff: http://git.reviewboard.kde.org/r/111681/diff/ Testing --- I tested the document from bug 322547 comment #3. Thanks, Christoph Feck ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color
On Aug. 16, 2013, 8:58 p.m., Albert Astals Cid wrote: To be honest i'm a bit confused by all the different patches trying to fix the same thing, there's this one, the other one that tries to use kcolorscheme, the other one that tries to let the user choose. And what I don't understand is why we need these patches. I would understand that if an epub sets the background color it will also set the text color (so all is fine, none of our colors is used) and if no color is set, i would undertand that the color scheme by the user provides acceptable colors. So where is the problem? The problem is not documents that set colors, but those that set no colors (for example, plain text files). In the rendering code, the pixmap is pre-filled with hard-coded white color, but the text is actually rendered using the system's foreground color. This leads to white on white with dark color schemes (which usually use white text). There are, as you see, several ways to fix it. Why I believe this one makes more sense that using the system's background color: See my rationale at Description above. - Christoph --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/#review37994 --- On Aug. 16, 2013, 8:58 p.m., Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/ --- (Updated Aug. 16, 2013, 8:58 p.m.) Review request for Okular. Description --- As indicated in bug 322547, some documents do not specify a text color, and probably assume the default text color to be black. QTextDocument, however, defaults to using the system text color. This patch changes the default text color to Qt::black. It should affect epub, fb2, odt, and plain text generators. I think it is better to use this approach instead of changing the paper color to use the system background color (see bug 253583), because 1) the document might specify a text color in some places, 2) the user is able to change the fg/bg colors anyway using Okular's Accessibility options, and those probably expect black on white. This addresses bugs 253583 and 322547. http://bugs.kde.org/show_bug.cgi?id=253583 http://bugs.kde.org/show_bug.cgi?id=322547 Diffs - core/textdocumentgenerator.cpp b260b3f Diff: http://git.reviewboard.kde.org/r/111681/diff/ Testing --- I tested the document from bug 322547 comment #3. Thanks, Christoph Feck ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111410: Selection tool: copy/extract as vector graphic by calling pdftocairo
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111410/ --- (Updated Aug. 16, 2013, 9:17 p.m.) Review request for Okular. Changes --- Updating the patch according to the comments by Albert Astals Cid. Description --- This patch implements the feature request of bug 321350: if a PDF file is viewed, the selection tool offers the new extraction method vector which allows to save to a file (PDF, SVG, EPS, PostScript). The crop operation is performed by calling pdftocairo with matching arguments. The resulting file contains the original PDF file's content without rendering it to a pixmap. I am not sure if calling an external program is an acceptable solution for this problem. However, it is tested if the program is available before showing the new option. Alternatively, the code of pdftocairo (as part of poppler) would had to be copied and integrated into Okular increasing the solution's complexity. I am not aware of a similar solution available using poppler-qt4 only. Maybe using a QPrinter printing to PDF would have been an alternative, but again this seemed to be too complex. Diffs (updated) - core/document.h fe296e0 core/document.cpp 3af92c8 core/generator.h a5a971b core/generator.cpp 41beb92 generators/poppler/generator_pdf.h 5d5853a generators/poppler/generator_pdf.cpp 1a44523 ui/pageview.cpp 5944072 Diff: http://git.reviewboard.kde.org/r/111410/diff/ Testing --- Thanks, Thomas Fischer ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111410: Selection tool: copy/extract as vector graphic by calling pdftocairo
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111410/#review38002 --- core/generator.h http://git.reviewboard.kde.org/r/111410/#comment28097 Why are these not const anymore? generators/poppler/generator_pdf.cpp http://git.reviewboard.kde.org/r/111410/#comment28098 Not sure how expensive this is, but maybe only search it once in exportFormatsSignlePAgeRegion? You can have a static bool so that you only search it once, and have pdftoCaitoBinary also be static so there's only one in memory even if you have N open pdf files generators/poppler/generator_pdf.cpp http://git.reviewboard.kde.org/r/111410/#comment28099 Maybe also a static bool named alreadySearched, just in case the binary is there but none of the options is supported? - Albert Astals Cid On Aug. 16, 2013, 9:17 p.m., Thomas Fischer wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111410/ --- (Updated Aug. 16, 2013, 9:17 p.m.) Review request for Okular. Description --- This patch implements the feature request of bug 321350: if a PDF file is viewed, the selection tool offers the new extraction method vector which allows to save to a file (PDF, SVG, EPS, PostScript). The crop operation is performed by calling pdftocairo with matching arguments. The resulting file contains the original PDF file's content without rendering it to a pixmap. I am not sure if calling an external program is an acceptable solution for this problem. However, it is tested if the program is available before showing the new option. Alternatively, the code of pdftocairo (as part of poppler) would had to be copied and integrated into Okular increasing the solution's complexity. I am not aware of a similar solution available using poppler-qt4 only. Maybe using a QPrinter printing to PDF would have been an alternative, but again this seemed to be too complex. Diffs - core/document.h fe296e0 core/document.cpp 3af92c8 core/generator.h a5a971b core/generator.cpp 41beb92 generators/poppler/generator_pdf.h 5d5853a generators/poppler/generator_pdf.cpp 1a44523 ui/pageview.cpp 5944072 Diff: http://git.reviewboard.kde.org/r/111410/diff/ Testing --- Thanks, Thomas Fischer ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color
On Aug. 16, 2013, 8:58 p.m., Albert Astals Cid wrote: To be honest i'm a bit confused by all the different patches trying to fix the same thing, there's this one, the other one that tries to use kcolorscheme, the other one that tries to let the user choose. And what I don't understand is why we need these patches. I would understand that if an epub sets the background color it will also set the text color (so all is fine, none of our colors is used) and if no color is set, i would undertand that the color scheme by the user provides acceptable colors. So where is the problem? Christoph Feck wrote: The problem is not documents that set colors, but those that set no colors (for example, plain text files). In the rendering code, the pixmap is pre-filled with hard-coded white color, but the text is actually rendered using the system's foreground color. This leads to white on white with dark color schemes (which usually use white text). There are, as you see, several ways to fix it. Why I believe this one makes more sense that using the system's background color: See my rationale at Description above. Ok, can you try Jaydeep's branch epub-qtextdoc according to https://www.dropbox.com/s/mgf778ug6yjie2l/GSoC%20patches.odt the color issues are fixed there. Can you confirm? Is the solution acceptable for you? - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/#review37994 --- On Aug. 16, 2013, 8:58 p.m., Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/ --- (Updated Aug. 16, 2013, 8:58 p.m.) Review request for Okular. Description --- As indicated in bug 322547, some documents do not specify a text color, and probably assume the default text color to be black. QTextDocument, however, defaults to using the system text color. This patch changes the default text color to Qt::black. It should affect epub, fb2, odt, and plain text generators. I think it is better to use this approach instead of changing the paper color to use the system background color (see bug 253583), because 1) the document might specify a text color in some places, 2) the user is able to change the fg/bg colors anyway using Okular's Accessibility options, and those probably expect black on white. This addresses bugs 253583 and 322547. http://bugs.kde.org/show_bug.cgi?id=253583 http://bugs.kde.org/show_bug.cgi?id=322547 Diffs - core/textdocumentgenerator.cpp b260b3f Diff: http://git.reviewboard.kde.org/r/111681/diff/ Testing --- I tested the document from bug 322547 comment #3. Thanks, Christoph Feck ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color
On Aug. 16, 2013, 8:58 p.m., Albert Astals Cid wrote: To be honest i'm a bit confused by all the different patches trying to fix the same thing, there's this one, the other one that tries to use kcolorscheme, the other one that tries to let the user choose. And what I don't understand is why we need these patches. I would understand that if an epub sets the background color it will also set the text color (so all is fine, none of our colors is used) and if no color is set, i would undertand that the color scheme by the user provides acceptable colors. So where is the problem? Christoph Feck wrote: The problem is not documents that set colors, but those that set no colors (for example, plain text files). In the rendering code, the pixmap is pre-filled with hard-coded white color, but the text is actually rendered using the system's foreground color. This leads to white on white with dark color schemes (which usually use white text). There are, as you see, several ways to fix it. Why I believe this one makes more sense that using the system's background color: See my rationale at Description above. Albert Astals Cid wrote: Ok, can you try Jaydeep's branch epub-qtextdoc according to https://www.dropbox.com/s/mgf778ug6yjie2l/GSoC%20patches.odt the color issues are fixed there. Can you confirm? Is the solution acceptable for you? From quickly checking commit 1823cf9998555e22c6f3d8701728882dc34b244b (which is documented to fix the color issue), I see that Jaydeep injects CSS code to change QTextDocument's default color to Qt::black. While this might have the same result, my approach is simpler and uses less resources. Additionally, it looks like his patch is limited to epub, while my patch works for all generators that use textdocumentgenerator.cpp? Jaydeep might clarify, if I am wrong. - Christoph --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/#review37994 --- On Aug. 16, 2013, 8:58 p.m., Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/ --- (Updated Aug. 16, 2013, 8:58 p.m.) Review request for Okular. Description --- As indicated in bug 322547, some documents do not specify a text color, and probably assume the default text color to be black. QTextDocument, however, defaults to using the system text color. This patch changes the default text color to Qt::black. It should affect epub, fb2, odt, and plain text generators. I think it is better to use this approach instead of changing the paper color to use the system background color (see bug 253583), because 1) the document might specify a text color in some places, 2) the user is able to change the fg/bg colors anyway using Okular's Accessibility options, and those probably expect black on white. This addresses bugs 253583 and 322547. http://bugs.kde.org/show_bug.cgi?id=253583 http://bugs.kde.org/show_bug.cgi?id=322547 Diffs - core/textdocumentgenerator.cpp b260b3f Diff: http://git.reviewboard.kde.org/r/111681/diff/ Testing --- I tested the document from bug 322547 comment #3. Thanks, Christoph Feck ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111681: TextDocumentGenerator: Use black as default text color
On Aug. 16, 2013, 8:58 p.m., Albert Astals Cid wrote: To be honest i'm a bit confused by all the different patches trying to fix the same thing, there's this one, the other one that tries to use kcolorscheme, the other one that tries to let the user choose. And what I don't understand is why we need these patches. I would understand that if an epub sets the background color it will also set the text color (so all is fine, none of our colors is used) and if no color is set, i would undertand that the color scheme by the user provides acceptable colors. So where is the problem? Christoph Feck wrote: The problem is not documents that set colors, but those that set no colors (for example, plain text files). In the rendering code, the pixmap is pre-filled with hard-coded white color, but the text is actually rendered using the system's foreground color. This leads to white on white with dark color schemes (which usually use white text). There are, as you see, several ways to fix it. Why I believe this one makes more sense that using the system's background color: See my rationale at Description above. Albert Astals Cid wrote: Ok, can you try Jaydeep's branch epub-qtextdoc according to https://www.dropbox.com/s/mgf778ug6yjie2l/GSoC%20patches.odt the color issues are fixed there. Can you confirm? Is the solution acceptable for you? Christoph Feck wrote: From quickly checking commit 1823cf9998555e22c6f3d8701728882dc34b244b (which is documented to fix the color issue), I see that Jaydeep injects CSS code to change QTextDocument's default color to Qt::black. While this might have the same result, my approach is simpler and uses less resources. Additionally, it looks like his patch is limited to epub, while my patch works for all generators that use textdocumentgenerator.cpp? Jaydeep might clarify, if I am wrong. (Let me add that I wasn't aware about his work while I wrote the patch. I have no intention to disturb his work.) - Christoph --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/#review37994 --- On Aug. 16, 2013, 8:58 p.m., Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111681/ --- (Updated Aug. 16, 2013, 8:58 p.m.) Review request for Okular. Description --- As indicated in bug 322547, some documents do not specify a text color, and probably assume the default text color to be black. QTextDocument, however, defaults to using the system text color. This patch changes the default text color to Qt::black. It should affect epub, fb2, odt, and plain text generators. I think it is better to use this approach instead of changing the paper color to use the system background color (see bug 253583), because 1) the document might specify a text color in some places, 2) the user is able to change the fg/bg colors anyway using Okular's Accessibility options, and those probably expect black on white. This addresses bugs 253583 and 322547. http://bugs.kde.org/show_bug.cgi?id=253583 http://bugs.kde.org/show_bug.cgi?id=322547 Diffs - core/textdocumentgenerator.cpp b260b3f Diff: http://git.reviewboard.kde.org/r/111681/diff/ Testing --- I tested the document from bug 322547 comment #3. Thanks, Christoph Feck ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 111829: Use DPI of current screen for PDF rendering
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111829/ --- (Updated Aug. 17, 2013, 2:02 a.m.) Review request for Okular and Albert Astals Cid. Changes --- Let's try with dedicated DPI class (named Resolution). Perhaps, multiplication opeartors are not strictly required, since they simplifies life not that much ass one would want Description --- This patch relies on master branch of LibKScreen. This patch does not solve the problem in bug completely, but makes Okular behaviour more correct (see below). The problem (in the bug) is that Okular uses fixed DPI for PDF rendering (72.0 dots per inch) and therefore scale of rendered document becomes incorrect. With current mainline laptop screens having DPI easily twice larger than this constant (72), the problem shows itself quiet strongly. Additional problems araise with multiscreen configuration, when 1) DPI of each individual screen may be different, and 2) there is no tools in Qt to detect DPI of individual screens in virtual desktop mode. Therefore XRandr has to be used for DPI detection. Raw XRandr is bad dependency for Okular and libkscreen is proposed instead. This patch approach to the solution in the following way: 1. libkscreen detection staff is added to CMakeLists.txt and config.h 2. It changes Utils::realDpi() function to use libkscreen if present. With libkscreen the function looks for output that contains maximal part of given widget (suppose to be used for document rendering) and returns DPI of that screen. 3. Genenerator interface is extended by adding UtilizeDPI feature and setDPI() method, that is called by Document class right before calling loadXXX() if the generator supports this feature 4. Poppler generator is changed to support UtilizeDPI feature. 5. PageSizeMetric enum is extended with Pixels value to explicitly say that page size is defined in screen pixels (see my posts in the bug); Poppler generator uses this case. To completetly fix the bug, Document must invalidate generated pixmaps after the widget movements into another screen. I do not know how to track this without subclassing the main window class. Therefore I decided to publish this part of work to get your responce, especially regarding item 3 (Generator class changes). In the current state, manual reloading of a document after moving Okular to another screen fixes the scale, that is, in my eyes, is quiet helpful already. Even if we subclass the Okular main window, I do not know what to do when Okular is used as KPart. This addresses bug 268757. http://bugs.kde.org/show_bug.cgi?id=268757 Diffs (updated) - CMakeLists.txt 217337f config-okular.h.cmake 7217f8d core/document.cpp 3af92c8 core/document_p.h 3a257de core/generator.h a5a971b core/generator.cpp 41beb92 core/generator_p.h b59293a core/utils.h 8d5d5fc core/utils.cpp 5dd8448 generators/poppler/generator_pdf.h 5d5853a generators/poppler/generator_pdf.cpp 1a44523 Diff: http://git.reviewboard.kde.org/r/111829/diff/ Testing --- Manual. In all screens, that report correct physical size to XRandr, size of documents is correct Thanks, Eugene Shalygin ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110914: Tabbed interface
On Aug. 14, 2013, 10:29 p.m., Albert Astals Cid wrote: When I started looking at dbus, I couldn't get anything to work. Whenever I tried to run any org.kde.okular method in qdbusviewer, it would say unable to find method or something similar. So I thought the problem might be related to the fact that two objects were each exporting an interface with the same name (org.kde.okular) but with different methods. When I changed the interface names, everything seemed to start working. I've never used dbus before this, so my understanding could be off. But basically, I had to do this for it to work on my computer. I'll go back again to see if it works without changing the names. - Jonathan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110914/#review37814 --- On Aug. 3, 2013, 10:03 p.m., Jonathan Doman wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110914/ --- (Updated Aug. 3, 2013, 10:03 p.m.) Review request for Okular. Description --- This patch adds support for a tabbed interface (multiple documents in one window). The core work just adds a tab bar that switches between multiple embedded okularparts, but there are many other considerations: - Tab context menu allows for duplicating or detaching (detached tabs start in new okular process) - `okular file.pdf` will open file in existing window if possible, unless --new flag is used. It also selects the most recently raised/activated window to use. This mirrors behavior I expect from browsers and other tabbed interfaces. - Warns when closing window with multiple tabs - No warning is given when opening an already open file. This is the behavior I strongly prefer (and observe in other programs), but will change if there is consensus otherwise. When selecting different tools in one part, the tool selection propagates to all parts, but the GUI does not reflect that. This bug is present in other programs (e.g. multiple okularparts in Konqueror), so I made no attempt to diagnose or fix. One menu item was added for the multiple tab warning option. When testing this, I noticed that items in the Settings menu seem to move around when switching tabs, and I cannot diagnose or fix this. It seems to be related to XMLGUI bug #64754. My development branch is also hosted at https://github.com/jrmrjnck/okular-tabbed This addresses bug 155515. http://bugs.kde.org/show_bug.cgi?id=155515 Diffs - part.cpp 71c3d0f5d908969ffcf18aba327297ccd2e1c8e1 part.h 4b3aafdb637080ae81eb0e45742f53a34738984d shell/main.cpp e0ca587ba167c4020d5af5bd33a2dc1b4923cac4 shell/shell.h c065c560fb4ddfcf181601cf35e9ca14581731f6 shell/shell.cpp 1708501daaef817a1ce35fa5d96701a66ab66983 shell/shell.rc 93fbc417588312792bab39b693c65e5d414c87c6 Diff: http://git.reviewboard.kde.org/r/110914/diff/ Testing --- Thanks, Jonathan Doman ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
Re: [Okular-devel] Review Request 110914: Tabbed interface
On Aug. 14, 2013, 10:29 p.m., Albert Astals Cid wrote: shell/shell.h, line 49 http://git.reviewboard.kde.org/r/110914/diff/2/?file=176095#file176095line49 Why are you changing the dbus names? This will break whatever scripts people where using. It seems my experiences were caused by a bug/feature in qdbusviewer (https://bugreports.qt-project.org/browse/QTBUG-6943). So, while technically it's okay to keep the interface names how they are, it makes more sense to me that they be different. - Jonathan --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110914/#review37814 --- On Aug. 3, 2013, 10:03 p.m., Jonathan Doman wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110914/ --- (Updated Aug. 3, 2013, 10:03 p.m.) Review request for Okular. Description --- This patch adds support for a tabbed interface (multiple documents in one window). The core work just adds a tab bar that switches between multiple embedded okularparts, but there are many other considerations: - Tab context menu allows for duplicating or detaching (detached tabs start in new okular process) - `okular file.pdf` will open file in existing window if possible, unless --new flag is used. It also selects the most recently raised/activated window to use. This mirrors behavior I expect from browsers and other tabbed interfaces. - Warns when closing window with multiple tabs - No warning is given when opening an already open file. This is the behavior I strongly prefer (and observe in other programs), but will change if there is consensus otherwise. When selecting different tools in one part, the tool selection propagates to all parts, but the GUI does not reflect that. This bug is present in other programs (e.g. multiple okularparts in Konqueror), so I made no attempt to diagnose or fix. One menu item was added for the multiple tab warning option. When testing this, I noticed that items in the Settings menu seem to move around when switching tabs, and I cannot diagnose or fix this. It seems to be related to XMLGUI bug #64754. My development branch is also hosted at https://github.com/jrmrjnck/okular-tabbed This addresses bug 155515. http://bugs.kde.org/show_bug.cgi?id=155515 Diffs - part.cpp 71c3d0f5d908969ffcf18aba327297ccd2e1c8e1 part.h 4b3aafdb637080ae81eb0e45742f53a34738984d shell/main.cpp e0ca587ba167c4020d5af5bd33a2dc1b4923cac4 shell/shell.h c065c560fb4ddfcf181601cf35e9ca14581731f6 shell/shell.cpp 1708501daaef817a1ce35fa5d96701a66ab66983 shell/shell.rc 93fbc417588312792bab39b693c65e5d414c87c6 Diff: http://git.reviewboard.kde.org/r/110914/diff/ Testing --- Thanks, Jonathan Doman ___ Okular-devel mailing list Okular-devel@kde.org https://mail.kde.org/mailman/listinfo/okular-devel
