Hi Ugras,
I am glad to see that you worked on this.
Am Mittwoch, 27. September 2006 11:29 schrieb Ozgur Ugras BARAN:
> Dear all,
>
> At last I had time and play with the insetcommandparams as requested
> on nomencl discussion. The new implementation keeps the contents and
> options in lists. There is also support for individual parameters with
> std::multimaps for future implementations. I haven't put any kind of
> nested command support, since it would be much easier when we migrate
> to the XML format.
What do you mean with nested command support?
> After the development, the scanCommand() becomes obsolete. However,
> insetBibitem uses scan command directly. Therefore, I will keep it
> there until the development approved. Then I can update insetBibitem
> to use new insetcommandparams.
>
> Well.. I hope what I have done here complies with what you asked for.. :)
I see that you kept the old interface. That is good because it allows the
old code to coexist for some time, but it creates also more overhead, so
one alwasys has to weigh the pros and cons.
I created a diff of your changes and am commenting that below. Please send
a diff the next time, that makes it easier to see what you changed.
Index: src/insets/insetcommandparams.C
===================================================================
--- src/insets/insetcommandparams.C (Revision 15173)
+++ src/insets/insetcommandparams.C (Arbeitskopie)
@@ -17,9 +17,7 @@
using std::string;
-using std::endl;
-using std::ostream;
-
+using std::pair;
InsetCommandParams::InsetCommandParams()
{}
@@ -28,12 +26,27 @@ InsetCommandParams::InsetCommandParams()
InsetCommandParams::InsetCommandParams(string const & n,
string const & c,
string const & o,
- string const & s)
- : cmdname(n), contents(c), options(o), sec_options(s),
+ string const & s,
+ string const & sc)
+ : cmdname(n), //contents(c), options(o), sec_options(s),
sec_contents(sc),
preview_(false)
-{}
+{
+ if(!c.empty()) setContents(c);
+ if(!sc.empty()) setSecContents(sc);
+ if(!o.empty()) setOptions(o);
+ if(!s.empty()) setSecOptions(s);
+}
Formatting is wrong. Never put the body of the if statement on the same
line. See our code rules in development/Code_rules. There are more
formatting issues, e.g. spurious whitespace at line ends. I fixed these
and attach your two files with only the formatting changed. Please have a
look at the differences and try to follow the rules next time.
+InsetCommandParams::InsetCommandParams(string const & n,
+ vector<string> const & c,
+ vector<string> const & o)
+ : cmdname(n), contents_(c) , options_(o),
+ preview_(false)
+{
+//TODO never tested, but should work!! :/
+}
+//TODO apparently this function is only needed for insetBibItem.. remove
after fix..
void InsetCommandParams::scanCommand(string const & cmd)
{
string tcmdname, toptions, tsecoptions, tcontents;
@@ -106,22 +119,43 @@ void InsetCommandParams::scanCommand(str
}
+
void InsetCommandParams::read(LyXLex & lex)
{
string token;
-
+///////////////////////////////////////////////////////////////////////
+// left for compatibility issues. remove if necessary!!
if (lex.eatLine()) {
token = lex.getString();
- scanCommand(token);
+ //scanCommand(token);
} else {
lex.printError("InsetCommand: Parse error: `$$Token'");
}
+///////////////////////////////////////////////////////////////////////
+ options_.clear();
+ contents_.clear();
while (lex.isOK()) {
lex.next();
token = lex.getString();
if (token == "\\end_inset")
break;
+ if (token == "\\command_name") {
+ lex.eatLine();
+ setCmdName(clearText(lex.getString()));
+ }
+ if (token == "\\add_contents") {
+ lex.eatLine();
+ addContents(clearText(lex.getString()));
+ }
+ if (token == "\\add_options") {
+ lex.eatLine();
+ addOptions(clearText(lex.getString()));
+ }
+ if (hasKey(token)) {
+ lex.eatLine();
+
entrymap_.insert(pair<string,string>(token,clearText(lex.getString())));
+ }
if (token == "preview") {
lex.next();
preview_ = lex.getBool();
The new keywords you introduced could be improved I think. IMO a syntax
\begin_inset Command
name cite
optparam "page~35"
param "mybook"
\end_inset
instead of the old
\begin_inset LatexCommand \cite[page~35]{mybook}
\end_inset
looks better than \add_contents and \add_options. The parsing could be done
with
lex.next();
name = lex.getString();
while (lex.isOK()) {
lex.next();
token = lex.getString();
if (token == "\\end_inset")
break;
else if (token == "param") {
lex.next();
addContents(clearText(lex.getString()));
} else if (token == "optparam") {
lex.next();
addOptions(clearText(lex.getString()));
}
}
@@ -129,40 +163,58 @@ void InsetCommandParams::read(LyXLex & l
}
if (token != "\\end_inset") {
lex.printError("Missing \\end_inset at this point. "
- "Read: `$$Token'");
+ "Read: `$$Token'");
}
}
+// it is necessary to put the input into {}, since some options or
contents
+// should be set even if it is null..
I see you already noticed some of the more ugly things :-)
We definitly need to know whether an option was given, but empty, or
whether it was not given at all. The natural way to do that with LyXLeX is
to use double quotes:
optparam ""
would result in an empty parameter, and the other case would be to not give
optparam at all. If you do that then you don't need clearText bewlo.
+// nest depth stuff that was introduced in scanCommand should be avoided,
+// since user can enter an erroneous input like foo{boo .
+// It is not necessary to mix up things even more! :)
+string InsetCommandParams::clearText(string const & cmd)
+{
+ return string(cmd,
+ cmd.find_first_not_of('{' , 0 ),
+ cmd.find_last_not_of ('}' , cmd.size()));
+}
+
void InsetCommandParams::write(ostream & os) const
{
+unsigned int ii;
os << "LatexCommand " << getCommand() << "\n";
+ os << "\\command_name " << '{' << getCmdName() << '}' << "\n";
+ for( ii = 0 ; ii < options_.size() ; ++ii )
+ os << "\\add_options " << '{' << getOptions(ii) << '}' << "\n";
+ for( ii = 0 ; ii < contents_.size() ; ++ii )
+ os << "\\add_contents " << '{' << getContents(ii) << '}' << "\n";
}
formatting again
+
string const InsetCommandParams::getCommand() const
{
string s;
+ unsigned int ii;
if (!getCmdName().empty()) s += '\\' + getCmdName();
- if (!getOptions().empty()) s += '[' + getOptions() + ']';
- if (!getSecOptions().empty()) {
- // for cases like \command[][sec_option]{arg}
- if (getOptions().empty()) s += "[]";
- s += '[' + getSecOptions() + ']';
- }
- s += '{' + getContents() + '}';
+ for( ii = 0 ; ii < options_.size() ; ++ii ) s += '[' + getOptions(ii)
+ ']';
+ if (getNumContents() == 0) s += "{}";
+ for( ii = 0 ; ii < contents_.size() ; ++ii ) s += '{' + getContents(ii)
+ '}';
return s;
}
Looks good apart from formatting, but it would be nice if it would be
possible to mix optional and required parameters (IIRC some commands need
that). Maybe store all in one vector, and an additional flag telling
whether they are optional or not?
-
bool operator==(InsetCommandParams const & o1,
InsetCommandParams const & o2)
{
- return o1.getCmdName() == o2.getCmdName()
- && o1.getContents() == o2.getContents()
- && o1.getOptions() == o2.getOptions()
- && o1.getSecOptions() == o2.getSecOptions()
- && o1.preview() == o2.preview();
+ bool isequal = (o1.getNumOptions() == o2.getNumOptions()) &&
+ (o1.getNumContents() == o2.getNumContents());
+ int ii;
+ for( ii = 0 ; ii < o1.getNumOptions() && isequal ; ++ii )
+ isequal = o1.getOptions(ii) == o2.getOptions(ii);
+ for( ii = 0 ; ii < o1.getNumContents() && isequal ; ++ii )
+ isequal = o1.getContents(ii) == o2.getContents(ii);
+ return isequal && (o1.getCmdName() == o2.getCmdName());
}
Why do you introduce isequal? That prevents shortcircuiting if we.g. the
first or secvond test fails.
Index: src/insets/insetcommandparams.h
===================================================================
--- src/insets/insetcommandparams.h (Revision 15173)
+++ src/insets/insetcommandparams.h (Arbeitskopie)
@@ -14,10 +14,17 @@
#include <string>
#include <iosfwd>
+#include <vector>
+#include <map>
+#include "debug.h"
class LyXLex;
-
+using std::string;
+using std::endl;
+using std::ostream;
+using std::vector;
+using std::map;
class InsetCommandParams {
public:
@@ -25,53 +32,97 @@ public:
InsetCommandParams();
///
explicit InsetCommandParams(std::string const & n,
- std::string const & c = std::string(),
- std::string const & o = std::string(),
- std::string const & s = std::string());
+ std::string const & c = std::string(),
+ std::string const & o = std::string(),
+ std::string const & s = std::string(),
+ std::string const & sc = std::string());
Please don't introduce sc. This constructor should be deprecated, and not
extended. The new one below is better.
+ explicit InsetCommandParams(std::string const & n,
+ std::vector<std::string> const & c,
+ std::vector<std::string> const & o);
///
void read(LyXLex &);
- /// Parse the command
+ ///Parse the command
Why did you change whitespace?
void scanCommand(std::string const &);
///
void write(std::ostream &) const;
- /// Build the complete LaTeX command
- std::string const getCommand() const;
///
std::string const & getCmdName() const { return cmdname; }
///
- std::string const & getOptions() const { return options; }
+ std::string const & getOptions() const { return
(options_.size())?options_[0]:null_str; }
Don't use null_str, but simply std::string().
+ ///
+ std::string const & getSecOptions() const { return
(options_.size()>1)?options_[1]:null_str; }
+ ///
+ std::string const & getContents() const { return
(contents_.size() )?contents_[0]:null_str; }
+ ///
+ std::string const & getSecContents() const { return (contents_.size()
>1)?contents_[1]:null_str; }
///
- std::string const & getSecOptions() const { return sec_options; }
+ int getNumOptions() const { return options_.size(); }
///
- std::string const & getContents() const { return contents; }
+ int getNumContents() const { return contents_.size(); }
///
void setCmdName(std::string const & n) { cmdname = n; }
///
- void setOptions(std::string const & o) { options = o; }
+ void setOptions(std::string const & c){ setElement(options_, 0, c);}
///
- void setSecOptions(std::string const & s) { sec_options = s; }
+ void setSecOptions(std::string const & c) { setElement(options_, 1, c);}
///
- void setContents(std::string const & c) { contents = c; }
+ void setContents(std::string const & c) { setElement(contents_, 0, c);}
+ ///
+ void setSecContents(std::string const & s) { setElement(contents_, 1,
s);}
///
bool preview() const { return preview_; }
///
void preview(bool p) { preview_ = p; }
+ ///
+ void addOptions(std::string const & o) { options_.push_back(o); }
+ ///
+ void addContents(std::string const & c) { contents_.push_back(c); }
+ ///
+ std::string const & getOptions(int i) const { return options_[i]; }
+ ///
+ std::string const & getContents(int i) const { return contents_[i]; }
+ ///
+ std::vector<std::string> getValuesForKeys(std::string const &
v) /*TODO*/;
+ ///
+ std::string getValueForKeys(std::string const & v) {return
(*(std::multimap<std::string,std::string>::iterator(entrymap_.find(v)))).first;}
+ ///
+ /// Build the complete LaTeX command
+ std::string const getCommand() const;
+
private:
///
- std::string cmdname;
+ std::string clearText(std::string const &);
+ ///
+ bool hasKey(std::string const & k) {return !(find(keys_.begin() ,
keys_.end() ,k) == keys_.end());};
///
- std::string contents;
+ void setElement(std::vector<std::string> & , int , std::string const &
c);
///
- std::string options;
+ std::string cmdname;
///
- std::string sec_options;
+ std::string null_str;
///
bool preview_;
-};
+ ///
+ std::vector<std::string> contents_;
+ ///
+ std::vector<std::string> options_;
+ ///
+ std::vector<std::string> keys_;
+ ///
+ std::multimap<std::string,std::string> entrymap_;
Why multimap? IMO one parameter should only appear once, so a map would be
OK.
+};
///
+inline void InsetCommandParams::setElement(std::vector<std::string> &
vec_ , int ii, std::string const & c)
+{
+ for (int i = vec_.size() ; i <= ii ; ++i ) {
+ vec_.push_back("");
+ }
+ vec_[ii] = c;
+}
+///
bool operator==(InsetCommandParams const &, InsetCommandParams const &);
///
All in all I think it goes into the right direction, but needs a bit of
polishing. Another idea I had was whether we should simply assign a name
to all parameters. We still need the order for LaTeX, and we also need to
know whether it is optional or not, but I think in LyX it would make the
code better if we could simply access a parameter by name instead of
position. The LyX file would then look like this:
\begin_inset Command
name cite
optparam text_after "page~35"
param citation "mybook"
\end_inset
I am however not so sure how to store this in memory. A map would not work,
since the ordering is not preserved, but maybe we could use a
map<std::string, int> for storing indices into the parameter vector. What
do you think?
Georg
