OpenLP

Merge lp://qastaging/~thelinuxguy/openlp/fix-newline-bug into lp://qastaging/openlp

  1. fix-newline-bug
  2. Merge into trunk
Proposed by Simon Hanna
Status: Merged
Merged at revision: 2817
Proposed branch: lp://qastaging/~thelinuxguy/openlp/fix-newline-bug
Merge into: lp://qastaging/openlp
Diff against target: 80 lines (+33/-9)
2 files modified
openlp/core/common/__init__.py (+7/-7)
tests/functional/openlp_core/common/test_common.py (+26/-2)
To merge this branch: bzr merge lp://qastaging/~thelinuxguy/openlp/fix-newline-bug
Reviewer Review Type Date Requested Status
Phill Approve
Tim Bentley Approve
Review via email: mp+343465@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2018-04-17.

Description of the change

Fix the fix for 1727517

To post a comment you must log in.
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

One minor fix as I do not like the rename of the field

review: Needs Fixing
Revision history for this message
Phill (phill-ridout) wrote : Posted in a previous version of this proposal

See in line. One question and a few (more) minor fixes please.

review: Needs Fixing
Revision history for this message
Simon Hanna (thelinuxguy) wrote : Posted in a previous version of this proposal

will update shortly

Revision history for this message
Phill (phill-ridout) wrote : Posted in a previous version of this proposal

I've done some research in to this. Officially the only code points allowed are:

"
[2] Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF] /* any Unicode character, excluding the surrogate blocks, FFFE, and FFFF. */
" ( https://www.w3.org/TR/REC-xml/#charsets)

So going on this our regex should be:

re.compile(r'[\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]')

we probably should filter out the other code points too.

Its worth noting that REPLACMENT_CHARS_MAP replaces the vertical tab and form feed chars with "\n\n" before the CONTROL_CHARS regex filters them out!

So in summary please replace the regex with the one in the inline comment. (Note: its not tested)

review: Needs Fixing
Revision history for this message
Simon Hanna (thelinuxguy) wrote : Posted in a previous version of this proposal

I wonder what the reason behind replacing vertial tabs with new lines is...

Looking at where the regex is being used I'm inclined to say the whole thing needs to be reworked.

It's also used in the filename cleaning where linefeeds are strange, but then again I don't know what the input always is.

I'm changing it to what you request, but you should really take a serious look at it. From what I see, you are the last one that touched the invalid file regex, I highly doubt this is what you actually want to happen. If it is, it is a mystery to me...

Revision history for this message
Phill (phill-ridout) wrote : Posted in a previous version of this proposal

One small typo, and a couple comments that need changing.

review: Needs Fixing
Revision history for this message
Phill (phill-ridout) wrote : Posted in a previous version of this proposal

> One small typo, and a couple comments that need changing.

Sorry!

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

See comments inline

lp:~thelinuxguy/openlp/fix-newline-bug (revision 2822)
https://ci.openlp.io/job/Branch-01-Pull/2502/ [SUCCESS]
https://ci.openlp.io/job/Branch-02a-Linux-Tests/2403/ [SUCCESS]
https://ci.openlp.io/job/Branch-02b-macOS-Tests/189/ [FAILURE]
https://ci.openlp.io/job/Branch-03a-Build-Source/101/ [SUCCESS]
https://ci.openlp.io/job/Branch-03b-Build-macOS/94/ [SUCCESS]
https://ci.openlp.io/job/Branch-04a-Code-Analysis/1563/ [SUCCESS]
https://ci.openlp.io/job/Branch-04b-Test-Coverage/1376/ [SUCCESS]

review: Needs Fixing
Revision history for this message
Tim Bentley (trb143) wrote :

Looks good to me.

review: Approve
Revision history for this message
Phill (phill-ridout) wrote :

Good, thanks for you patience

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.