Re: [PATCH] Branches and ToC bug
On Fri, Jan 19, 2007 at 08:55:06PM +, José Matos wrote: > On Friday 19 January 2007 4:02:43 pm Jean-Marc Lasgouttes wrote: > > OK, I'll apply it and tell Jose' that you made me do it if he > > complains. > > Abdel... hum. How were you capable of such thing? ;-) It's his pretty face. Who can resist? - Martin pgpPhcCDxFf3K.pgp Description: PGP signature
Re: [PATCH] Branches and ToC bug
Great, thanks! - Martin On Fri, 19 Jan 2007, Jean-Marc Lasgouttes wrote: "Jean-Marc" == Jean-Marc Lasgouttes <[EMAIL PROTECTED]> writes: Jean-Marc> OK, the trivial version is what I propose for 1.4.x: Jean-Marc> besides implementing InsetBranch::textString, it fixes an Jean-Marc> output order bug in writePlaintextParagraph. Jean-Marc> The 1.5 version is more complicated because I took the Jean-Marc> liberty to cleanup Paragraph::asString (remove two useless Jean-Marc> versions) and to simplify the InsetBase::textString method. I applied both. JMarc
Re: [PATCH] Branches and ToC bug
On Friday 19 January 2007 4:24:55 pm Jean-Marc Lasgouttes wrote: > I applied both. Thanks. > JMarc -- José Abílio
Re: [PATCH] Branches and ToC bug
On Friday 19 January 2007 4:02:43 pm Jean-Marc Lasgouttes wrote: > OK, I'll apply it and tell Jose' that you made me do it if he > complains. Abdel... hum. How were you capable of such thing? ;-) > JMarc -- José Abílio
Re: [PATCH] Branches and ToC bug
> "Jean-Marc" == Jean-Marc Lasgouttes <[EMAIL PROTECTED]> writes: Jean-Marc> OK, the trivial version is what I propose for 1.4.x: Jean-Marc> besides implementing InsetBranch::textString, it fixes an Jean-Marc> output order bug in writePlaintextParagraph. Jean-Marc> The 1.5 version is more complicated because I took the Jean-Marc> liberty to cleanup Paragraph::asString (remove two useless Jean-Marc> versions) and to simplify the InsetBase::textString method. I applied both. JMarc
Re: [PATCH] Branches and ToC bug
> "Abdelrazak" == Abdelrazak Younes <[EMAIL PROTECTED]> writes: >> The 1.5 version is more complicated because I took the liberty to >> cleanup Paragraph::asString (remove two useless versions) and to >> simplify the InsetBase::textString method. Abdelrazak> Whaou! Breaking news! JMarc is taking some liberty with Abdelrazak> the code!!! Well, I happen to like refactoring code like a madman as much as anyone else. It is just that I have been bound to fix others' bugs instead (more on that soon). Abdelrazak> When a patch removes more code than it adds, it's always a Abdelrazak> good sign. >> Jose', is that OK with you? Abdelrazak> I am not Jose but I think the change you made are risk Abdelrazak> free at first glance. OK, I'll apply it and tell Jose' that you made me do it if he complains. JMarc
Re: [PATCH] Branches and ToC bug
Jean-Marc Lasgouttes wrote: "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> Jean-Marc, could you do this, as it is near-trivial? I'm again Martin> having access problems (i.e., its near-complete absence) over Martin> the extended weekend :-( OK, the trivial version is what I propose for 1.4.x: besides implementing InsetBranch::textString, it fixes an output order bug in writePlaintextParagraph. The 1.5 version is more complicated because I took the liberty to cleanup Paragraph::asString (remove two useless versions) and to simplify the InsetBase::textString method. Whaou! Breaking news! JMarc is taking some liberty with the code!!! A nice thing now is that asString never calls stuff from output_plaintext, which is designed for longer texts. For example, in 1.4 a footnote will not appear in general, but it will appear in a branch inset (because plaintext is used). There is also a small improvement in the rendering of quotes. I think the resulting code is much better. When a patch removes more code than it adds, it's always a good sign. Jose', is that OK with you? I am not Jose but I think the change you made are risk free at first glance. Abdel.
Re: [PATCH] Branches and ToC bug
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes:
Martin> Jean-Marc, could you do this, as it is near-trivial? I'm again
Martin> having access problems (i.e., its near-complete absence) over
Martin> the extended weekend :-(
OK, the trivial version is what I propose for 1.4.x: besides
implementing InsetBranch::textString, it fixes an output order bug in
writePlaintextParagraph.
The 1.5 version is more complicated because I took the liberty to
cleanup Paragraph::asString (remove two useless versions) and to
simplify the InsetBase::textString method. A nice thing now is that
asString never calls stuff from output_plaintext, which is designed
for longer texts. For example, in 1.4 a footnote will not appear in
general, but it will appear in a branch inset (because plaintext is
used).
There is also a small improvement in the rendering of quotes.
I think the resulting code is much better.
Jose', is that OK with you?
JMarc
Index: src/insets/insetbranch.h
===
--- src/insets/insetbranch.h (revision 16762)
+++ src/insets/insetbranch.h (working copy)
@@ -63,7 +63,10 @@ public:
OutputParams const & runparams) const;
///
int plaintext(Buffer const &, std::ostream &,
- OutputParams const & runparams) const;
+ OutputParams const & runparams) const;
+ ///
+ int textString(Buffer const &, std::ostream & os,
+ OutputParams const & runparams) const;
///
void validate(LaTeXFeatures &) const;
///
Index: src/insets/insetbranch.C
===
--- src/insets/insetbranch.C (revision 16762)
+++ src/insets/insetbranch.C (working copy)
@@ -249,6 +249,13 @@ int InsetBranch::plaintext(Buffer const
}
+int InsetBranch::textString(Buffer const & buf, ostream & os,
+ OutputParams const & runparams) const
+{
+ return plaintext(buf, os, runparams);
+}
+
+
void InsetBranch::validate(LaTeXFeatures & features) const
{
InsetText::validate(features);
Index: src/output_plaintext.C
===
--- src/output_plaintext.C (revision 16762)
+++ src/output_plaintext.C (working copy)
@@ -200,11 +200,9 @@ void asciiParagraph(Buffer const & buf,
switch (c) {
case Paragraph::META_INSET: {
InsetBase const * inset = par.getInset(i);
- if (runparams.linelen > 0) {
-os << word;
-currlinelen += word.length();
-word.erase();
- }
+ os << word;
+ currlinelen += word.length();
+ word.erase();
OutputParams rp = runparams;
rp.depth = par.params().depth();
if (inset->plaintext(buf, os, rp)) {
Index: src/insets/insetbranch.h
===
--- src/insets/insetbranch.h (revision 16761)
+++ src/insets/insetbranch.h (working copy)
@@ -63,6 +63,8 @@ public:
int plaintext(Buffer const &, odocstream &,
OutputParams const & runparams) const;
///
+ void textString(Buffer const & buf, odocstream &) const;
+ ///
void validate(LaTeXFeatures &) const;
///
InsetBranchParams const & params() const { return params_; }
Index: src/insets/insetcharstyle.h
===
--- src/insets/insetcharstyle.h (revision 16761)
+++ src/insets/insetcharstyle.h (working copy)
@@ -85,8 +85,7 @@ public:
int plaintext(Buffer const &, odocstream &,
OutputParams const &) const;
/// the string that is passed to the TOC
- virtual int textString(Buffer const &, odocstream &,
- OutputParams const &) const;
+ virtual void textString(Buffer const &, odocstream &) const;
///
void validate(LaTeXFeatures &) const;
Index: src/insets/insetbase.h
===
--- src/insets/insetbase.h (revision 16761)
+++ src/insets/insetbase.h (working copy)
@@ -202,8 +202,7 @@ public:
virtual int docbook(Buffer const &, odocstream & os,
OutputParams const &) const;
/// the string that is passed to the TOC
- virtual int textString(Buffer const &, odocstream &,
- OutputParams const &) const { return 0; };
+ virtual void textString(Buffer const &, odocstream &) const {}
/** This enum indicates by which means the inset can be modified:
- NOT_EDITABLE: the inset's content cannot be modified at all
Index: src/insets/insetquotes.C
===
--- src/insets/insetquotes.C (revision 16761)
+++ src/insets/insetquotes.C (working copy)
@@ -328,10 +328,10 @@ int InsetQuotes::latex(Buffer const &, o
}
-int InsetQuotes::plaintext(Buffer const &, odocstream & os,
- OutputParams const &) const
+int InsetQuotes::plaintext(Buffer const & buf, odocstream & os,
+ OutputParams const &) const
{
- os << '"';
+ os << dispString(buf.params().language);
return 0;
}
@@ -354,10 +354,9 @@ int InsetQuotes::docbook(Buffer const &,
}
-int InsetQuotes::textString(Buffer const & buf, odocstream &
Re: [PATCH] Branches and ToC bug
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> This is _much_ better. BTW did you notice Martin> InsetBranch::plaintext()? The infrastructure is there, waiting Martin> to be used... My problem was to know whether we should restrict ourselves to the first paragraph. Martin> Does anyone see a gotcha or want to test this still (generally Martin> a good idea), or can I commit this to 1.5? (a similar patch to Martin> toc.C in 1.4 would also be appropriate.) >> A patch similar to above would be OK for 1.4. Martin> Jean-Marc, could you do this, as it is near-trivial? I'm again Martin> having access problems (i.e., its near-complete absence) over Martin> the extended weekend :-( I'll have a look. JMarc
Re: [PATCH] Branches and ToC bug
On Fri, 2007-01-19 at 10:03 +0100, Jean-Marc Lasgouttes wrote: > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > Martin> My original patch is still needed and fixes a useability bug. > Martin> As I wrote earlier,a ToC containing only numbers is unuseable > Martin> for navigation or outlining. I verified that it does the job. > > I know see what the bug you reported really is. Sorry for answering to > a different question :) > > Unfortunately, I do not think that this answers correctly to the > question :) It is correct, but not good :-) > The right way to make this work is to implement textString for text > insets. Since textString is supposed to be used for shortish things, I > propose the following: implement InsetBranch::textString that does > nothing if branch is disabled, and otherwise calls > paragraphs()->begin()->asString(), that is, only output its first > paragraph. This is _much_ better. BTW did you notice InsetBranch::plaintext()? The infrastructure is there, waiting to be used... > It would also be possible to start by implementing > InsetText::textString to call paragraphs()->begin()->asString(), but > this may lead to unwanted insets in the ToC. No... > Martin> Does anyone see a gotcha or want to test this still (generally > Martin> a good idea), or can I commit this to 1.5? (a similar patch to > Martin> toc.C in 1.4 would also be appropriate.) > > A patch similar to above would be OK for 1.4. Jean-Marc, could you do this, as it is near-trivial? I'm again having access problems (i.e., its near-complete absence) over the extended weekend :-( - Martin signature.asc Description: This is a digitally signed message part
Re: [PATCH] Branches and ToC bug
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> My original patch is still needed and fixes a useability bug. Martin> As I wrote earlier,a ToC containing only numbers is unuseable Martin> for navigation or outlining. I verified that it does the job. I know see what the bug you reported really is. Sorry for answering to a different question :) Unfortunately, I do not think that this answers correctly to the question :) The right way to make this work is to implement textString for text insets. Since textString is supposed to be used for shortish things, I propose the following: implement InsetBranch::textString that does nothing if branch is disabled, and otherwise calls paragraphs()->begin()->asString(), that is, only output its first paragraph. It would also be possible to start by implementing InsetText::textString to call paragraphs()->begin()->asString(), but this may lead to unwanted insets in the ToC. Martin> Does anyone see a gotcha or want to test this still (generally Martin> a good idea), or can I commit this to 1.5? (a similar patch to Martin> toc.C in 1.4 would also be appropriate.) A patch similar to above would be OK for 1.4. JMarc
Re: [PATCH] Branches and ToC bug
On Thu, 2007-01-18 at 21:09 +0200, Martin Vermeer wrote: > On Thu, Jan 18, 2007 at 06:59:10PM +0200, Martin Vermeer wrote: > > On Thu, Jan 18, 2007 at 05:02:20PM +0100, Jean-Marc Lasgouttes wrote: > > > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > > > > > >> If you want to take in account branches (or more generally nested > > > >> textinset), you should do it completely, I can't believe it is so > > > >> difficult. > > > > > > Martin> I think branches are the main problem. We don't really need to > > > Martin> see footnotes and the like in the ToC. But when using > > > Martin> branches, e.g., for translating to two languages, all of the > > > Martin> section header text will be inside one or the other branch > > > Martin> inset. There is nothing left and the ToC entry will be just > > > Martin> the number. This is IMHO the only thing that needs fixing. > > > > > > I think it would be interesting to try to implement > > > insettext::addToTOC and make it add call the existing code > > > recursively. It is probably not that much work. > > > > Probably so. > > > > > Then you would have to specialize it for branch (exit early if > > > inactive) and maybe note (never output anything). > > > > I suspect you would have to shortcircuit it for every collapsable inset > > that is closed. > > > > > This is the way I have been wanting to go for numbering, but I never > > > found the time to actually do it. I think it is much better than an > > > ad-hoc solution. > > > > This is not an ad-hoc solution. It is the ONLY solution that will put in > > ToC entries > > for headers that are not inside a branch inset but CONTAIN one or more > > branch insets. > > It's the solution to a different problem. And essentially a useability bug > > fix... a ToC > > containing only numbers is unuseable for navigation or outlining. > > > > > And I am sure there are people who put section > > > headers in minipages... > > > > Sure. That's where _your_ proposed solution is called for. > > > > > JMarc > > > > - Martin > > Actually putting section headers in minipages already works... as long > as you don't close the minipage and try to go the header inside by means > of ToC or navigation menu. The crash error message is > > Assertion triggered in virtual void > lyx::InsetCollapsable::cursorPos(const lyx::BufferView&, const > lyx::CursorSlice&, bool, int&, int&) const by failing check "status() != > Collapsed" in file insetcollapsable.C:244 > > I put this in bugzilla this afternoon. > > The mechanism by which collapsible insets are recursively traversed by > ToC I don't know. It's not the inset.addToToC mechanism. I suspect it is just the use of a pariterator (or whatever) to traverse all paragraphs in the document, also inside textinsets. addToToC is needed only for cases not covered by this, like (apparently) float, wrap and include. > Anyway, we have > to stop this crash by preventing the cursor to go into a closed inset. > How to do that? This bug is fixed in latest SVN. Great! My original patch is still needed and fixes a useability bug. As I wrote earlier,a ToC containing only numbers is unuseable for navigation or outlining. I verified that it does the job. (And Jean-Marc's objection that it only handles the first paragraph is invalid, because in the use case that the patch fixes, that is all there is. To spell it out: this is about a Default para inside a Branch inset inside an outer para of type, e.g., Section header.) Does anyone see a gotcha or want to test this still (generally a good idea), or can I commit this to 1.5? (a similar patch to toc.C in 1.4 would also be appropriate.) - Martin signature.asc Description: This is a digitally signed message part
Re: [PATCH] Branches and ToC bug
On Thu, Jan 18, 2007 at 06:59:10PM +0200, Martin Vermeer wrote: > On Thu, Jan 18, 2007 at 05:02:20PM +0100, Jean-Marc Lasgouttes wrote: > > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > > > >> If you want to take in account branches (or more generally nested > > >> textinset), you should do it completely, I can't believe it is so > > >> difficult. > > > > Martin> I think branches are the main problem. We don't really need to > > Martin> see footnotes and the like in the ToC. But when using > > Martin> branches, e.g., for translating to two languages, all of the > > Martin> section header text will be inside one or the other branch > > Martin> inset. There is nothing left and the ToC entry will be just > > Martin> the number. This is IMHO the only thing that needs fixing. > > > > I think it would be interesting to try to implement > > insettext::addToTOC and make it add call the existing code > > recursively. It is probably not that much work. > > Probably so. > > > Then you would have to specialize it for branch (exit early if > > inactive) and maybe note (never output anything). > > I suspect you would have to shortcircuit it for every collapsable inset that > is closed. > > > This is the way I have been wanting to go for numbering, but I never > > found the time to actually do it. I think it is much better than an > > ad-hoc solution. > > This is not an ad-hoc solution. It is the ONLY solution that will put in ToC > entries > for headers that are not inside a branch inset but CONTAIN one or more branch > insets. > It's the solution to a different problem. And essentially a useability bug > fix... a ToC > containing only numbers is unuseable for navigation or outlining. > > > And I am sure there are people who put section > > headers in minipages... > > Sure. That's where _your_ proposed solution is called for. > > > JMarc > > - Martin Actually putting section headers in minipages already works... as long as you don't close the minipage and try to go the header inside by means of ToC or navigation menu. The crash error message is Assertion triggered in virtual void lyx::InsetCollapsable::cursorPos(const lyx::BufferView&, const lyx::CursorSlice&, bool, int&, int&) const by failing check "status() != Collapsed" in file insetcollapsable.C:244 I put this in bugzilla this afternoon. The mechanism by which collapsible insets are recursively traversed by ToC I don't know. It's not the inset.addToToC mechanism. Anyway, we have to stop this crash by preventing the cursor to go into a closed inset. How to do that? - Martin pgpUOD3YYkDCn.pgp Description: PGP signature
Re: [PATCH] Branches and ToC bug
On Thu, Jan 18, 2007 at 05:02:20PM +0100, Jean-Marc Lasgouttes wrote: > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > >> If you want to take in account branches (or more generally nested > >> textinset), you should do it completely, I can't believe it is so > >> difficult. > > Martin> I think branches are the main problem. We don't really need to > Martin> see footnotes and the like in the ToC. But when using > Martin> branches, e.g., for translating to two languages, all of the > Martin> section header text will be inside one or the other branch > Martin> inset. There is nothing left and the ToC entry will be just > Martin> the number. This is IMHO the only thing that needs fixing. > > I think it would be interesting to try to implement > insettext::addToTOC and make it add call the existing code > recursively. It is probably not that much work. Probably so. > Then you would have to specialize it for branch (exit early if > inactive) and maybe note (never output anything). I suspect you would have to shortcircuit it for every collapsable inset that is closed. > This is the way I have been wanting to go for numbering, but I never > found the time to actually do it. I think it is much better than an > ad-hoc solution. This is not an ad-hoc solution. It is the ONLY solution that will put in ToC entries for headers that are not inside a branch inset but CONTAIN one or more branch insets. It's the solution to a different problem. And essentially a useability bug fix... a ToC containing only numbers is unuseable for navigation or outlining. > And I am sure there are people who put section > headers in minipages... Sure. That's where _your_ proposed solution is called for. > JMarc - Martin pgpZnhHlxRP79.pgp Description: PGP signature
Re: [PATCH] Branches and ToC bug
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: >> If you want to take in account branches (or more generally nested >> textinset), you should do it completely, I can't believe it is so >> difficult. Martin> I think branches are the main problem. We don't really need to Martin> see footnotes and the like in the ToC. But when using Martin> branches, e.g., for translating to two languages, all of the Martin> section header text will be inside one or the other branch Martin> inset. There is nothing left and the ToC entry will be just Martin> the number. This is IMHO the only thing that needs fixing. I think it would be interesting to try to implement insettext::addToTOC and make it add call the existing code recursively. It is probably not that much work. Then you would have to specialize it for branch (exit early if inactive) and maybe note (never output anything). This is the way I have been wanting to go for numbering, but I never found the time to actually do it. I think it is much better than an ad-hoc solution. And I am sure there are people who put section headers in minipages... JMarc
Re: [PATCH] Branches and ToC bug
On Thu, Jan 18, 2007 at 03:46:22PM +0100, Jean-Marc Lasgouttes wrote: > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > Martin> When I translate the section headers using branches, nothing > Martin> appears in the ToC, just the number. Text inside a branch > Martin> inset does not make it to the ToC or nav menu. > > Martin> Attached a fix. With this, the active/selected branch(es) will > Martin> be used to create the ToC entry. > > But this only takes in account the first paragraph of the branch > inset? If you want to take in account branches (or more generally > nested textinset), you should do it completely, I can't believe it is > so difficult. > > JMarc Eh, I looked into this and it is an entirely different problem. Handling section headers etc. inside branch (or text) insets means implementing the method inset.addToToc(buffer) for these insets. Perhaps a good idea, but an entirely unrelated exercise from handling branch insets inside section headers. - Martin pgp2C7lelkP7U.pgp Description: PGP signature
Re: [PATCH] Branches and ToC bug
On Thu, Jan 18, 2007 at 03:46:22PM +0100, Jean-Marc Lasgouttes wrote: > > "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: > > Martin> When I translate the section headers using branches, nothing > Martin> appears in the ToC, just the number. Text inside a branch > Martin> inset does not make it to the ToC or nav menu. > > Martin> Attached a fix. With this, the active/selected branch(es) will > Martin> be used to create the ToC entry. > > But this only takes in account the first paragraph of the branch > inset? Ah yes. But section headers have only one paragraph. (And if you were to put a complete section into a branch inset, your header would again be the first paragraph.) Of course there could be sub-headers in that case... OK, I'll try to do it. > If you want to take in account branches (or more generally > nested textinset), you should do it completely, I can't believe it is > so difficult. I think branches are the main problem. We don't really need to see footnotes and the like in the ToC. But when using branches, e.g., for translating to two languages, all of the section header text will be inside one or the other branch inset. There is nothing left and the ToC entry will be just the number. This is IMHO the only thing that needs fixing. > JMarc - Martin pgpf5O6TXW1R1.pgp Description: PGP signature
Re: [PATCH] Branches and ToC bug
> "Martin" == Martin Vermeer <[EMAIL PROTECTED]> writes: Martin> When I translate the section headers using branches, nothing Martin> appears in the ToC, just the number. Text inside a branch Martin> inset does not make it to the ToC or nav menu. Martin> Attached a fix. With this, the active/selected branch(es) will Martin> be used to create the ToC entry. But this only takes in account the first paragraph of the branch inset? If you want to take in account branches (or more generally nested textinset), you should do it completely, I can't believe it is so difficult. JMarc
[PATCH] Branches and ToC bug
When I translate the section headers using branches, nothing appears in the
ToC, just
the number. Text inside a branch inset does not make it to the ToC or nav menu.
Attached a fix. With this, the active/selected branch(es) will be used to
create the
ToC entry.
- Martin
Index: TocBackend.C
===
--- TocBackend.C(revision 16710)
+++ TocBackend.C(working copy)
@@ -23,6 +23,7 @@
#include "debug.h"
#include "insets/insetoptarg.h"
+#include "insets/insetbranch.h"
#include "support/convert.h"
@@ -169,6 +170,16 @@ void TocBackend::update()
tocstring += par.asString(*buffer_, false);
break;
}
+ case InsetBase::BRANCH_CODE: {
+ if (!tocstring.empty()
+ ||
!static_cast(inset).isBranchSelected(*buffer_))
+ break;
+ Paragraph const & par =
+
*static_cast(inset).paragraphs().begin();
+ if (!pit->getLabelstring().empty())
+ tocstring = pit->getLabelstring() + ' ';
+ tocstring += par.asString(*buffer_, false);
+ }
default:
break;
}
pgpnSo7QhccN8.pgp
Description: PGP signature
