'Re: Review Request: Kpresenter filter footer delcaration support.'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Kpresenter filter footer delcaration support.
From:       jos.van.den.oever () kogmbh ! com
Date:       2010-02-01 8:40:13
Message-ID: 20100201084013.25862.93705 () localhost
[Download RAW message or body]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2771/#review4003
-----------------------------------------------------------

In general a nice patch.
It is large, so I'd like you to run ppttoodp in valgrind on a folder with at least a \
100 ppt files. use the script in tools/script for downloading ppt files to get them.

Other than that the presentationDeclaration is not clear to me. It seems that it \
could be a more optimal container. Could you explain why you use a QHash< QPair < > , \
QString> for it and not e.g. a QHash< QHash < > > ? Or even a QMap instead of a \
QHash.

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.h
<http://reviewboard.kde.org/r/2771/#comment3368>

    coordinate -> coordinate

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.h
<http://reviewboard.kde.org/r/2771/#comment3369>

    masterObjects -> masterObjects

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.h
<http://reviewboard.kde.org/r/2771/#comment3370>

    Cordinates -> coordinates

    (some IDEs have a spell-checker ;-)

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3372>

    Better not to use a pointer here. If you want a shortcut, use & pd. However, the \
full name presentationDeclaration is not that long.  

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3374>

    pd and xmlwriter should never be zero here, so no need to test it.

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3375>

    use a temporary for result of getSlideHF, since that function has some cost

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3373>

    Why is this commented out?

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3376>

    no std::cout please

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3371>

    Would it make sense to use a QMap for presentationDeclaration? Then you do not \
need this findDeclaration. Same for findNotesDeclaration. insertDeclaration would \
also be easier.

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3379>

    ah, you are using the declaration hash as a multihash.

    Wouldnt a two level map be more appropriate?

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3378>

    You can use count(type), that removes the need for this function.

trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp
<http://reviewboard.kde.org/r/2771/#comment3377>

    You can use count(type)

- vandenoever

On 2010-01-30 16:29:48, Amit Aggarwal wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2771/
> -----------------------------------------------------------
> 
> (Updated 2010-01-30 16:29:48)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> Adding footer declaration support for kpresenter filter.
> 
> 
> Diffs
> -----
> 
> trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.h 1082456 
> trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp 1082456 
> trunk/koffice/filters/kpresenter/powerpoint/mso/simpleParser.h 1082456 
> trunk/koffice/filters/kpresenter/powerpoint/mso/simpleParser.cpp 1082456 
> 
> Diff: http://reviewboard.kde.org/r/2771/diff
> 
> 
> Testing
> -------
> 
> Tested on different ppt files.
> 
> 
> Thanks,
> 
> Amit
> 
> 

_______________________________________________
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] 