On 29/03/14 11:28, Vincent van Ravesteijn wrote:
> Hmm, I don't see it.
I was sure to have pushed, but I didn't ?
Check now pls.
> On a closer look at the patch, I don't think that AS_STR_PLAINTEXT should be
> an option to asString(). Plain text is an output format.
What I did, was just having a glance at 2 nearly identical methods, only that
one was checking also deleted text vs SKIPDELETED, the other one (mine old
version introduced back years ago) not. So, instead of growing even more the
other method, used nowhere except in lyxfind.cpp, I just merged back the 2
methods, introducing a further AS_STR_ flag that is only used by lyxfind.cpp.
Namely, it will force Inset::plaintext() rather than Inset::toString(). Now, in
this very moment it's just been a feature-preserving transformation, that
doesn't pretend to dig any way deep into why multiple export methods exist and
how they should map onto each other.
I can only briefly mention that I've also got the impression that the
::forToc() series seems yet another very similar one.
> However, plaintext() is often used for other things which feels a bit
> wrong. Some insets have a toString function that just call plaintext
> with a default OutputParams object, but I feel like this should be
> the other way around. If no special make-up is needed for plaintext,
> just return toString(). Also, the default OutputParams object often
> produces the correct output, because the plaintext() function doesn't
> use most of the params. However, this still is wrong. If the
> plaintext function suddenly does care about dry_run, all the callers
> are suddenly wrong.
> OutputParams::for_toc is also confused with Inset::forToc. The first is meant
> for the table of contents in an html document, the second one is used for the
> outliner in LyX.
Ah, ok, there's even further confusion on that side then!
> With respect to your patch. If we really want to say: it's ok to use
> plaintext for other things than just for output, you should create the
> OutputParams object within asString.
>
> if (options & AS_STR_FORSEARCH) {
guess you meant to say "options.for_search"...
> OutputParams params(encoding);
> params.for_search = true;
> plaintext(os, params);
> }
The problem seems only that I seemed to use more params into
lyxfind.cpp/stringifySearchBuffer():
OutputParams runparams(&buffer.params().encoding());
runparams.nice = true;
runparams.flavor = OutputParams::LATEX;
runparams.linelen = 100000; //lyxrc.plaintext_linelen;
runparams.dryrun = true;
runparams.for_search = true;
[...]
str += par.asString(pos_type(0), par.size(),
AS_STR_INSETS | AS_STR_SKIPDELETE |
AS_STR_PLAINTEXT,
&runparams);
Refactory-wise, I hadn't even tried to check what the bool for_search was used
for. I can't remember to have introduced it. However, it turns out it's used
very sporadically
$ grep for_search src/insets/*.cpp
src/insets/InsetBibtex.cpp: if (op.for_tooltip || op.for_toc ||
op.for_search) {
src/insets/InsetInclude.cpp: if (op.for_tooltip || op.for_toc ||
op.for_search) {
and, it brings further resemblances between for_toc and for_search (!).
Funny thing, is that reading the comment by Richard when he introduced
for_search in 3f748c79c (I just learnt how to use git blame):
Here again, not only were we parsing BibTeX files, since Julien's
(sensible) introduction of plaintext output for that inset, but we
were in fact writing (to disk) complete plaintext output for
included files every time we did such a search.
this sounds quite weird. Kind of, instead of for_search, should have been named
as smth like "no_side_effects".
T.
