Code

Opened 7 years ago

Closed 7 years ago

#4573 closed (wontfix)

Minor rewrite of linebreaks and linebreaksbr

Reported by: Johan Bergström <bugs@…> Owned by: nobody
Component: Template system Version: master
Severity: Keywords: linebreaks html
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This relatively small patch modifieslinebreaks and linebreaksbr. They now share the same backend (django.utils.html.linebreaks) which makes linebreaksbr accept additional types of newlines (\r\n etc). This patch also adds support for html (as addition to xhtml) output which are passed in as an argument to the filter:

    <h1>linebreaks/linebreaksbr both accepts "html" or 0 (False) as argument</h1>
    {{ blog.entry|linebreaks:"html" }}

Both filters and util defaults to xhtml output (the current behaviour).

Attachments (6)

django-linebreaks.patch (3.4 KB) - added by Johan Bergström <bugs@…> 7 years ago.
Includes patch, docs and tests.
django-linebreaks.diff (3.4 KB) - added by Lfe 7 years ago.
Last patch somehow didn't apply cleanly. Here's the new version (with a proper mime-type)
django-linebreaks-v2.diff (3.4 KB) - added by Johan Bergström <bugs@…> 7 years ago.
...and here's the slightly updated version. Should save a cpu cycle and fix a nasty bug (is vs ==).
django-linebreaks-v3.diff (3.4 KB) - added by Johan Bergström <bugs@…> 7 years ago.
Fixed a typo..
django-linebreaks-html-v4.diff (5.6 KB) - added by Johan Bergström <bugs@…> 7 years ago.
Fourth revision of the patch
django-linebreaks-html-v5.diff (5.4 KB) - added by Johan Bergström <bugs@…> 7 years ago.
Thank you both for feedback. Here's the same patch with the <br/> behaviour reverted.

Download all attachments as: .zip

Change History (13)

Changed 7 years ago by Johan Bergström <bugs@…>

Includes patch, docs and tests.

Changed 7 years ago by Lfe

Last patch somehow didn't apply cleanly. Here's the new version (with a proper mime-type)

comment:1 Changed 7 years ago by Johan Bergström <bugs@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Safari somehow got my IRC nickname as name when posting the updated patch, sorry for any confusion this may have caused...

Changed 7 years ago by Johan Bergström <bugs@…>

...and here's the slightly updated version. Should save a cpu cycle and fix a nasty bug (is vs ==).

Changed 7 years ago by Johan Bergström <bugs@…>

Fixed a typo..

comment:2 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

How about changing

xhtml = True 
if arg == "html": 
    xhtml = False 

to

xhtml = (arg != 'html')

comment:3 Changed 7 years ago by Johan Bergström <bugs@…>

SmileyChris: My major itch was HTML (as an option to XHTML) support - which should be outlined in my patch. If the committer feels that my code could be rewritter further - you/he/she has my blessing :-)

Changed 7 years ago by Johan Bergström <bugs@…>

Fourth revision of the patch

comment:4 Changed 7 years ago by Johan Bergström <bugs@…>

Ok. Here's another stab at this. I've implemented the improvement SmileyChris suggested. This should also apply cleanly after the unicode merge. I modified the test case for linebreaks so it accepts arguments and added extra test cases for linebreaksbr. One thing this patch also does is change the newline output from <br /> to <br/>, since using truncatewords will strip the line break containing a space (bug of feature?). I guess most people write line breaks with spaces, but this patch is slimmer than modifying truncatewords.

comment:5 Changed 7 years ago by mtredinnick

I haven't read the patch yet, so I have no comment on the specifics, however one thing in your comment raises alarms.

The space before the slash in <br /> is required for HTML parsing. The trailing slash is actually invalid in HTML (as opposed to XHTML) and so it is exploiting the required error recovery behaviour in order to work. You can't just remove it or there will be subtle errors in some renderers when used in SGML-based HTML output.

comment:6 Changed 7 years ago by SmileyChris

You shouldn't be using truncatewords after that filter - it's not HTML aware (that's why I wrote the truncatewords_html filter) so it's not something you need to worry about.

Changed 7 years ago by Johan Bergström <bugs@…>

Thank you both for feedback. Here's the same patch with the <br/> behaviour reverted.

comment:7 Changed 7 years ago by jacob

  • Resolution set to wontfix
  • Status changed from new to closed

While there's nothing technically wrong with this patch, I just don't see a reason to include it since it's just code churn.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.