Re: [patch 1/2] TextSimpleOperator introduction

[Michal H. <ms...@gm...>]

On Thu, Aug 28, 2008 at 07:12:17AM +0000, Jozef Misutka wrote:
> 
> i am not sure whether a factory class is reasonable for such a generic
> structure as pdf operators ; moreover, most of the direct
> initialization is in tests, but I do not mind...
> 

Yeah, most of direct SimpleGenericOperator creation is in testing code,
but there are still one place in the gui. I am wondering whether this
single place addresses all operators that can be added by gui operations
(draw line, add text, ...).

> we should somehow change the function names like getEncodedText to
> something more cleare - get_unicode, get_whateverencoding, but the
> original is very ambiguous and we will get lost when implementing
> unicode.
> 

what about getFontText? 

We can't do anything about encoding here because we can only provide
what we get from the font (and font can use whatever encoding it likes),
so I would like to prevent from some naming according encoding. 

> otherwise it looks ok to me.
> 
> thanx for the work!
> 
> > Date: Wed, 27 Aug 2008 20:03:08 -0600
> > From: ms...@gm...
> > To: pdf...@li...
> > Subject: [patch 1/2] TextSimpleOperator introduction
> >
> > This patch introduces the TextSimpleOperator specialized class for text
> > operators holding a text and replaces script based getTextFromTextOperator
> > function implementation by native TextSimpleOperator::getEncodedText function.
> >
> > We need this new operator for proper handling of encoding for text objects.
> > Currently we are simply providing raw text without any consideration of font
> > encoding. This patch prepares background for proper implementation inside
> > the TextSimpleOperator class.
> >
> > I also think that it is cleaner to have getTextFromTextOperator implemented
> > in the core rather than scripting because this functionality may be required
> > by some code which is not based on scripting (e.g. users of devel package)
> > and it is too core to be scriptable (but this can be arguable)
> >
> > TODO: We should get rid of direct creation of operators and provide some
> > factory method which will return shared_ptr
> > There are several places where SimpleGenericOperator is created directly:
> > ===========
> > gui/base.cc:296: boost::shared_ptr op(new SimpleGenericOperator(opTxt,param));
> > kernel/contentschangetag.cc:66: return shared_ptr (new SimpleGenericOperator (CHANGE_TAG_NAME, opers));
> > kernel/ccontentstream.cc:302: return shared_ptr (new SimpleGenericOperator (string (o->getCmd()),operands));
> > kernel/ccontentstream.cc:337: return shared_ptr (new SimpleGenericOperator (chcktp->name, argNum, operands));
> > kernel/cpagedisplay.cc:266: shared_ptr cmop (new SimpleGenericOperator ("cm", 6, operands));
> > kernel/pdfoperators.cc:141: return shared_ptr (new SimpleGenericOperator (_opText,ops));
> > kernel/textoutputengines.cc:257: shared_ptr newop (new SimpleGenericOperator ("Tj", opers));
> > tests/kernel/testpdfoperators.cc:290: oper = shared_ptr (new SimpleGenericOperator ("BT", 0, operands));
> > tests/kernel/testpdfoperators.cc:297:// oper = shared_ptr (new SimpleGenericOperator ("Tf", 2, operands));
> > tests/kernel/testpdfoperators.cc:306: oper = shared_ptr (new SimpleGenericOperator ("Td", 2, operands));
> > tests/kernel/testpdfoperators.cc:313: oper = shared_ptr (new SimpleGenericOperator ("q", 0, operands));
> > tests/kernel/testpdfoperators.cc:323: oper = shared_ptr (new SimpleGenericOperator ("rg", 3, operands));
> > tests/kernel/testpdfoperators.cc:330: oper = shared_ptr (new SimpleGenericOperator ("Q", 0, operands));
> > tests/kernel/testpdfoperators.cc:338: oper = shared_ptr (new SimpleGenericOperator ("Tj", 1, operands));
> > tests/kernel/testpdfoperators.cc:344: oper = shared_ptr (new SimpleGenericOperator ("ET", 0, operands));
> > tests/kernel/testpdfoperators.cc:386: shared_ptr oper (new SimpleGenericOperator ("Tj", 1, operands));
> > tests/kernel/testccontentstream.cc:452: ops.push_back (boost::shared_ptr (new SimpleGenericOperator("lala",0,operands)));
> > ===========
> >
> > I was thinking of extracting instance creation functionality from
> > createOperator function into something like
> > createOperator(const char *, Operands&)
> > but InlineImageOperator requires StreamReader so I was not successful.
> > Any idea how to accomplish this, Jozo?
> >
> > * gui/pdfoperators.qs
> > - getTextFromTextOperator implementation removed and replaced by
> > getEncodedText
> >
> > * gui/qspdfoperator.{cc,h}
> > - QSPdfOperator::getEncodedText added to enable
> > TextSimpleOperator::getEncodedText method to be exported to the
> > scripting.
> > I am not sure whether this is proper place, but there doesn't seem
> > to better one.
> >
> > * kernel/ccontentstream.cc
> > - createOperator updated to create TextSimpleOperator instance for
> > text operators
> >
> > * kernel/pdfoperators.{cc,h}
> > - TextSimpleOperator class added
> > - inherits from SimpleGenericOperator
> > - adds getEncodedText
> > - based on previous getTextFromTextOperator
> > implementation and doesn't implement the encoding
> > conversion yet
> >
> > * kernel/stateupdater.h
> > - isTextOp added
> > Index: pdfedit-patches/src/gui/pdfoperator.qs
> > ===================================================================
> > --- pdfedit-patches.orig/src/gui/pdfoperator.qs 2008-08-26 13:03:10.000000000 -0600
> > +++ pdfedit-patches/src/gui/pdfoperator.qs 2008-08-26 14:08:57.000000000 -0600
> > @@ -971,37 +971,7 @@ function getTextFromTextOperator( op ) {
> > if (! isTextOp( op ))
> > return "";
> >
> > - switch (op.getName()) {
> > - case "'":
> > - case "Tj":
> > - if (op.paramCount() == 1) {
> > - return op.params().property(0).value();
> > - }
> > - break;
> > - case "\"":
> > - if (op.paramCount() == 3) {
> > - return op.params().property(2).value();
> > - }
> > - break;
> > - case "TJ":
> > - if ((op.paramCount() != 1) ||
> > - (op.params().property(0).type() != "Array")) {
> > - break;
> > - }
> > - var p = op.params().property(0); // Array of String and Numbers
> > - var c = p.count();
> > - var i = 0;
> > - var text = "";
> > - for ( ; i < c ; ++i ) {
> > - if (p.property(i).getType() == "String") {
> > - text = text + p.property(i).value();
> > - }
> > - }
> > - return text;
> > - break;
> > - }
> > -
> > - return "";
> > + return op.getEncodedText();
> > }
> >
> > /** Move text operator relative [dx,dy] */
> > Index: pdfedit-patches/src/gui/qspdfoperator.cc
> > ===================================================================
> > --- pdfedit-patches.orig/src/gui/qspdfoperator.cc 2008-08-26 13:08:16.000000000 -0600
> > +++ pdfedit-patches/src/gui/qspdfoperator.cc 2008-08-26 13:40:23.000000000 -0600
> > @@ -112,6 +112,15 @@ QString QSPdfOperator::getText() {
> > return util::convertToUnicode(text,util::PDF);
> > }
> >
> > +QString QSPdfOperator::getEncodedText() {
> > + std::string text;
> > + TextSimpleOperator * textOp = dynamic_cast(obj.get());
> > + if (!textOp)
> > + return QString::null;
> > + textOp->getEncodedText(text);
> > + return util::convertToUnicode(text, util::PDF);
> > +}
> > +
> > /**
> > Create new operator iterator from this PDF operator.
> > The iterator will be initially positioned at this item
> > Index: pdfedit-patches/src/gui/qspdfoperator.h
> > ===================================================================
> > --- pdfedit-patches.orig/src/gui/qspdfoperator.h 2008-08-26 13:08:16.000000000 -0600
> > +++ pdfedit-patches/src/gui/qspdfoperator.h 2008-08-26 13:11:49.000000000 -0600
> > @@ -126,6 +126,14 @@ public slots:
> > int childCount();
> > /*- Return text representation of this pdf operator */
> > QString getText();
> > +
> > + // TODO place to somehow better place or leave it here?
> > + /*-
> > + Returns encoded text from text operator. Returns an empty string
> > + for all other operators.
> > + */
> > + QString getEncodedText();
> > +
> > /*- Return name of this pdf operator */
> > QString getName();
> > /*- Returns parameters of this operator in array */
> > Index: pdfedit-patches/src/kernel/ccontentstream.cc
> > ===================================================================
> > --- pdfedit-patches.orig/src/kernel/ccontentstream.cc 2008-08-26 13:52:46.000000000 -0600
> > +++ pdfedit-patches/src/kernel/ccontentstream.cc 2008-08-26 16:33:55.000000000 -0600
> > @@ -330,11 +330,14 @@ namespace {
> > //
> > // If endTag is "" it is a simple operator, composite otherwise
> > //
> > + if (isTextOp(*chcktp))
> > + return shared_ptr (new TextSimpleOperator(chcktp->name, argNum, operands));
> > +
> > if (isSimpleOp(*chcktp))
> > return shared_ptr (new SimpleGenericOperator (chcktp->name, argNum, operands));
> >
> > - else // Composite operator
> > - return shared_ptr (new UnknownCompositePdfOperator (chcktp->name, chcktp->endTag));
> > + // Composite operator
> > + return shared_ptr (new UnknownCompositePdfOperator (chcktp->name, chcktp->endTag));
> > }
> >
> > /**
> > Index: pdfedit-patches/src/kernel/pdfoperators.cc
> > ===================================================================
> > --- pdfedit-patches.orig/src/kernel/pdfoperators.cc 2008-08-26 12:29:53.000000000 -0600
> > +++ pdfedit-patches/src/kernel/pdfoperators.cc 2008-08-26 16:34:28.000000000 -0600
> > @@ -171,6 +171,70 @@ SimpleGenericOperator::init_operands (sh
> > }
> >
> >
> > +void TextSimpleOperator::getEncodedText(std::string& str)
> > +{
> > +using namespace utils;
> > + utilsPrintDbg(debug::DBG_DBG, "");
> > + std::string name, rawStr;
> > + getOperatorName(name);
> > + Operands ops;
> > + getParameters(ops);
> > + if(name == "'" || name == "Tj")
> > + {
> > + if(ops.size() != 1 || !isString(ops[0]))
> > + {
> > + utilsPrintDbg(debug::DBG_WARN, "Bad operands for operator "
> > + <<name<<" count="< + <<" ops[0] type="<< ops[0]->getType());
> > + return;
> > + }
> > + rawStr = getStringFromIProperty(ops[0]);
> > + }
> > + else if (name == "\"")
> > + {
> > + if(ops.size() != 3 || !isArray(ops[2]))
> > + {
> > + utilsPrintDbg(debug::DBG_WARN, "Bad operands for operator "
> > + <<name<<" count="< + <<" ops[2] type="<< ops[2]->getType());
> > + return;
> > + }
> > + rawStr = getStringFromIProperty(ops[2]);
> > + }
> > + else if (name == "TJ")
> > + {
> > + shared_ptr op = ops[0];
> > + if (!isArray(op) || ops.size() != 1)
> > + {
> > + utilsPrintDbg(debug::DBG_WARN, "Bad operands for TJ operator: ops[type="
> > + << op->getType() <<" size="<<ops.size()<<"]");
> > + return;
> > + }
> > + shared_ptr opArray = IProperty::getSmartCObjectPtr(op);
> > + std::vector props;
> > + opArray->_getAllChildObjects(props);
> > + std::vector::iterator i;
> > + for(i=props.begin(); i!=props.end(); ++i)
> > + {
> > + shared_ptr p = *i;
> > +
> > + // TODO consider spacing coming from values
> > + if(!(isString(p)))
> > + continue;
> > + rawStr += getStringFromIProperty(p);
> > + }
> > +
> > + }else
> > + {
> > + utilsPrintDbg(debug::DBG_WARN, "Bad operator name="< + return;
> > + }
> > +
> > + // TODO transform rawStr -> str
> > + str = rawStr;
> > +}
> > +
> > +
> > //==========================================================
> > // Concrete implementations of CompositePdfOperator
> > //==========================================================
> > Index: pdfedit-patches/src/kernel/pdfoperators.h
> > ===================================================================
> > --- pdfedit-patches.orig/src/kernel/pdfoperators.h 2008-08-26 12:29:53.000000000 -0600
> > +++ pdfedit-patches/src/kernel/pdfoperators.h 2008-08-26 16:34:18.000000000 -0600
> > @@ -115,6 +115,34 @@ public:
> > }; // class SimpleGenericOperator
> >
> >
> > +/** Text dedicated operator class.
> > + * This class represents those text operators which contains text to be
> > + * displayed. This is necessary, because text string stored in operator's
> > + * operands is not the same as the displayed one in general and may be
> > + * affected by font encoding. This class enhances SimpleGenericOperator
> > + * class with font encoding specific methods getEncodedText and setRawText.
> > + * Those two methods should be prefered when text is set or obtained rather
> > + * than direct seting/geting operators operands.
> > + */
> > +class TextSimpleOperator: public SimpleGenericOperator
> > +{
> > +public:
> > + TextSimpleOperator (const char* opTxt, const size_t numOper, Operands& opers)
> > + :SimpleGenericOperator(opTxt, numOper, opers) {}
> > + TextSimpleOperator(const std::string& opTxt, Operands& opers)
> > + :SimpleGenericOperator(opTxt, opers) {}
> > +
> > + virtual ~TextSimpleOperator() {}
> > +
> > + /** Returns string represented by this operator correctly transformed
> > + * according to font encoding.
> > + * @param str String to be set.
> > + */
> > + virtual void getEncodedText(std::string& str);
> > +
> > +}; // class TextSimpleOperator
> > +
> > +
> >
> > //==========================================================
> > // Concrete implementations of CompositePdfOperator
> > Index: pdfedit-patches/src/kernel/stateupdater.h
> > ===================================================================
> > --- pdfedit-patches.orig/src/kernel/stateupdater.h 2008-08-26 13:53:02.000000000 -0600
> > +++ pdfedit-patches/src/kernel/stateupdater.h 2008-08-26 16:33:55.000000000 -0600
> > @@ -225,6 +225,22 @@ inline bool
> > isSimpleOp (const StateUpdater::CheckTypes& chck)
> > { return ('\0' == chck.endTag[0]); }
> >
> > +/**
> > + * Is it a text operator (one which holds text to be displayed).
> > + * @param chck Check type structure.
> > + * @return True if chck is a text operator, false otherwise.
> > + */
> > +inline bool isTextOp(const StateUpdater::CheckTypes& chck)
> > +{
> > + if (!strcmp(chck.name, "TJ") ||
> > + !strcmp(chck.name, "Tj") ||
> > + !strcmp(chck.name, "\"") ||
> > + !strcmp(chck.name, "'")
> > + )
> > + return true;
> > + return false;
> > +}
> > +
> >
> >
> > //==========================================================
-- 
Michal Hocko