Re: [patch] Fix bug 8874 Chunk insets behave differently form ERT insets when applying formatting
Jean-Marc Lasgouttes wrote: > Thanks for looking at this. Thanks for the comments, I took all of them into account abnd submitted the patch. Georg
Re: [patch] Fix bug 8874 Chunk insets behave differently form ERT insets when applying formatting
09/02/2014 15:30, Georg Baum: You mean resetFontEdit() should return return true if isPassThru() returns true? This would certainly make sense, but I am not sure if we should do it now. My proposed patch is conservative in the sense that the behaviour is not changed for any inset except the chunk inset. OK, apply your path as it is now, it is the safest bet at this stage. I have added your comment to the bug report for posterity's sake. Just code comments: can resetFontEdit be rewritten as follows? It seems clearer to me. +bool Inset::resetFontEdit() const +{ + return getLayout().resetsFont() || !inheritFont(); +} Also the comment over resetsFontEdit may need a clean up, e.g. the following snippet: For copy/paste the operations the language is never changed, since Thanks for looking at this. JMarc
Re: [patch] Fix bug 8874 Chunk insets behave differently form ERT insets when applying formatting
Jean-Marc Lasgouttes wrote: > I have a couple questions still. > > * the description I see of ResetsFont in the Customization manual > strikes me as being like the opposite of Inset::inheritFont. How could > an inset have non-compatible values for these two values? Would it make > any sense? Or is one of the value for screen display and the other for > LaTeX output? In this case do we need a InheritFont InsetLayout parameter? The method name is more clear than the layout tag¹: Inset::resetFontEdit() defines the behaviour of the inset for editing operations like changing the language or font size. Inset::inheritFont() describes how the inset is treated by LaTeX, and therefore whether LyX needs to export outside font changes in the inset again or not. You are right, in most cases these two methods are just the negation of each other. The only exceptions are currently Flex insets (except the URL inset): Both inheritFont() and resetFontEdit() return true. After revisiting my changes at http://www.lyx.org/trac/changeset/f176a184/lyxgit I currently do not see anymore why the separation was needed: The same effect could have been achieved by making inheritFont() customizable in the layout files. I am also not sure whether the conflicting values of resetFontEdit() and inheritFont() for the flex insets do make sense. I suggest to revisit these after applying the patch, since after the layout2layout conversion it will be explicit which insets need to be looked at. For the future we could also look at merging resetFontEdit() and inheritFont() again, unless somebody finds a reason not to do so (or I remember again the real cause for the separation). For now I think this would be too dangerous. > * concerning resetFontEdit, one might think that using isPassThru would > make sense somewhere. You mean resetFontEdit() should return return true if isPassThru() returns true? This would certainly make sense, but I am not sure if we should do it now. My proposed patch is conservative in the sense that the behaviour is not changed for any inset except the chunk inset. Georg ¹: I now believe that I was a bit on the wrong track when I used the ResetsFont layout tag (which Richard initially introduced in order to fix bug 5657) for the editing purposes, however, it has been like that for several years now.
Re: [patch] Fix bug 8874 Chunk insets behave differently form ERT insets when applying formatting
06/02/2014 22:14, Georg Baum: This is one of three bugs scheduled for 2.1.0: If you apply font changes, e.g. small size to a paragraph including a chunk inset, the result will not be compilable anymore (only after saving and reloading the document). Jean-Marc did the detective work and found out the reason: resetFontEdit() returns the wrong value for chunk insets. The attached patch fixes that by setting the correct value in the layout file, and making the defaults of resetFontEdit() and inheritFont() compatible. See http://www.lyx.org/trac/ticket/8874 if you want to understand all details. I tested this patch extensively, and it works fine now (it is the third version), but since it involves a layout file format change I'd rather ask: Is it OK to go in? Of course I would convert all other layout files at the same time. I have a couple questions still. * the description I see of ResetsFont in the Customization manual strikes me as being like the opposite of Inset::inheritFont. How could an inset have non-compatible values for these two values? Would it make any sense? Or is one of the value for screen display and the other for LaTeX output? In this case do we need a InheritFont InsetLayout parameter? * concerning resetFontEdit, one might think that using isPassThru would make sense somewhere. JMarc
Re: [patch] Fix bug 8874 Chunk insets behave differently form ERT insets when applying formatting
Georg Baum wrote: > I tested this patch extensively, and it works fine now (it is the third > version), but since it involves a layout file format change I'd rather ask: > Is it OK to go in? I am all for it. Regards, Jürgen
[patch] Fix bug 8874 Chunk insets behave differently form ERT insets when applying formatting
This is one of three bugs scheduled for 2.1.0: If you apply font changes, e.g. small size to a paragraph including a chunk inset, the result will not be compilable anymore (only after saving and reloading the document). Jean-Marc did the detective work and found out the reason: resetFontEdit() returns the wrong value for chunk insets. The attached patch fixes that by setting the correct value in the layout file, and making the defaults of resetFontEdit() and inheritFont() compatible. See http://www.lyx.org/trac/ticket/8874 if you want to understand all details. I tested this patch extensively, and it works fine now (it is the third version), but since it involves a layout file format change I'd rather ask: Is it OK to go in? Of course I would convert all other layout files at the same time. Georgdiff --git a/lib/layouts/litinsets.inc b/lib/layouts/litinsets.inc index 8bcee50..05440cb 100644 --- a/lib/layouts/litinsets.inc +++ b/lib/layouts/litinsets.inc @@ -6,7 +6,7 @@ # Note that this file is included in sweave.module, # knitr.module and noweb.module. -Format 48 +Format 49 Counter chunk PrettyFormat "Chunk ##" @@ -43,4 +43,5 @@ InsetLayout "Flex:Chunk" LeftDelim << RightDelim>>= EndArgument +ResetsFont false End diff --git a/lib/scripts/layout2layout.py b/lib/scripts/layout2layout.py index 36e4130..8644017 100644 --- a/lib/scripts/layout2layout.py +++ b/lib/scripts/layout2layout.py @@ -162,6 +162,9 @@ import os, re, string, sys # Incremented to format 48, 31 May 2013 by rgh # Add InitialValue tag for counters +# Incremented to format 49, 04 Feb 2014 by gb +# Change default of "ResetsFont" tag to false + # Do not forget to document format change in Customization # Manual (section "Declaring a new text class"). @@ -169,7 +172,7 @@ import os, re, string, sys # development/tools/updatelayouts.sh script to update all # layout files to the new format. -currentFormat = 48 +currentFormat = 49 def usage(prog_name): @@ -255,6 +258,7 @@ def convert(lines): re_Builtin = re.compile(r'^(\s*)LaTeXBuiltin\s+(\w*)', re.IGNORECASE) re_True = re.compile(r'^\s*(?:true|1)\s*$', re.IGNORECASE) re_InsetLayout = re.compile(r'^\s*InsetLayout\s+(?:Custom|CharStyle|Element):(\S+)\s*$', re.IGNORECASE) +re_ResetsFont = re.compile(r'^(\s*)ResetsFont(\s+)(\S+)$', re.IGNORECASE) # with quotes re_QInsetLayout = re.compile(r'^\s*InsetLayout\s+"(?:Custom|CharStyle|Element):([^"]+)"\s*$', re.IGNORECASE) re_InsetLayout_CopyStyle = re.compile(r'^\s*CopyStyle\s+(?:Custom|CharStyle|Element):(\S+)\s*$', re.IGNORECASE) @@ -330,6 +334,11 @@ def convert(lines): opts = 0 reqs = 0 inchapter = False +isflexlayout = False # only used for 48 -> 49 +# Whether a style is inherited (works only for CopyStyle currently, +# not for true inherited styles, see bug 8920 +inherited = False# only used for 48 -> 49 +resetsfont_found = False # only used for 48 -> 49 while i < len(lines): # Skip comments and empty lines @@ -386,6 +395,42 @@ def convert(lines): i += 1 continue +if format == 48: +# The default of ResetsFont in LyX changed from true to false, +# because it is now used for all InsetLayouts, not only flex ones. +# Therefore we need to set it to true for all flex insets which do +# do not already have a ResetsFont. +match = re_InsetLayout2.match(lines[i]) +if match: +resetsfont_found = False +inherited = False +name = string.lower(match.group(1)) +if name == "flex" or name[:5] == "flex:": +isflexlayout = True +else: +isflexlayout = False +match = re_ResetsFont.match(lines[i]) +if match: +resetsfont_found = True +match = re_End.match(lines[i]) +if match: +if isflexlayout and not resetsfont_found and not inherited: +lines.insert(i, "\tResetsFont true") +i += 1 +match = re_Style.match(lines[i]) +if match: +isflexlayout = False +inherited = False +match = re_Counter.match(lines[i]) +if match: +isflexlayout = False +inherited = False +match = re_CopyStyle.match(lines[i]) +if match: +inherited = True +i += 1 +continue + if format >= 44 and format <= 47: # nothing to do. i += 1 diff --git a/src/TextClass.cpp b/src/TextClass.cpp index b77f31b..9d4509c 100644 --- a/src/TextClass.cpp +++ b/src/TextClass.cpp @@ -61,7 +61,7 @@ namespace lyx { // development/tools/updatelayouts.sh script, to update the format of // all of our layou