[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: Review Request: Correct export pdf in KPresenter and allow to
From: "Thorsten Zachmann" <t.zachmann () zagge ! de>
Date: 2010-10-21 2:53:46
Message-ID: 20101021025346.17106.71002 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On 2010-10-16 04:41:09, Thorsten Zachmann wrote:
> > /trunk/koffice/kpresenter/part/KPrPdfPrintJob.h, line 30
> > <http://svn.reviewboard.kde.org/r/5610/diff/2/?file=39200#file39200line30>
> >
> > It might be a good idea to inherit KoPAPrintJob and try to share as much code as \
> > possible between this two classes.
> > The KPrPdfPrintJob misses the KoPAPageProvider that is needed to get the page \
> > number correct. You can have a look at KoPAPageProvider on how it is used.
>
> Benjamin Port wrote:
> Ok, I'll going to use KoPAPageProvider for page number.
> However I'm not sure it's a good idea to inherits KoPAPrintJob because code it's \
> different. And attribute like KoPAPageProvider were private in KoPAPrintJob
It is fine with me to make the KoPAPageProvider protected. That is preferred instead \
of having a lot of duplicate code and makes it more maintainable.
> On 2010-10-16 04:41:09, Thorsten Zachmann wrote:
> > /trunk/koffice/kpresenter/part/KPrPdfPrintJob.cpp, line 68
> > <http://svn.reviewboard.kde.org/r/5610/diff/2/?file=39201#file39201line68>
> >
> > Is that is what is wanted when exporting a file to pdf. Don't the user want to \
> > set the size of the paper to use and then put the content as big as possible \
> > centered on the page?
> > Otherwise there might be strange page sizes which has nothing to do with real \
> > page sizes.
>
> Thomas Zander wrote:
> The point of setting the size of the paper first is forced when you print to paper, \
> obviously you can't change paper size. On the other hand when printing to PDF there \
> are multiple usecases and using the slide's size makes a lot of sense for various \
> usecases. Like for example when using a PDF viewer to actually present.
> So when creating a PDF I think Benjamin is right. We should not forget about the \
> people printing to a real printer but in my personal opinion that usecase is \
> reserved for a feature like 'create handouts'.
> Benjamin Port wrote:
> For me the task to put the content as big as possible is done by the pdf reader \
> tool. I think export pdf is an easy to use functionnality to allow user to have \
> presentation at pdf format. So for me the good way to do this is not to have a real \
> paper size, but to have a paper size like slide size. And if someone want to print \
> as pdf with custom size, imo the good way is to use the print task and redirect \
> printer to pdf file, so user can select paper size...
It is your call. Thanks for the clarification.
- Thorsten
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5610/#review8176
-----------------------------------------------------------
On 2010-10-12 16:41:11, Benjamin Port wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5610/
> -----------------------------------------------------------
>
> (Updated 2010-10-12 16:41:11)
>
>
> Review request for KOffice.
>
>
> Summary
> -------
>
> BUG:241245
> This patch correct the bug 214245, when we export to pdf in KPresenter, now the \
> page size is slide size. With the new methods to export to pdf, each KOffice \
> application can choose to kept the old export feature (just print), or implement a \
> specific operation for export pdf.
>
> Diffs
> -----
>
> /trunk/koffice/kpresenter/part/CMakeLists.txt 1185156
> /trunk/koffice/kpresenter/part/KPrPdfPrintJob.h PRE-CREATION
> /trunk/koffice/kpresenter/part/KPrPdfPrintJob.cpp PRE-CREATION
> /trunk/koffice/kpresenter/part/KPrView.h 1185156
> /trunk/koffice/kpresenter/part/KPrView.cpp 1185156
> /trunk/koffice/libs/main/KoMainWindow.cpp 1185156
> /trunk/koffice/libs/main/KoView.h 1185156
> /trunk/koffice/libs/main/KoView.cpp 1185156
>
> Diff: http://svn.reviewboard.kde.org/r/5610/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5610/">http://svn.reviewboard.kde.org/r/5610/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 16th, 2010, 4:41 a.m., <b>Thorsten \
Zachmann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://svn.reviewboard.kde.org/r/5610/diff/2/?file=39200#file39200line30" \
style="color: black; font-weight: bold; text-decoration: \
underline;">/trunk/koffice/kpresenter/part/KPrPdfPrintJob.h</a> <span \
style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
"></pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: \
0; ">class KoPAPageBase;</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">30</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="n">class</span> <span class="n">KPrPdfPrintJob</span> <span class="o">:</span> \
<span class="n">public</span> <span class="n">KoPrintJob</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It might be a good idea \
to inherit KoPAPrintJob and try to share as much code as possible between this two \
classes.
The KPrPdfPrintJob misses the KoPAPageProvider that is needed to get the page number \
correct. You can have a look at KoPAPageProvider on how it is used. </pre> \
</blockquote>
<p>On October 16th, 2010, 5:37 p.m., <b>Benjamin Port</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ok, I'll going to \
use KoPAPageProvider for page number. However I'm not sure it's a good idea \
to inherits KoPAPrintJob because code it's different. And attribute like \
KoPAPageProvider were private in KoPAPrintJob</pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It is fine \
with me to make the KoPAPageProvider protected. That is preferred instead of having a \
lot of duplicate code and makes it more maintainable.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 16th, 2010, 4:41 a.m., <b>Thorsten \
Zachmann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://svn.reviewboard.kde.org/r/5610/diff/2/?file=39201#file39201line68" \
style="color: black; font-weight: bold; text-decoration: \
underline;">/trunk/koffice/kpresenter/part/KPrPdfPrintJob.cpp</a> <span \
style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
"></pre></td> <td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: \
0; ">void KPrPdfPrintJob::startPrinting(RemovePolicy removePolicy)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">68</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">m_printer</span><span class="p">.</span><span \
class="n">setPaperSize</span><span class="p">(</span><span \
class="n">QSizeF</span><span class="p">(</span><span class="n">layout</span><span \
class="p">.</span><span class="n">width</span><span class="p">,</span><span \
class="n">layout</span><span class="p">.</span><span class="n">height</span><span \
class="p">),</span><span class="n">QPrinter</span><span class="o">::</span><span \
class="n">Millimeter</span><span class="p">);</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is that is what is \
wanted when exporting a file to pdf. Don't the user want to set the size of the \
paper to use and then put the content as big as possible centered on the page?
Otherwise there might be strange page sizes which has nothing to do with real page \
sizes.</pre> </blockquote>
<p>On October 16th, 2010, 2:25 p.m., <b>Thomas Zander</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The point of setting the \
size of the paper first is forced when you print to paper, obviously you can't \
change paper size. On the other hand when printing to PDF there are multiple usecases \
and using the slide's size makes a lot of sense for various usecases. Like for \
example when using a PDF viewer to actually present.
So when creating a PDF I think Benjamin is right. We should not forget about the \
people printing to a real printer but in my personal opinion that usecase is reserved \
for a feature like 'create handouts'.</pre> </blockquote>
<p>On October 16th, 2010, 5:30 p.m., <b>Benjamin Port</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For me the task to put \
the content as big as possible is done by the pdf reader tool. I think export pdf is \
an easy to use functionnality to allow user to have presentation at pdf format. So \
for me the good way to do this is not to have a real paper size, but to have a paper \
size like slide size. And if someone want to print as pdf with custom size, imo the \
good way is to use the print task and redirect printer to pdf file, so user can \
select paper size...</pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It is your \
call. Thanks for the clarification.</pre> <br />
<p>- Thorsten</p>
<br />
<p>On October 12th, 2010, 4:41 p.m., Benjamin Port wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KOffice.</div>
<div>By Benjamin Port.</div>
<p style="color: grey;"><i>Updated 2010-10-12 16:41:11</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">BUG:241245 This patch correct the bug 214245, when we export to pdf in \
KPresenter, now the page size is slide size. With the new methods to export to pdf, \
each KOffice application can choose to kept the old export feature (just print), or \
implement a specific operation for export pdf.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/koffice/kpresenter/part/CMakeLists.txt <span style="color: \
grey">(1185156)</span></li>
<li>/trunk/koffice/kpresenter/part/KPrPdfPrintJob.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>/trunk/koffice/kpresenter/part/KPrPdfPrintJob.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>/trunk/koffice/kpresenter/part/KPrView.h <span style="color: \
grey">(1185156)</span></li>
<li>/trunk/koffice/kpresenter/part/KPrView.cpp <span style="color: \
grey">(1185156)</span></li>
<li>/trunk/koffice/libs/main/KoMainWindow.cpp <span style="color: \
grey">(1185156)</span></li>
<li>/trunk/koffice/libs/main/KoView.h <span style="color: \
grey">(1185156)</span></li>
<li>/trunk/koffice/libs/main/KoView.cpp <span style="color: \
grey">(1185156)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5610/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic