Gregory Jefferis schreef:
This patch identifies the author of tracked changes written out to lyx file
with a hash value constructed from author+email
That way the author id never changes on write, allowing painless merging of
files containing tracked changes.
Comments very welcome - this is not final.
Thank you for contributing. I've added some comments below.
In particular I planned to change it to write the author's hash next to the
author info in the preamble. Then I would use that on read, giving the
possibility of using any arbitrary value as unique identifier.
Yes, this would be very welcome. It would also have the advantage that
the LyX file is understandable for humans and other applications, which
can't compute which hash value corresponds to which author. Besides
it'll ease the conversion of the fileformat.
+int32_t Author::findHash(docstring name, docstring email)
As findHash is a member of Author, it might be more logical to use the
private members of Author instead of specifying the two parameters.
Personally, I would have called it "computeHash". Maybe we do not need
to return the hash value, but just assign it to hash_ in the function.
The way it is now, findHash should be made const.
In other places in the code we use the crc checksum, could you use that
for this or is this different ?
int32_t -> boost::int32_t (otherwise I can't compile)
+ docstring fullauthor=name+email+'\0';
As Andre already said, try to stick to our style guidelines. So this
would be "docstring const full_author = name + email + '\0';". Moreover,
you use spaces at the beginning of a line instead of tabs.
Why is the '\0' necessary ?
+ a=Author(from_utf8(trim(token(s, '\"',
1))),from_utf8(trim(token(s, '\"', 2))));
I would leave the name and email variables for clarity, and I think we
should try to avoid assigning to a reference:
a.name_ = from_utf8(trim(token(s, '\"', 1)));
a.email_ = from_utf8(trim(token(s, '\"', 2)));
a.hash_ = findHash(a.name_, a.email_);
+ author_map[a.hash()]=buffer_aid;
+ author_map[num_authors_so_far]=buffer_aid;
Why is this necessary ? Would this no longer be needed if you write the
hash value to the author list in the file?
+ /// number of authors in the file's author list
+ unsigned int author_num;
This isn't used anywhere.
+ os << "\n\\change_deleted " <<
bparams.authors().get(change.author).hash()
Maybe we should have a pointer to the author in the Change. This is a
bit of a detour. But let's leave that for now.
Many thanks,
You too.
Greg.
Vincent
