Per Abdel's suggestion that we focus on bug-fixing at this point, this will be the last patch in this series for a bit. But I wanted to get this done before I forget what it is I was doing, so here it is.

The idea behind this patch is to make real key-value support for InsetCommand parameters possible. This should be particularly useful for the listings version of InsetInclude, though we would need some kind of UI for it before it would really be helpful. (See below for some thoughts.) This doesn't substantially change anything else, though some things do get re-arranged a bit.

Basically, the idea is this. First, we introduce a whole range of parameter types: Normal LaTeX optional and required parameters; ones for LyX's internal use (like embed); and finally, in connection with keyval, ones that represent keys and ones that represent optional and required arguments where the keyval stuff will appear. (I'm assuming here that there will always be exactly one of those, and that it will accept only keyval-type material.) The parameters themselves are stored in a map, so it's really only the output routines that need to care about the different types of parameters.

Regarding the frontend, it seems to me that something like the following would work:
(i) scan the parameter list for LATEX_KEY type parameters
(ii) the dialog will have a series of lines, each of which has a combo box listing the acceptable keys and a QLineEdit for entering its value, as well as a "delete" button of some sort for removing this key and its value
(iii) there should be an "add line" button to add a new line, activated only when all other lines are filled with values
Probably not even too hard.


git-svn-id: svn://svn.lyx.org/lyx/lyx-devel/trunk@23235 a592a061-630c-0410-9148-cb99ea01b6c8

src/insets/InsetBibitem.cpp

