[prev in list] [next in list] [prev in thread] [next in thread] 

List:       lyx-devel
Subject:    Re: r36974 - in lyx-devel/trunk/src: . insets mathed
From:       Richard Heck <rgheck () comcast ! net>
Date:       2010-12-20 23:01:03
Message-ID: 4D0FE02F.9090008 () comcast ! net
[Download RAW message or body]

On 12/20/2010 05:29 PM, Vincent van Ravesteijn wrote:
>
>> Author: rgheck
>> Date: Mon Dec 20 22:55:09 2010
>> New Revision: 36974
>> URL: http://www.lyx.org/trac/changeset/36974
>>
>> Log:
>> Finish disentangling tocString(). We introduce a new method, forToc(),
>> that also makes sure it doesn't do more work than it needs to do, by
>> limiting the size to 40 characters. Previously, InsetBranch::addToToc()
>> would have added a string representing the entire contents of the
>> branch! It's hard to imagine that having to recalculate that sort of
>> thing doesn't cause some problems with speed, especially in documents
>> with lots of notes and branches and such.
>>
>
>
> I've some questions about this.
>
I look forward to answering them.

> 3. You gave a docstring & argument to forToc, while toString has an 
> odocstream & argument. Why the confusing difference ?
>
Because I need to be able to find out how many characters have been 
written, and you can't find the length of an odocstream. I thought about 
using an odocstringstream, but that seems unnecessarily complicated.

> 1. Why do you prefer to have a separate function which needs an 
> implementation in dozens of functions while it does more or less the 
> same as toString(). I mean, why don't you like my solution of 
> AS_STR_INTOC ? We could also add AS_STR_SHORTEN. And generalize 
> toString to
> - void toString(odocstream & os, int options = AS_STR_NONE, size_t 
> max_len = 0);
>
First, as I said, you can't use an odocstream here, if you want to check 
length. But the main reason for the extra method is that there are 
differences in what it makes sense to do in the different cases. In many 
cases, we will do roughly the same thing, hence....

> 2. I especially don't like this kind of duplicated code:
>
> {{{
> void InsetQuotes::toString(odocstream & os) const
> {
>     os << displayString();
> }
>
>
> void InsetQuotes::forToc(docstring & os, size_t) const
> {
>     os += displayString();
> }
> }}}
>
> and there are more of these: InsetBranch, InsetRef, InsetSpecialChar, 
> InsetMathHull.
>
the fact that we get similar code in this case. But if you look at 
InsetCitation, for example, or InsetSpace, then you will see what I 
mean. In the case of InsetQuotes, I'm tempted just to have:

void InsetQuotes::forToc(docstring & os, size_t) const
{
     os += '\'';
}

and be done with it, but perhaps people would complain.

> Now you have this code in InsetRef:
>
> {{{
> void InsetRef::toString(odocstream & os) const
> {
>     plaintext(os, OutputParams(0));
> }
>
>
> void InsetRef::forToc(docstring & os, size_t) const
> {
>     odocstringstream ods;
>     plaintext(ods, OutputParams(0));
>     os += ods.str();
> }
> }}}
>
I meant to change this. Why do we need to have the label referenced in 
the TOC? That is just a waste of space and of computation time. So now 
it just writes "#". I suppose we could also do something like "[REF]", 
translated, of course. Either is sufficient for the purpose.

> 2. I would prefer a different name than "forToc". Especially since it 
> does almost the same as toString(), I would like to see a name that 
> resembles that it does the same (almost), like tocString() maybe ;).
>
tocString() seemed dangerous. Too easy to type toString() when you mean 
tocString(), or vice versa. But I'll be happy to change it to something 
else, if we can think of something good. asTocString()? And change 
toString() to asString()?

> 4. I find it a bit confusing that InsetBranch::addToToc() does not use 
> InsetBranch::forToc(). So, forToc() is only used when this branch is 
> in an InsetText which is added to the TOC, while when adding the 
> branch to the toc we override the behaviour by calling 
> InsetText::forToc directly.
>
It is generally true that an Inset's forToc() behavior and its 
addToToc() behavior are different. The former, quite generally, has to 
do with what happens when we encounter the Inset while building a string 
for the TOC; the latter has to do with what this Inset wants to add to 
the TOC itself. For example, Footnotes add themselves to the TOC, but 
they do not appear in the strings we write to it for other things, so it 
would be wrong to try to call InsetFoot::forToc() from 
InsetFoot::addToToc(). In the case of Branch, we see this, primarily, in 
the fact that, in one case, it matters if the branch is on, but not in 
the other.

Richard

[prev in list] [next in list] [prev in thread] [next in thread]