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

Various list numbering improvements #565

Merged
merged 6 commits into from Jul 11, 2014

Conversation

Projects
None yet
6 participants
@satsuper
Contributor

satsuper commented Apr 30, 2014

This PR adds support for displaying multi level list numbering. The approach used was to use a different counter for each level instead of using nested CSS counters. This was because you can limit the levels of numbering displayed to a subset of the current level e.g. displaying two levels

  1. Level 1
        1.1. Level 2
            1.1. Level 3
                1.1. Level 4

The nested counter approach was not compatible with the above option of list numbering.

Also added was continuing the numbering of previous lists in the document. This approach was intially modeled off existing code in OdfCanvas that partially implemented this feature but due to a regression this code was not being reached anymore.

The new approach assigns unique counters to all text:list elements regardless of whether they need to be continued or not. This allows us to circumvent CSS scoping rules that prevent using the same counter across different lists which was how the old approach worked. If a list continues another list in the document it can simply be assigned the unique counters from the list it continues from. This is a cleaner approach overall as it reduces the amount of overridden CSS rules.

Lists can either continue the list directly preceding them in document or they can continue a list anywhere in the document by referencing its xml:id. Both these cases are handled in this PR.

To better illustrate the three main improvements made in this PR I have included a comparison screenshot of WebODF rendering the updated visual-tests.fodt.

comparison

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci Apr 30, 2014

Build failed.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1547/

kogmbh-ci commented Apr 30, 2014

Build failed.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1547/

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci May 2, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1552/

kogmbh-ci commented May 2, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1552/

* the ODF list content if they affect the final style
* @param {!CSSStyleSheet} styleSheet
* @param {!Element} odfBody
* @param {!Object.<!string, !odf.StyleTreeNode>} listStyles

This comment has been minimized.

@peitschie

peitschie May 5, 2014

Contributor

missing @return {undefined}

@peitschie

peitschie May 5, 2014

Contributor

missing @return {undefined}

This comment has been minimized.

@satsuper

satsuper May 6, 2014

Contributor

Done

@satsuper

satsuper May 6, 2014

Contributor

Done

@peitschie

This comment has been minimized.

Show comment
Hide comment
@peitschie

peitschie May 5, 2014

Contributor

Due to the difficulties with getting the actual rendered text from a pseudo element, I'd suggest additional tests be added to webodf/tests/visual-tests.fodt for these new list behaviours. This allows a document that can be loaded and manually verified by a human to ensure WebODF behaviour remains correct.

Contributor

peitschie commented May 5, 2014

Due to the difficulties with getting the actual rendered text from a pseudo element, I'd suggest additional tests be added to webodf/tests/visual-tests.fodt for these new list behaviours. This allows a document that can be loaded and manually verified by a human to ensure WebODF behaviour remains correct.

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci May 6, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1558/

kogmbh-ci commented May 6, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1558/

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci May 6, 2014

Build failed.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1562/

kogmbh-ci commented May 6, 2014

Build failed.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1562/

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci May 7, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1577/

kogmbh-ci commented May 7, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1577/

@satsuper satsuper changed the title from List numbering to Various list numbering improvements May 7, 2014

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci May 8, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1581/

kogmbh-ci commented May 8, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1581/

@thz

This comment has been minimized.

Show comment
Hide comment
@thz

thz May 8, 2014

Member

May I propose to also update the welcome.odt file to feature the improvements with this PR?

Member

thz commented May 8, 2014

May I propose to also update the welcome.odt file to feature the improvements with this PR?

@peitschie

This comment has been minimized.

Show comment
Hide comment
@peitschie

peitschie May 8, 2014

Contributor

@thz we would definitely be open to that. Do you have any suggestions?

Contributor

peitschie commented May 8, 2014

@thz we would definitely be open to that. Do you have any suggestions?

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci May 9, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1584/

kogmbh-ci commented May 9, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1584/

@kossebau

This comment has been minimized.

Show comment
Hide comment
@kossebau

kossebau May 13, 2014

Member

Cool work in total :) Will be happy to have that in soon.

May I ask to have visual-tests.fodt get extended with some show-off also for value of text:display-levels != value of text:level and != 1? Works nicely as I found by twisting the L2 style, but would be nice to have that in as a separate sample.

And for text:continue-numbering="true" atext:level` > 1 variant would be nice as well.

The "Should be" idea as used in the "Multiple display levels list" sample is nice, perhaps that could be the pattern everywhere? Current comment could be noted in brackets behind that still, e.g.
"Should be ' (single quote)"

The "Custom numbering" is not yet done, or? At least it looks different in LO. So TODO items could be noted as such, so one can see what is a regression and what a TODO.

Member

kossebau commented May 13, 2014

Cool work in total :) Will be happy to have that in soon.

May I ask to have visual-tests.fodt get extended with some show-off also for value of text:display-levels != value of text:level and != 1? Works nicely as I found by twisting the L2 style, but would be nice to have that in as a separate sample.

And for text:continue-numbering="true" atext:level` > 1 variant would be nice as well.

The "Should be" idea as used in the "Multiple display levels list" sample is nice, perhaps that could be the pattern everywhere? Current comment could be noted in brackets behind that still, e.g.
"Should be ' (single quote)"

The "Custom numbering" is not yet done, or? At least it looks different in LO. So TODO items could be noted as such, so one can see what is a regression and what a TODO.

@kossebau

This comment has been minimized.

Show comment
Hide comment
@kossebau

kossebau May 13, 2014

Member

The "Should be" idea as used in the "Multiple display levels list" sample is nice, perhaps that could be the pattern everywhere? Current comment could be noted in brackets behind that still, e.g.
"Should be ' (single quote)"

