[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:       "Vesa Pikki" <vesa.pikki () ixonos ! com>
Date:       2010-01-28 7:28:33
Message-ID: 20100128072833.4270.28575 () localhost
[Download RAW message or body]


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

(Updated 2010-01-28 07:28:32.582186)


Review request for KOffice.


Changes
-------

* @param text TextContainer to search hyperlinks from

 Extra Please delete it.
Deleted


    HyperlinkRange findNextHyperlinkStart(const PPT::TextContainer& text,

  Comment param is not proper
Updated


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

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

Simplified also removes sequences of whitespace inside the string into a single \
whitespace. I wouldn't want to touch any content that is written by the user.


container->interactiveInfoAtom.action == 3

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

Added enumeration InteractiveInfoActionEnum and used it's values.


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

Added inline.


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

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

I only want to remove last \r, not all of them.


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

Fixed 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;

I would not want to remove any whitespace added by the user.


Summary
-------

Here is a patch for adding a rudimentary support for hyperlinks within powerpoint \
files in powerpoint filter.


Diffs (updated)
-----

  trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.h 1081300 
  trunk/koffice/filters/kpresenter/powerpoint/PptToOdp.cpp 1081300 
  trunk/koffice/filters/kpresenter/powerpoint/mso/simpleParser.h 1081300 
  trunk/koffice/filters/kpresenter/powerpoint/mso/simpleParser.cpp 1081300 

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]