Re: [xwiki-devs] [xwiki-notifications] r17629 - platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter
jvelociter (SVN) wrote:
Author: jvelociter Date: 2009-03-14 23:32:39 +0100 (Sat, 14 Mar 2009) New Revision: 17629
Modified: platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java Log: XWIKI-3360 Including a document with first or second heading level breaks the including document section edit links (Syntax 1.0)
Internal count of section was incremented for headings of included documents while they are not part of the document and thus not subject to section edit in an included context. In doubt I left the comment related to the increment, though I am not sure it is relevant.
Modified: platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java =================================================================== --- platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java 2009-03-14 15:43:17 UTC (rev 17628) +++ platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java 2009-03-14 22:32:39 UTC (rev 17629) @@ -158,15 +158,15 @@ }
if (level.equals("1") || level.equals("1.1")) { - // TODO: This is unstable, meaning that it works in the current skin, but it might - // fail if there are other headings processed before the document content. - sectionNumber++; // This check is needed so that only the document content generates sectionedit // links. // TODO: Find a better way to make this check, as this prevents generating links for // titles that are transformed by velocity (1.1 about $doc.fullName) or by radeox // (1.1 This is *important*). if (doc != null && doc.getContent().indexOf(title.trim()) != -1) { + // TODO: This is unstable, meaning that it works in the current skin, but it might + // fail if there are other headings processed before the document content. + sectionNumber++; StringBuffer editparams = new StringBuffer(); if (xcontext.getWiki().getEditorPreference(xcontext).equals("wysiwyg")) { editparams.append("xpage=wysiwyg§ion=").append(sectionNumber);
You just re-introduced http://jira.xwiki.org/jira/browse/XWIKI-1176 Too bad there was no test for it (my fault). The problem is that the section edit buttons should be added just for real sections in the current document, but it is hard to do that. The current code does a simple check to see if the current header (wiki syntax) is found in the document. This raises two issues: since radeox is executed after velocity, if the document source contained a heading with velocity, this check raises a false negative. Now: - if we put the increment inside the if, the sections will be wrong because of this false negative - if we leave it outside, true negatives (like imported content or headers generated inside loops) will also shift section numbers So either way, the whole code is wrong. For reference, this was the change I made: http://fisheye2.atlassian.com/changelog/xwiki?cs=4485 (for some unknown reason svn fails to show the changes, although it says that 3 lines were added and one removed). -- Sergiu Dumitriu http://purl.org/net/sergiu/
Sergiu Dumitriu wrote:
jvelociter (SVN) wrote:
Author: jvelociter Date: 2009-03-14 23:32:39 +0100 (Sat, 14 Mar 2009) New Revision: 17629
Modified: platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java Log: XWIKI-3360 Including a document with first or second heading level breaks the including document section edit links (Syntax 1.0)
Internal count of section was incremented for headings of included documents while they are not part of the document and thus not subject to section edit in an included context. In doubt I left the comment related to the increment, though I am not sure it is relevant.
Modified: platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java =================================================================== --- platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java 2009-03-14 15:43:17 UTC (rev 17628) +++ platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java 2009-03-14 22:32:39 UTC (rev 17629) @@ -158,15 +158,15 @@ }
if (level.equals("1") || level.equals("1.1")) { - // TODO: This is unstable, meaning that it works in the current skin, but it might - // fail if there are other headings processed before the document content. - sectionNumber++; // This check is needed so that only the document content generates sectionedit // links. // TODO: Find a better way to make this check, as this prevents generating links for // titles that are transformed by velocity (1.1 about $doc.fullName) or by radeox // (1.1 This is *important*). if (doc != null && doc.getContent().indexOf(title.trim()) != -1) { + // TODO: This is unstable, meaning that it works in the current skin, but it might + // fail if there are other headings processed before the document content. + sectionNumber++; StringBuffer editparams = new StringBuffer(); if (xcontext.getWiki().getEditorPreference(xcontext).equals("wysiwyg")) { editparams.append("xpage=wysiwyg§ion=").append(sectionNumber);
You just re-introduced http://jira.xwiki.org/jira/browse/XWIKI-1176 Too bad there was no test for it (my fault).
The problem is that the section edit buttons should be added just for real sections in the current document, but it is hard to do that. The current code does a simple check to see if the current header (wiki syntax) is found in the document. This raises two issues: since radeox is executed after velocity, if the document source contained a heading with velocity, this check raises a false negative. Now:
- if we put the increment inside the if, the sections will be wrong because of this false negative - if we leave it outside, true negatives (like imported content or headers generated inside loops) will also shift section numbers
So either way, the whole code is wrong.
Ok, I see. I think we should keep he increment inside the clause and either: - Improve the check as the TODO suggests (but I don't really see how now) - Decide that "1.1 About $doc.fullName" (or other title with radeox/velocity) is not a section, and change Document#getSections to reflect that (checking each title against a regular expressions for example, and discard it if it contains velocity or radeox). Having the increment outside the clause sounds wrong any case. XWIKI-3360 can be experienced on the only edit link of Main.WebHome of a fresh XE, so I prefer not to revert for now, and reopen XWIKI-1176 if needed. Jerome.
For reference, this was the change I made: http://fisheye2.atlassian.com/changelog/xwiki?cs=4485 (for some unknown reason svn fails to show the changes, although it says that 3 lines were added and one removed).
Jerome Velociter wrote:
Sergiu Dumitriu wrote:
jvelociter (SVN) wrote:
Author: jvelociter Date: 2009-03-14 23:32:39 +0100 (Sat, 14 Mar 2009) New Revision: 17629
Modified: platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java Log: XWIKI-3360 Including a document with first or second heading level breaks the including document section edit links (Syntax 1.0)
Internal count of section was incremented for headings of included documents while they are not part of the document and thus not subject to section edit in an included context. In doubt I left the comment related to the increment, though I am not sure it is relevant.
Modified: platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java =================================================================== --- platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java 2009-03-14 15:43:17 UTC (rev 17628) +++ platform/core/branches/xwiki-core-1.8/xwiki-core/src/main/java/com/xpn/xwiki/render/filter/XWikiHeadingFilter.java 2009-03-14 22:32:39 UTC (rev 17629) @@ -158,15 +158,15 @@ }
if (level.equals("1") || level.equals("1.1")) { - // TODO: This is unstable, meaning that it works in the current skin, but it might - // fail if there are other headings processed before the document content. - sectionNumber++; // This check is needed so that only the document content generates sectionedit // links. // TODO: Find a better way to make this check, as this prevents generating links for // titles that are transformed by velocity (1.1 about $doc.fullName) or by radeox // (1.1 This is *important*). if (doc != null && doc.getContent().indexOf(title.trim()) != -1) { + // TODO: This is unstable, meaning that it works in the current skin, but it might + // fail if there are other headings processed before the document content. + sectionNumber++; StringBuffer editparams = new StringBuffer(); if (xcontext.getWiki().getEditorPreference(xcontext).equals("wysiwyg")) { editparams.append("xpage=wysiwyg§ion=").append(sectionNumber); You just re-introduced http://jira.xwiki.org/jira/browse/XWIKI-1176 Too bad there was no test for it (my fault).
The problem is that the section edit buttons should be added just for real sections in the current document, but it is hard to do that. The current code does a simple check to see if the current header (wiki syntax) is found in the document. This raises two issues: since radeox is executed after velocity, if the document source contained a heading with velocity, this check raises a false negative. Now:
- if we put the increment inside the if, the sections will be wrong because of this false negative - if we leave it outside, true negatives (like imported content or headers generated inside loops) will also shift section numbers
So either way, the whole code is wrong.
Ok, I see. I think we should keep he increment inside the clause and either:
- Improve the check as the TODO suggests (but I don't really see how now) - Decide that "1.1 About $doc.fullName" (or other title with radeox/velocity) is not a section, and change Document#getSections to reflect that (checking each title against a regular expressions for example, and discard it if it contains velocity or radeox).
OK for 2. There is little chance of finding a better fix, since regular expressions are a very bad foundation.
Having the increment outside the clause sounds wrong any case.
XWIKI-3360 can be experienced on the only edit link of Main.WebHome of a fresh XE, so I prefer not to revert for now, and reopen XWIKI-1176 if needed.
Jerome.
For reference, this was the change I made: http://fisheye2.atlassian.com/changelog/xwiki?cs=4485 (for some unknown reason svn fails to show the changes, although it says that 3 lines were added and one removed).
-- Sergiu Dumitriu http://purl.org/net/sergiu/
participants (2)
-
Jerome Velociter -
Sergiu Dumitriu