@@ -53,8 +53,8 @@ ParamInfo const & InsetBibitem::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("label", true);
-		param_info_.add("key", false);
+		param_info_.add("label", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("key", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetBibtex.cpp

@@ -57,10 +57,10 @@ ParamInfo const & InsetBibtex::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("options", true);
-		param_info_.add("btprint", true);
-		param_info_.add("bibfiles", false);
-		param_info_.add("embed", false);
+		param_info_.add("btprint", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("bibfiles", ParamInfo::LATEX_REQUIRED);
+		param_info_.add("embed", ParamInfo::LYX_INTERNAL);
+		param_info_.add("options", ParamInfo::LYX_INTERNAL);
 	}
 	return param_info_;
 }

src/insets/InsetCitation.cpp

@@ -390,9 +390,9 @@ ParamInfo const & InsetCitation::findInfo(string const & /* cmdName */)
 	// we have to allow both here. InsetCitation takes care that
 	// LaTeX output is nevertheless correct.
 	if (param_info_.empty()) {
-		param_info_.add("after", true);
-		param_info_.add("before", true);
-		param_info_.add("key", false);
+		param_info_.add("after", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("before", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("key", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetCommandParams.cpp

@@ -30,10 +30,10 @@
 #include "Lexer.h"
 
 #include "support/debug.h"
+#include "support/docstream.h"
 #include "support/ExceptionMessage.h"
 #include "support/gettext.h"
 #include "support/lstrings.h"
-#include "support/docstream.h"
 
 #include <boost/assert.hpp>
 
@@ -42,55 +42,68 @@ using namespace lyx::support;
 
 namespace lyx {
 
-ParamInfo::ParamData::ParamData(std::string const & s, bool b) :
-	name_(s), optional_(b)
+ParamInfo::ParamData::ParamData(std::string const & s, ParamType t) :
+	name_(s), type_(t)
 {}
 
 
+bool ParamInfo::ParamData::isOptional() const
+{
+	return type_ == ParamInfo::LATEX_OPTIONAL ||
+	       type_ == ParamInfo::LATEX_KV_OPTIONAL;
+}
+
+
+bool ParamInfo::ParamData::isKeyValArg() const
+{
+	return type_ == ParamInfo::LATEX_KV_REQUIRED 
+		|| type_ == ParamInfo::LATEX_KV_OPTIONAL;
+}
+
+
 bool ParamInfo::ParamData::operator==(ParamInfo::ParamData const & rhs) const
 {
-	return name() == rhs.name() && isOptional() == rhs.isOptional();
+	return name() == rhs.name() && type() == rhs.type();
 }
 
 
 bool ParamInfo::hasParam(std::string const & name) const
 {
 	const_iterator it = begin();
-	for (; it != end(); ++it) {
+	const_iterator last = end();
+	for (; it != last; ++it) {
 		if (it->name() == name)
-			return true;
+			return !it->isKeyValArg();
 	}
 	return false;
 }
 
 
-void ParamInfo::add(std::string const & name, bool opt)
+void ParamInfo::add(std::string const & name, ParamType type)
 { 
-	info_.push_back(ParamData(name, opt)); 
+	info_.push_back(ParamData(name, type)); 
 }
 
 
 bool ParamInfo::operator==(ParamInfo const & rhs) const
 {
-	// the idea here is to check each ParamData for equality
-	const_iterator itL  = begin();
-	const_iterator itR  = rhs.begin();
-	const_iterator endL = end();
-	const_iterator endR = rhs.end();
-	while (true) {
-		// if they both end together, return true
-		if (itL == endL && itR == endR)
-				return true;
-		// but if one ends before the other, return false
-		if (itL == endL || itR == endR)
+	if (size() != rhs.size())
 		return false;
-		//check this one for equality
-		if (*itL != *itR)
-			return false;
-		// equal, so check the next one
-		++itL;
-		++itR;
+	return equal(begin(), end(), rhs.begin());
 }
+
+
+ParamInfo::ParamData const & 
+	ParamInfo::operator[](std::string const & name) const
+{
+	BOOST_ASSERT(hasParam(name));
+	const_iterator it = begin();
+	const_iterator last = end();
+	for (; it != last; ++it) {
+		if (it->name() == name)
+			return *it;
+	}
+	return *it; // silence warning
 }
 
 
@@ -318,6 +331,65 @@ void InsetCommandParams::write(ostream & os) const
 }
 
 
+docstring InsetCommandParams::makeKeyValArgument() const
+{
+	odocstringstream os;
+	bool didone = false;
+	ParamInfo::const_iterator it  = info_.begin();
+	ParamInfo::const_iterator end = info_.end();
+	for (; it != end; ++it) {
+		if (!it->isKey())
+			continue;
+		string const & name = it->name();
+		docstring const & data = (*this)[name];
+		if (data.empty())
+			continue;
+		if (didone)
+			os << ",";
+		else 
+			didone = true;
+		os << from_utf8(name) << "=" << data;
+	}
+	return os.str();
+}
+
+
+bool InsetCommandParams::writeEmptyOptional(ParamInfo::const_iterator ci) const
+{
+	if (!ci->isOptional())
+		BOOST_ASSERT(false);
+	++ci; // we want to start with the next one
+	ParamInfo::const_iterator end = info_.end();
+	for (; ci != end; ++ci) {
+		switch (ci->type()) {
+		case ParamInfo::LATEX_KEY:
+		case ParamInfo::LYX_INTERNAL:
+			break;
+
+		case ParamInfo::LATEX_REQUIRED:
+		case ParamInfo::LATEX_KV_REQUIRED:
+			return false;
+
+		case ParamInfo::LATEX_OPTIONAL: {
+			std::string const & name = ci->name();
+			docstring const & data = (*this)[name];
+			if (!data.empty())
+				return true;
+			break;
+		}
+
+		case ParamInfo::LATEX_KV_OPTIONAL: {
+			docstring data = makeKeyValArgument();
+			if (!data.empty())
+				return true;
+			break;
+		}
+		} //end switch
+	}
+	return false;
+}
+
+
 docstring const InsetCommandParams::getCommand() const
 {
 	docstring s = '\\' + from_ascii(cmdName_);
@@ -326,33 +398,45 @@ docstring const InsetCommandParams::getCommand() const
 	ParamInfo::const_iterator end = info_.end();
 	for (; it != end; ++it) {
 		std::string const & name = it->name();
+		switch (it->type()) {
+		case ParamInfo::LATEX_KEY:
+		case ParamInfo::LYX_INTERNAL:
+			break;
+
+		case ParamInfo::LATEX_REQUIRED: {
 			docstring const & data = (*this)[name];
-		if (!it->isOptional()) {
 			s += '{' + data + '}';
 			noparam = false;
-			continue;
+			break;
 		}
+		case ParamInfo::LATEX_KV_REQUIRED: {
+			s += "{" + makeKeyValArgument() + "}";
+			noparam = false;
+			break;
+		}
+		case ParamInfo::LATEX_OPTIONAL: {
+			docstring const & data = (*this)[name];
 			if (!data.empty()) {
 				s += '[' + data + ']';
 				noparam = false;
-			continue;
-		}
-		// This param is therefore optional but empty.
-		// But we need to write it anyway if nonempty
-		// optional parameters follow before the next
-		// required parameter.
-		ParamInfo::const_iterator it2 = it;
-		for (++it2; it2 != end; ++it2) {
-			if (!it2->isOptional())
-				break;
-			std::string const & name2 = it2->name();
-			docstring const & data2 = (*this)[name2];
-			if (!data2.empty()) {
+			} else if (writeEmptyOptional(it)) {
 					s += "[]";
 					noparam = false;
+			}
 			break;
 		} 
+		case ParamInfo::LATEX_KV_OPTIONAL: {
+			docstring data = makeKeyValArgument();
+			if (!data.empty()) {
+				s += '[' + data + ']';
+				noparam = false;
+			} else if (writeEmptyOptional(it)) {
+					s += "[]";
+					noparam = false;
 				}
+			break;
+		}
+		} //end switch
 	}
 	if (noparam)
 		// Make sure that following stuff does not change the
@@ -362,18 +446,11 @@ docstring const InsetCommandParams::getCommand() const
 }
 
 
-namespace {
-	//predicate for what follows
-	bool paramIsNonOptional(ParamInfo::ParamData pi)
-	{
-		return !pi.isOptional();
-	}
-}
-
 docstring const InsetCommandParams::getFirstNonOptParam() const
 {
 	ParamInfo::const_iterator it = 
-		find_if(info_.begin(), info_.end(), paramIsNonOptional);
+		find_if(info_.begin(), info_.end(), 
+			not1(mem_fun_ref(&ParamInfo::ParamData::isOptional)));
 	if (it == info_.end())
 		BOOST_ASSERT(false);
 	return (*this)[it->name()];
@@ -383,8 +460,7 @@ docstring const InsetCommandParams::getFirstNonOptParam() const
 docstring const & InsetCommandParams::operator[](string const & name) const
 {
 	static const docstring dummy; //so we don't return a ref to temporary
-	if (!info_.hasParam(name))
-		BOOST_ASSERT(false);
+	BOOST_ASSERT(info_.hasParam(name));
 	ParamMap::const_iterator data = params_.find(name);
 	if (data == params_.end() || data->second.empty())
 		return dummy;
@@ -394,8 +470,8 @@ docstring const & InsetCommandParams::operator[](string const & name) const
 
 docstring & InsetCommandParams::operator[](string const & name)
 {
-	if (!info_.hasParam(name))
-		BOOST_ASSERT(false);
+	BOOST_ASSERT(info_.hasParam(name));
+	ParamInfo::ParamData const & pd = info_[name];
 	return params_[name];
 }
 

src/insets/InsetCommandParams.h

@@ -29,17 +29,50 @@ class Lexer;
 
 class ParamInfo {
 public:
+	/// Types of parameters
+	/// WARNING: LATEX_KV_* `parameters' aren't really parameters at all
+	/// but merely markers for where the keyval-type parameters should
+	/// appear in the LaTeX output. ParamInfo::hasParam(name) therefore 
+	/// returns FALSE if the corresponding `parameter' is of type
+	/// LATEX_KV_*.
+	/// It is assumed here that there is exactly one argument that accepts
+	/// the key=value pairs.
+	enum ParamType {
+		LATEX_OPTIONAL,    /// normal optional argument
+		LATEX_REQUIRED,    /// normal required argument
+		LATEX_KV_OPTIONAL, /// optional argument that uses keyval
+		LATEX_KV_REQUIRED, /// required argument that uses keyval
+		LATEX_KEY,         /// a key to be used with keyval argument
+		LYX_INTERNAL       /// a parameter used internally by LyX
+	};
 	///
 	class ParamData {
 	// No parameter may be named "preview", because that is a required
 	// flag for all commands.
 	public:
 		///
-		ParamData(std::string const &, bool);
+		ParamData(std::string const &, ParamType);
 		///
 		std::string name() const { return name_; }
 		///
-		bool isOptional() const { return optional_; }
+		ParamType type() const { return type_; }
+		/// whether this is a key for use with keyval
+		bool isKey() const
+			{ return type_ == LATEX_KEY; }
+		/// whether this is an optional LaTeX argument
+		inline bool isOptional() const;
+		/// whether this is a keyval argument
+		inline bool isKeyValArg() const;
+#if 0
+		//presently unused but perhaps useful at some point
+		/// whether this is a required LaTeX argument
+		bool isRequired() const
+			{ return type_ == ParamInfo::LATEX_REQUIRED ||
+					type_ == ParamInfo::LATEX_KV_REQUIRED; }
+		/// whether this is a LaTeX argument
+		inline bool isLaTeXArgument() const
+			{ return isOptional() || isRequired(); }
+#endif
 		///
 		bool operator==(ParamData const &) const;
 		/// 
@@ -49,11 +82,11 @@ public:
 		///
 		std::string name_;
 		///
-		bool optional_;
+		ParamType type_;
 	};
 
 	/// adds a new parameter
-	void add(std::string const & name, bool optional);
+	void add(std::string const & name, ParamType type);
 	///
 	bool empty() const { return info_.empty(); }
 	///
@@ -61,12 +94,17 @@ public:
 	///
 	typedef std::vector<ParamData>::const_iterator const_iterator;
 	///
-	const_iterator begin() const { return info_.begin(); }
-	///
-	const_iterator end() const { return info_.end(); }
+	const_iterator const begin() const { return info_.begin(); }
 	///
+	const_iterator const end() const { return info_.end(); }
+	/// \return true if name corresponds to a parameter of some sort.
+	/// \return false if the parameter does not exist at all of it it 
+	/// corresponds to a `parameter' of type LATEX_KV_*; these do not 
+	/// really represent parameters but just argument places.
 	bool hasParam(std::string const & name) const;
 	///
+	ParamData const & operator[](std::string const & name) const;
+	///
 	bool operator==(ParamInfo const &) const;
 private:
 	///
@@ -103,8 +141,12 @@ public:
 	/// ways that make removal hard.
 	docstring const getFirstNonOptParam() const;
 	/// get parameter \p name
+	/// WARNING: You cannot access LATEX_KV_* arguments in this way.
+	/// LyX will assert if you attempt to do so.
 	docstring const & operator[](std::string const & name) const;
 	/// set parameter \p name
+	/// WARNING: You cannot access LATEX_KV_* arguments in this way.
+	/// LyX will assert if you attempt to do so.
 	docstring & operator[](std::string const & name);
 	///
 	bool preview() const { return preview_; }
@@ -128,6 +170,11 @@ private:
 	static bool isCompatibleCommand(InsetCode code, std::string const & s);
 	///
 	std::string getDefaultCmd(InsetCode);
+	///
+	docstring makeKeyValArgument() const;
+	/// checks whether we need to write an empty optional parameter
+	/// \return true if a non-empty optional parameter follows ci
+	bool writeEmptyOptional(ParamInfo::const_iterator ci) const;
 	/// Description of all command properties
 	ParamInfo info_;
 	/// what kind of inset we're the parameters for
@@ -135,6 +182,9 @@ private:
 	/// The name of this command as it appears in .lyx and .tex files
 	std::string cmdName_;
 	///
+	// if we need to allow more than one value for a parameter, this
+	// could be made a multimap. it may be that the only thing that
+	// would then need changing is operator[].
 	typedef std::map<std::string, docstring> ParamMap;
 	/// The parameters, by name.
 	ParamMap params_;

src/insets/InsetFloatList.cpp

@@ -52,7 +52,7 @@ ParamInfo const & InsetFloatList::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("type", false);
+		param_info_.add("type", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetHyperlink.cpp

@@ -37,9 +37,9 @@ ParamInfo const & InsetHyperlink::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("name", true);
-		param_info_.add("target", false);
-		param_info_.add("type", false);
+		param_info_.add("name", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("target", ParamInfo::LATEX_REQUIRED);
+		param_info_.add("type", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetInclude.cpp

@@ -173,9 +173,9 @@ ParamInfo const & InsetInclude::findInfo(string const & /* cmdName */)
 	// In the other cases, this second parameter should just be empty.
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("filename", false);
-		param_info_.add("embed", false);
-		param_info_.add("lstparams", true);
+		param_info_.add("filename", ParamInfo::LATEX_REQUIRED);
+		param_info_.add("lstparams", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("embed", ParamInfo::LYX_INTERNAL);
 	}
 	return param_info_;
 }

src/insets/InsetIndex.cpp

@@ -84,7 +84,7 @@ ParamInfo const & InsetPrintIndex::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("name", false);
+		param_info_.add("name", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetLabel.cpp

@@ -37,7 +37,7 @@ ParamInfo const & InsetLabel::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("name", false);
+		param_info_.add("name", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetNomencl.cpp

@@ -39,9 +39,9 @@ ParamInfo const & InsetNomencl::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("prefix", true);
-		param_info_.add("symbol", false);
-		param_info_.add("description", false);
+		param_info_.add("prefix", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("symbol", ParamInfo::LATEX_REQUIRED);
+		param_info_.add("description", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }
@@ -92,7 +92,7 @@ ParamInfo const & InsetPrintNomencl::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("labelwidth", true);
+		param_info_.add("labelwidth", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetRef.cpp

@@ -56,8 +56,8 @@ ParamInfo const & InsetRef::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("name", true);
-		param_info_.add("reference", false);
+		param_info_.add("name", ParamInfo::LATEX_OPTIONAL);
+		param_info_.add("reference", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }

src/insets/InsetTOC.cpp

@@ -36,7 +36,7 @@ ParamInfo const & InsetTOC::findInfo(string const & /* cmdName */)
 {
 	static ParamInfo param_info_;
 	if (param_info_.empty()) {
-		param_info_.add("type", false);
+		param_info_.add("type", ParamInfo::LATEX_REQUIRED);
 	}
 	return param_info_;
 }