Given that "Should be" is used in the tests when noting checks that failed, perhaps for known to work cases it could be written as "Is", and only for those were it is known to not work it would be "Should be"? Just a thought.

Member

kossebau commented May 13, 2014

The "Should be" idea as used in the "Multiple display levels list" sample is nice, perhaps that could be the pattern everywhere? Current comment could be noted in brackets behind that still, e.g.
"Should be ' (single quote)"

Given that "Should be" is used in the tests when noting checks that failed, perhaps for known to work cases it could be written as "Is", and only for those were it is known to not work it would be "Should be"? Just a thought.

@satsuper

This comment has been minimized.

Show comment
Hide comment
@satsuper

satsuper May 14, 2014

Contributor

What are you referring to regarding the custom numbering? A custom list numbering style should work as far as I know. Could you give me an example of what looks different in LibreOffice?

I think your suggestion for the visual tests is good and will update the tests I have added to the file

Contributor

satsuper commented May 14, 2014

What are you referring to regarding the custom numbering? A custom list numbering style should work as far as I know. Could you give me an example of what looks different in LibreOffice?

I think your suggestion for the visual tests is good and will update the tests I have added to the file

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci Jun 16, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1838/

kogmbh-ci commented Jun 16, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1838/

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci Jun 16, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1839/

kogmbh-ci commented Jun 16, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1839/

@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci Jul 8, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1916/

kogmbh-ci commented Jul 8, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1916/

@satsuper

This comment has been minimized.

Show comment
Hide comment
@satsuper

satsuper Jul 8, 2014

Contributor

Just pushed up a rebase so that this can be looked at again soon :bowtie:

Contributor

satsuper commented Jul 8, 2014

Just pushed up a rebase so that this can be looked at again soon :bowtie:

@kossebau

This comment has been minimized.

Show comment
Hide comment
@kossebau

kossebau Jul 10, 2014

Member

On my schedule for tomorrow/this friday. Really looking forward to get this finally in.

Member

kossebau commented Jul 10, 2014

On my schedule for tomorrow/this friday. Really looking forward to get this finally in.

@kossebau

This comment has been minimized.

Show comment
Hide comment
@kossebau

kossebau Jul 11, 2014

Member

Cool 🍦 ! Might be that my eyes got tired by looking at it a third(?) time, but could only find those minor problems :) So please work on those, then rebase, for the final testing. I put the "Ship it" stamp already on my desk, to be able to quickly apply it :)

Member

kossebau commented Jul 11, 2014

Cool 🍦 ! Might be that my eyes got tired by looking at it a third(?) time, but could only find those minor problems :) So please work on those, then rebase, for the final testing. I put the "Ship it" stamp already on my desk, to be able to quickly apply it :)

Satvik Kumar added some commits May 21, 2014

Satvik Kumar
Refactor calculation of CSS content for lists
Moved the processing of the list style to get the content rule into a
separate method so it can be re used for continue list styles. Also
ensured that the new getContentRule function returns a complete rule with
the semicolon on the end.
Satvik Kumar
Support multi level list numbering
Implemented a changed list counter setup where each list level has its own
counter. This is to support behaviour defined where you can display only a
subset of the list numbers.

This is defined here:
http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part1.html#__RefHeading__1418890_253892949
Satvik Kumar
Provide each text:list with a unique CSS counter
Giving each list a unique counter allows us to implement continued
numbering functionality while avoiding unexpected numbering due to how CSS
counter values are scoped to an elements sibilings or children.
Satvik Kumar
Remove dead code from OdfCanvas and OdfUtils
The removed code was intended to implement continue list functionality.
However this code no longer worked due to a regression introduced in commit 3ae9e0c.

As I have refactored this area and implemented the continue list
functionality in the new list styles class this old code is no longer
required.

The removed code in OdfCanvas also made use of the
getFirstNonWhitespaceChild function in OdfUtils and the removed code was
the last remaining usage of it. As it is preferable to make use of DOM API
calls which give us the desired behaviour such as
element.firstElementChild I have removed this function.
Satvik Kumar
Implement continued numbering
This adds support for the text:continue-numbering and text:continue-list
attributes. The text:continue-numbering attribute specifies whether to
continue the immediately preceding list if it has the same style.

http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part1.html#attribute-text_continue-numbering

The text:continue-list attribute specifes the unique identifier of a list
to continue numbering from in the document. These must also have the same
style.

http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part1.html#attribute-text_continue-list

https://tools.oasis-open.org/issues/browse/OFFICE-3558
Satvik Kumar
Added tests for continued and multi level lists
Added unit tests for the continue lists and added some examples of
continued and multi level lists to visual-tests.fodt
@kogmbh-ci

This comment has been minimized.

Show comment
Hide comment
@kogmbh-ci

kogmbh-ci Jul 11, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1939/

kogmbh-ci commented Jul 11, 2014

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1939/

@kossebau

This comment has been minimized.

Show comment
Hide comment
@kossebau

kossebau Jul 11, 2014

Member

Really good work! Let's have it in master. Please merge!

Member

kossebau commented Jul 11, 2014

Really good work! Let's have it in master. Please merge!

satsuper added a commit that referenced this pull request Jul 11, 2014

Merge pull request #565 from satsuper/list-numbering
Various list numbering improvements

@satsuper satsuper merged commit 85fff61 into kogmbh:master Jul 11, 2014

1 check passed

default Merged build finished.
Details

@satsuper satsuper deleted the satsuper:list-numbering branch Aug 8, 2014

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