Skip to content
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

#2: Changes in OBFL for the continuation of a volume #4

Closed
wants to merge 1 commit into from
Closed

#2: Changes in OBFL for the continuation of a volume #4

wants to merge 1 commit into from

Conversation

PaulRambags
Copy link

Closes #2.

@PaulRambags
Copy link
Author

Hi @kalaspuffar and @bertfrees, can you please review this pull request?

@PaulRambags
Copy link
Author

@kalaspuffar Can you please provide a location where the generated HTML can be stored?

The generated HTML file is called obfl-specification.html and it is generated with "gradlew buildSite". This will resolve all processing instructions, such as, <?toc?> and <?changes?>.
The obfl-specification.html has also a reference to this location:

  <dt>This version:</dt>
    <dd><a href="https://braillespecs.github.io/obfl/obfl-specification.html">https://braillespecs.github.io/obfl/obfl-specification.html</a></dd>

Please let me know what to put there.

<dd><dl>
<dt>use-when [optional]</dt>
<dd>A condition.</dd>
<dd>The only currently supported condition is: $resumed.</dd>
Copy link

@bertfrees bertfrees Nov 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to do this in the implementation but don't put it in the specification. I would turn it around. Mention that $resumed is a supported variable (and explain what it is!), but don't say that it is the only supported condition. For example we also discussed the possibility to say (not $resumed).

@@ -1565,6 +1566,8 @@ <h4 id="L1105" class="include">toc-entry</h4>

<p>The toc-entry element defines an entry in a table of contents.</p>

<ins datetime="2019114T14:00:00">The contents of a toc-entry should be put in a child toc-entry-content element instead of in the toc-entry element itself.</ins>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say "instead of in the toc-entry element itself". It's not because this used to be supported in older versions that we need to mention it in the latest version. It's clear from the sections below that only toc-entry-content are allowed inside toc-entry.

Comment on lines +1582 to +1583
<dd>toc-entry-content [1 or more], toc-entry [0 or more]</dd>
<dd>There must be exactly one toc-entry-content element without a use-when attribute.</dd>
Copy link

@bertfrees bertfrees Nov 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limitation "There must be exactly one toc-entry-content element without a use-when attribute" is not really needed I think. You should be able to say e.g. use-when="true", or even if none of the toc-entry-content match, Dotify should be able to cope with that by just not inserting an entry.

In fact this limitation could even be problematic because you wouldn't be able to have a single toc-entry-content with use-when="(not $resumed)", which might be needed to get the current behavior (depending on what the default value of use-when is).

I think you could even argue that the content model should be changed to "toc-entry-content [0 or more], toc-entry [0 or more]". The way it is now, Pipeline will need to generate empty toc-entry-content in some cases where a toc-entry is used merely as a container for other toc-entrys. This is not a problem, but it does show that a toc-entry-content is not always needed.

Comment on lines +1655 to +1657
<dt>apply-toc-entry-content-use-when [optional]</dt>
<dd>Whether or not to use the toc-entry-content elements in the corresponding table-of-contents that have a use-when attribute.</dd>
<dd>One of 'false' (default) or 'true'.</dd>
Copy link

@bertfrees bertfrees Nov 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three remarks:

  • I vaguely remember us talking about using the range attribute to control whether the document range toc should include the continuation entries or not. However in principle a document range toc-sequence can reference another table-of-contents than the volume range toc-sequence. The resulting OBFL will be less beautiful, but it's not really a problem. Not sure if I realized this before when I brought up the idea.
  • What's problematic with this solution is that an option to disable all the use-when is weird. Better would be an option to disable the $resumed variable (set it to a constant false). (The name is also really wordy :-) but okay let's forget that for a moment.)
  • I still wonder whether it would make more sense to add new range values instead of a dedicated attribute, because basically it is the range that we are controlling. There would be a total of 4 range values:
    • document
    • volume
    • document-incl-resumed
    • volume-incl-resumed

Copy link
Author

@PaulRambags PaulRambags Nov 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of 4 range variables.
I would also suggest to use a toc-entry-content-resumed element instead of the use-when="$resumed" construction. I have a feeling that this use-when is not coming naturally and the only option that we will support is $resumed.
Also, note that OBFL has already the concepts any-interrupted and any-resumed. These concepts make no use of a use-when. I mean, it is any-resumed and not any use-when="$resumed".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum are we going back to that discussion? I though it was sorted.

I understand the concern that the use-when it is a bit artificial because there are no other use cases. But I have nothing against the principle. I would also be fine with a dedicated element, I don't have a strong opinion about it (although if we go for the dedicated element I would go for toc-entry-content-when-resumed or toc-entry-content-on-resumed).

The comparison with any-interrupted etc. is not totally appropriate I think. These elements are not part of a switch. Together they form a single "volume-transition", evaluated once (not twice) per volume. But I understand where you're coming from, it's indeed related in a way. It depends on how you explain how these features work exactly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that a use-when only determines when a toc-entry-content may be used, but it is impossible to add any specific functionality. This is not enough. It is too general.

For instance, if you look at Example 1 in brailleapps/dotify.formatter.impl#106 (comment), case 4, you see a scenario where the toc-text-resumed of a parent is taken. This cannot be expressed with merely a use-when construction.

@fredrikschill I like your idea of keeping the OBFL specification as general as possible. However in this case we need a more specific construction. I will adapt @bertfrees's suggestion toc-entry-content-on-resumed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, if you look at Example 1 in brailleapps/dotify.formatter.impl#106 (comment), case 4, you see a scenario where the toc-text-resumed of a parent is taken. This cannot be expressed with merely a use-when construction.

OK true, but I'm not really convinced that it should behave like in that example. We could also require that in those cases you explicitly repeat the toc-entry-content use-when="..." in the descendant toc-entrys that are supposed to inherit. Sometimes it's good to leave out special behavior, it can make the spec easier to understand.

Anyway, like I said I don't have a strong opinion about which road to take. @kalaspuffar @PaulRambags @fredrikschill I'll let you guys come to an agreement.

Copy link
Author

@PaulRambags PaulRambags Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fredrikschill

The default behaviour when use-when is omited must be true, i.e. use always. Nothing else makes sense.

In that case, when you introduce use-when as you suggested, you will have to add use-when="(! $resumed)" everywhere to get the standard behavior. With standard behavior I mean how toc-entrys behave in the current implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please show the exponential example?

Basically, you will have to add a resumed text at every toc-entry.
Here is a part of the example above, and how it would be with a use-when and a $resumed and such that use-when evaluates to true by default:

<table-of-contents>
  ...
  <toc-entry ref="2">
    <toc-entry-content use-when="(! $resumed)">Chapter 2</toc-entry-content>
    <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
    <toc-entry ref="2.1">
      <toc-entry-content use-when="(! $resumed)">Chapter 2.1</toc-entry-content>
      <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
      <toc-entry ref="2.1.1">
        <toc-entry-content use-when="(! $resumed)">Chapter 2.1.1</toc-entry-content>
        <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
      </toc-entry>
      <toc-entry ref="2.1.2">
        <toc-entry-content use-when="(! $resumed)">Chapter 2.1.2</toc-entry-content>
        <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
      </toc-entry>
    </toc-entry>
	
    <!-- here, Dotify applies a volume break in the content -->
    <!-- the second volume also has some resumed text of the previous chapter,
         in other words, its content does not start with the block with id "2.2" -->
	
    <toc-entry ref="2.2">
      <toc-entry-content use-when="(! $resumed)">Chapter 2.2</toc-entry-content>
      <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
      <toc-entry ref="2.2.1">
        <toc-entry-content use-when="(! $resumed)">Chapter 2.1.1</toc-entry-content>
        <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
      </toc-entry>
      <toc-entry ref="2.2.2">
        <toc-entry-content use-when="(! $resumed)">Chapter 2.2.2</toc-entry-content>
        <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
      </toc-entry>
    </toc-entry>
    <toc-entry ref="2.3">
      <toc-entry-content use-when="(! $resumed)">Chapter 2.3</toc-entry-content>
      <toc-entry-content use-when="$resumed">Continuation of Chapter 2</toc-entry-content-on-resumed>
    </toc-entry>
  </toc-entry>
</table-of-contents>

As you can see, the text "Continuation of Chapter 2" appears 8 times.
This small example has a TOC hierarchy with just 3 levels. If there would be more levels, the amount of times that you will have to insert this text in the TOC will increase exponentially.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PaulRambags Sure it is possible. Daniel's example is just one way to do it.

That the amount of repetition would be exponential is of course bollocks, you know it. You have to repeat a toc-entry-content at most once for every descendant toc-entry. How is that exponential? Exponential with what?

Anyway, let's end the bike-shedding now because it is getting us nowhere. I think Paul has made his point clear that the use-when approach is not all rosy. Now we need to make a decision and get on with our lives.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exponential with what?

Suppose you want only a resumed text at the top level, as in my example. So only at levels 1, 2, 3, ... but not at 1.1, 1.2, etc.
If the depth of your TOC is n (in my example, n = 3 because there is no section 1.1.1.1 only a section 1.1.1) and at every level there are (in average) m items (in my example, m = 3 because there is a 2.1, 2.2 and 2.3, then you will have to repeat the same resumed text m^(n - 1) times, where ^ stands for "to the power of". So if the TOC depth increases, the number of repetitions increases exponentially.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, interesting way of looking at it :-)
So a book with 6 heading levels has 46656 headings?

Copy link

@bertfrees bertfrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @PaulRambags I added some comments in the code. I definitely think there needs to be some more documentation about how the TOCs are generated, what the $resumed variable means, and how the right toc-entry-content elements are selected. Also you made some limitations that I would drop.


<h4 id="L1106" class="include">toc-entry-content</h4>

<p>The toc-entry-content element defines the content of a toc-entry.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 'a content' not 'the content'

@PaulRambags
Copy link
Author

I am closing this pull request. I'm going to work on a new version based on the refactored version (see #5) and with the notion of a toc-entry-on-resumed idrange="..." as described in this comment.

@PaulRambags PaulRambags closed this Dec 6, 2019
@PaulRambags PaulRambags deleted the issues/2 branch December 6, 2019 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continuation of previous volume in TOC
4 participants