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

List:       koffice-devel
Subject:    Re: Review Request: Hyperlink support for powerpoint filter.
From:       "Amit Aggarwal" <amitcs06 () gmail ! com>
Date:       2010-01-27 17:13:15
Message-ID: 20100127171315.17369.734 () localhost
[Download RAW message or body]


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


Please find my some comments

* @param text TextContainer to search hyperlinks from

 Extra Please delete it.

    HyperlinkRange findNextHyperlinkStart(const PPT::TextContainer& text,

  Comment param is not proper

> writeTextSpan(KoXmlWriter& xmlWriter,
                                     const QString &text)

 this function is doing only simplified. Can we use QString::simplifed and remove \
this function ? 

container->interactiveInfoAtom.action == 3

 For this magic numbers can we use enum as per the MS-PPT specs

 utf16ToString function 
Can we use macro or inline function as it is very small ?


        if (text.endsWith("\r")) {
            text = text.left(text.length() - 1);
        }

Instead of this I think QString::Simplified function will help ?

        if (!getListStyleName(0, 0).isEmpty()) {
                out.xml.addAttribute("text:style-name", getListStyleName(0, 0));
        }
Wrong indentation

    nameStr.remove('\r');
    nameStr.remove('\v');

Not required. getText instead of returing QString directly we can assign QString into \
variable and use simplified function.

ex:-    QString text =  QString::fromAscii(textChars, textChars.size());
        text = text.simplified();
        return text;

- Amit


On 2010-01-27 11:43:04, Vesa Pikki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2737/
> -----------------------------------------------------------
> 
> (Updated 2010-01-27 11:43:04)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> Here is a patch for adding a rudimentary support for hyperlinks within powerpoint \
> files in powerpoint filter. 
> 
> Diffs
> -----
> 
> trunk/koffice/filters/kpresenter/powerpoint/mso/simpleParser.h 1080994 
> trunk/koffice/filters/kpresenter/powerpoint/mso/simpleParser.cpp 1080994 
> trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp 1080994 
> trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.h 1080994 
> 
> Diff: http://reviewboard.kde.org/r/2737/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vesa
> 
> 

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