>>> I added a static AuthorList::Color(int id) method in stead of a
>>> Change::Color() method, because I feel that the AuthorList is
>>> deciding on which author gets which id and thus which color. If you
>>> think it should be done differently, I'm sure you'll tell me and
I'll
>>> change
>> it.
>
> I still prefer Change::color(), since authorlist does not have any
> private information that helps it to decide. Moreover, the rowpainte
> code does not need to know that the color depends on the author (it
> could depend on how old the change is, for example).
Very good point, learning every every day.. I'll just move the code into
Change::color() .
(although I still feel that Change should not distribute color codes,
but I don't want to introduce too many methods that do nothing besides
relaying, unless you'd say I should).
> + static ColorCode Color(int id);
>
> Please no method name starting with a capital letter.
Sorry, this shouldn't have been done.
> In general in this code, it would make more sense to have a
> Change const & change = par_.lookupChange(pos);
> instead of calling lookupChange repeatedly (not your fault,
> I know).
Yes, it's a general problem that I try to write patches that change the
least as possible, so you can easily review and understand the changes,
but this might introduce the 'errors' you mentioned. Moreover, I try to
do things the same as it is already done in the code somewhere else,
but, considering that that may be written a long time ago by different
authors, that might not always be the best way (also referring to the
comment of Andre on using char_type bb[] = { one_char }, although the
comment is right and should be made). And of course, I have to improve
my own skills.
I shall write a new patch this evening.
Vincent
