Richard Heck wrote:
Attached is an updated patch. It's long, to be sure, but not really that
complicated. A lot of it is really code re-organization that was made
possible (and necessary) by the fact that we now have full-fledged
classes where once there was just a collection of functions. So a great
lot of this can really be ignored: Biblio.cpp and Buffer*, and almost
nothing happens in the insets/* files, either. (There are a lot of
changes in insets/InsetCitation.cpp, but these are almost all
re-arranging of code---quite a lot of stuff from the biblio namespace
was used only here and so got moved---and simple adaptations to other
changes.) So if you want to look at this, look at the definitions of the
new classes in Biblio.h and then jump ahead to line 1262. The action is
in QCitation*.
The reorganisation looks OK AFAICS, I don't have the time to look at the
details and won't in any case because I am mostly ignorant about BibTeX.
You're the expert anyway.
Some cosmetics comments:
+docstring const BibTeXInfo::getValueForField(docstring const & field) const
{
- const_iterator it = find(key);
- return it == end();
+ BibTeXInfo::const_iterator it = find(field);
+ if (it == end())
+ return docstring();
+ return it->second;
}
You can return a const ref here:
docstring const BibTeXInfo::getValueForField(docstring const & field) const
{
BibTeXInfo::const_iterator it = find(field);
if (it != end())
return it->second;
static docstring const empty_value;
return empty_value;
}
+docstring const BibTeXInfo::getValueForField(string const & field) const
+{
+ return getValueForField(from_ascii(field));
+}
Ditto, but why is there a need for this method?
Also are we sure that we don't need unicode keys for the BibKeyList? If
no, then I'd suggest to just use that:
std::map<docstring, BibTeXInfo>
About the the std::map inheritance, you have to ask yourself if you
really need the different map methods. If yes, then methods like
getValueForField are not really needed as you use the map operators
directly. But, AFAIU, we only need some const access methods so it might
be better to just put the actual map in a private member. This has the
advantage that we can change the internal structure without changing the
public interface. Something like that.
class BibKeys {
private:
typedef std::map<docstring, BibTeXInfo> Container;
public:
typedef Container::const_iterator const_iterator;
const_iterator begin() const { return data_.begin(); }
const_iterator end() const { return data_.end(); }
BibTeXInfo const & item(docstring const & key) const;
...
private:
std::map<docstring, BibTeXInfo> data_;
};
I changed from BibKeyList to BibKeys because it is not really a list.
I would also personally simplify the access method names. Instead of
offering different access method for fields, entry, I'd suggest to just
use the interface in BibTeXInfo:
getAbbreviatedAuthor(key) -> item(key).getAbbreviatedAuthor();
As you can see, I am not a big fan of redirection. I prefer explicit
rather than implicit. This facilitate further reorganization if need be.
I'd like to commit to trunk fairly soon,
Good luck with that ;-)
This
needs testing, as it could go into 1.5.x, and it'll only get it if it's
in the trunk.
I am pretty sure this won't be accepted for 1.5.x.
Abdel.
