New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supports <ol start=N> and <li value=N> attributes #135

Merged
merged 4 commits into from Mar 21, 2018

Conversation

Projects
None yet
2 participants
@poire-z
Contributor

poire-z commented Mar 21, 2018

See koreader/koreader#3782
This adds support for <ol start=N> and <li value=N> attributes, and allows zero and negative values for decimal numbering.
This HTML (texts truncated) snippet:

<ol class="list_">
        <li value="2" class="block_18">(arts. 10 e 927, §1º) Para a formação do precedente, somente podem ser usados argumentos submetidos ao contraditório. <i class="calibre5">(Grupo: Prec
</ol>
        <p class="block_17">Â| </p>
<ol class="list_">
        <li value="3" class="block_18"></li>
        <li value="3b" class="block_18"></li>
        <li value="0" class="block_18"></li>
        <li class="block_18"></li>
        <li value="-27" class="block_18"></li>
        <li class="block_18"></li>
        <li value="9" class="block_18"></li>
        <li class="block_18"></li>
</ol>
<p class="block_19">Â| </p>
<ol class="list_" start="0">
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
</ol>
<ol class="list_" start="-2">
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
</ol>
<ol class="list_" start="42">
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
</ol>
<ol style="list-style-type: lower-alpha" start="5">
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
        <li class="block_18">(art. 69, § 1º) A carta arbitral</li>
</ol>

gives this:
li_numbering

Added support for zero, as otherwise, this list item had no left padding, and would stick out of the others.

It's a bit unclear whether this part of the code is called for each list item (it seems so, given the above results), as in

if ( rm==erm_list_item ) {
// put item number/marker to list
lString16 marker;
int marker_width = 0;
ListNumberingPropsRef listProps = enode->getDocument()->getNodeNumberingProps( enode->getParentNode()->getDataIndex() );
if ( listProps.isNull() ) {
int counterValue = 0;
ldomNode * parent = enode->getParentNode();
int maxWidth = 0;
for ( int i=0; i<parent->getChildCount(); i++ ) {
lString16 marker;
int markerWidth = 0;
ldomNode * child = parent->getChildElementNode(i);
if ( child && child->getNodeListMarker( counterValue, marker, markerWidth ) ) {
if ( markerWidth>maxWidth )
maxWidth = markerWidth;
}
}
listProps = ListNumberingPropsRef( new ListNumberingProps(counterValue, maxWidth) );
enode->getDocument()->setNodeNumberingProps( enode->getParentNode()->getDataIndex(), listProps );
}
int counterValue = 0;
if ( enode->getNodeListMarker( counterValue, marker, marker_width ) ) {
if ( !listProps.isNull() )
marker_width = listProps->maxWidth;
css_list_style_position_t sp = style->list_style_position;
LVFont * font = enode->getFont().get();
lUInt32 cl = style->color.type!=css_val_color ? 0xFFFFFFFF : style->color.value;
lUInt32 bgcl = style->background_color.type!=css_val_color ? 0xFFFFFFFF : style->background_color.value;
int margin = 0;
if ( sp==css_lsp_outside )
margin = -marker_width;
marker += "\t";
txform->AddSourceLine( marker.c_str(), marker.length(), cl, bgcl, font, flags|LTEXT_FLAG_OWNTEXT, line_h,
margin, NULL );
flags &= ~LTEXT_FLAG_NEWLINE;
}

it looks like it reuses the previous counterValue and give it to the function we fixed, which should bypass what we added if non-zero (= already defined).

Supports <ol start=N> and <li value=N> attributes
And allows zero and negative value for decimal numbering.
@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 21, 2018

@poire-z That was fast. Not difficult of course and I was considering adding a quick child->getAttributeValue(attr_value) myself this afternoon. ;-)

@@ -11032,6 +11032,13 @@ bool ldomNode::getNodeListMarker( int & counterValue, lString16 & marker, int &
// calculate counter
ldomNode * parent = getParentNode();
counterValue = 0;
// See if parent has a 'start' attribute that overrides this 0
lString16 value = parent->getAttributeValue(attr_start);

This comment has been minimized.

@Frenzie

Frenzie Mar 21, 2018

Member

Maybe also quote + reference to the spec for convenience?

// https://www.w3.org/TR/html5/grouping-content.html#the-li-element
// "The value attribute, if present, must be a valid integer giving the ordinal value of the list item."

Edit: sorry, this one here is the start attribute. But you know what I mean.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 21, 2018

It's a bit unclear whether this part of the code is called for each list item (it seems so, given the above results), as in

Yeah, it looks a bit wasteful. Probably generally not too problematic but I wonder how that goes on a list of a few thousand items.

@@ -11058,6 +11060,8 @@ bool ldomNode::getNodeListMarker( int & counterValue, lString16 & marker, int &
}
// See if it has a 'value' attribute that overrides
// the incremented value
// https://www.w3.org/TR/html5/grouping-content.html#the-li-element
// "The value attribute, if present, must be a valid integer giving the ordinal value of the list item.

This comment has been minimized.

@Frenzie

Frenzie Mar 21, 2018

Member

Skipped the end quote. ;-)

@@ -190,6 +190,7 @@ XS_ATTR( StyleSheet )
XS_ATTR( title )
XS_ATTR( subtitle )
XS_ATTR( suptitle )
XS_ATTR( start )

This comment has been minimized.

@Frenzie

Frenzie Mar 21, 2018

Member

Hm, is there any order to this mess?

This comment has been minimized.

@poire-z

poire-z Mar 21, 2018

Contributor

Dunno, but we don't want to increase the delta with upstream, do we? :)
As these are turned out into an enum, and so incremented values are associated to these attributes, and these numbers are stored (instead of the string value of the attribute name) in the node>attr>attrvalue mapping (stored in cache file), I figured adding at the end was the right thing to do, so we don't shuffle these numbers.
I dunno if I need to increment the cachefile version number in spite of this, so past caches are invalidated and not re-used.

This comment has been minimized.

@Frenzie

Frenzie Mar 21, 2018

Member

Oh, I didn't realize it was an enum. :-)

This comment has been minimized.

@Frenzie

Frenzie Mar 21, 2018

Member

Dunno, but we don't want to increase the delta with upstream, do we? :)

Well… not until after we've sucked it dry anyway. I became somewhat bored looking into that. I would like to modernize some aspects of the code base for sure after finishing that. (clang-tidy can conveniently do most of it for you.)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 21, 2018

Bumping cache file format version, even if I don't know if it's really needed (but it's possible these attributes have these enum numbers, and other attributes not listed here get the numbers after - and one of these could be interpreted as "start=" with this)

@Frenzie Frenzie merged commit bd270d0 into koreader:master Mar 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@poire-z poire-z deleted the poire-z:ol_li_numbering_attr branch Mar 21, 2018

Frenzie added a commit to Frenzie/koreader-base that referenced this pull request Mar 21, 2018

Frenzie added a commit to koreader/koreader-base that referenced this pull request Mar 21, 2018

Frenzie added a commit to Frenzie/koreader that referenced this pull request Mar 21, 2018

Frenzie added a commit to koreader/koreader that referenced this pull request Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment