Skip to content

Language and dir rewrite #275

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

Merged
merged 9 commits into from
Jul 25, 2018
Merged

Language and dir rewrite #275

merged 9 commits into from
Jul 25, 2018

Conversation

iherman
Copy link
Member

@iherman iherman commented Jul 24, 2018

This is the version is agreed on the Telco 2018-08-23.

Fix #220


Preview | Diff

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@TzviyaSiegman could you also have a look at appendix C, please, to see if it is o.k.?

@llemeurfr
Copy link
Contributor

In 4.4.5 I would rather define the language as the "natural language of the publication", and in the infoset requirement add that it also specifies the natural language of the textual values; not the reverse.

I would also explain in 4.4.5 that the base direction is the "base direction of the publication", and move the first note of 4.4.5.1 there (it's functional).

In 4.4.5.1 I would remove "(i.e., if the base direction is set) ".

In the definition of inLanguage and inDirection I would put the emphasis on its use at the level of the publication ("Default language for the Web Publication as well as for textual values").

In the note 4.4.5.2.1 the is a typo: it is RECOMMENDED to use set these properties
and IMO an error: the containing link element in case of embedding

Note that I'm still not convinced at all about the practical use of the base direction (for UI needs + as indication for text directionality). And as I said yesterday I don't support the difference of processing between embedded and detached manifests. But I'll move these rants elsewhere.

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@llemeurfr thanks. I have made those changes; can you check and, if you are o.k. with merging, give your thumbs up?

(I have made the issue on inheritance explicit in the text)

@llemeurfr
Copy link
Contributor

In the note in 4.4.5.1, I would argue that there should be no notion of override in "it is possible to set, e.g., the language for that resource individually, which will override the global setting for the publication as a whole." and make it required or recommended. -> ""it is recommended to set the language and base direction for each resource individually."

Apart from that, thumbs up for me.

@mattgarrish
Copy link
Member

I think we could just make the note about placement into a parenthetical. There are also some normative keywords in lower case. Do you want me to make some small adjustments to fix these?

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@mattgarrish please do; I have some other stuff to worry about.

@llemeurfr is fine with the PR and unless @TzviyaSiegman has objections, I guess you should merge this (and the other two PR-s actually)

@TzviyaSiegman
Copy link
Contributor

@iherman in App C, the third example "displayed as" - Hebrew is correct, but the english is displaying as RTL.

Copy link
Contributor

@TzviyaSiegman TzviyaSiegman left a comment

Choose a reason for hiding this comment

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

minor rendering issue in Appendix C (third example

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@TzviyaSiegman do you think it is possible to merge the whole thing and you can then edit the example? It is less error prone than me doing it...

@TzviyaSiegman
Copy link
Contributor

@iherman I can try!

@r12a
Copy link

r12a commented Jul 24, 2018

Do you want me to look at the Hebrew example ?

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@r12a yes please, if you can also have a look!

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@TzviyaSiegman it seems that @mattgarrish wants to do a small editorial change; I would leave to him to merge this PR

@r12a
Copy link

r12a commented Jul 24, 2018

is it the lines from 3100 onwards?

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@r12a Appendix C

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@r12a
Copy link

r12a commented Jul 24, 2018

@iherman what should the column "Displayed as" show? Is it what you would expect to see if some link in the chain correctly interpreted the presence of the RLM/LRM as meaning "put something around this string in the target location to make it have the correct base direction"?

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

@r12a Well... I originally expected the "value" to show the string the way it is stored in a program. But then I realized that even a usual screen editor (I use SublimeText) probably shows the hebrew characters in the display mode when in HTML and not as it is stored inside.... I am not sure how to achieve that. The right hand column is the way one would expect to see it.

@r12a
Copy link

r12a commented Jul 24, 2018

Welcome to the crazy world of bidi examples ;-). It's quite hard to make it do what you want. I'm in the process of rewriting that whole table for you. First column will show the characters in memory in order of storage. Last column will show expected outcome, if there's an interpreter in place to build the context around the string in the target location that sets the right base direction.

@iherman
Copy link
Member Author

iherman commented Jul 24, 2018

Wow. @r12a, I will owe you a beer or equivalent in Tokyo!

@r12a
Copy link

r12a commented Jul 24, 2018

Bidi code sent by email, btw.

@mattgarrish
Copy link
Member

I've done some minor cleanup on this: removing normative keywords where we aren't really say anything normative, and also from notes, which by definition are informative.

One thing I found concerning (and I see this elsewhere in the spec), is that there are times where we indicate to see another specification, without being specific what we're referring people to (e.g., "(see [html])"). It would be good to root these out and be more specific about what section we're referring to so that readers don't have to hunt down the information for themselves.

@iherman
Copy link
Member Author

iherman commented Jul 25, 2018

@mattgarrish, on #275 (comment): actually, the links to the handling of lang and dir were linked from the document itself, but it was not really clear. I did make some minor editorial change to make it clearer. That being said, a review of the document as whole for this would be indeed useful.

@iherman iherman merged commit f33f89c into master Jul 25, 2018
@iherman iherman deleted the language-dir-rewrite branch July 25, 2018 11:10
@mattgarrish
Copy link
Member

Thanks, that was my issue: does the reference simply refer to previous links or does it mean there is something else the user has to look up. Clearer now.

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.

5 participants