Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

RTL support #64

Merged
merged 4 commits into from
Aug 16, 2023
Merged

RTL support #64

merged 4 commits into from
Aug 16, 2023

Conversation

Monirzadeh
Copy link
Contributor

@Monirzadeh Monirzadeh commented Aug 3, 2023

It adds basic RTL support for go-epub

@Monirzadeh
Copy link
Contributor Author

Monirzadeh commented Aug 3, 2023

@bmaupin as I see these errors not related to my change.
If I am not wrong, old test should be updated (files not found).
Am I right?

@bmaupin
Copy link
Owner

bmaupin commented Aug 8, 2023

@Monirzadeh correct, it looks like one of the remote resources for one of the tests is returning a 404, as per #66

Regarding this PR, I'm having trouble understanding what it's for. We already have a method to set page-progression-direction. Isn't that enough for RTL support?

Thanks!

@Monirzadeh
Copy link
Contributor Author

Monirzadeh commented Aug 8, 2023

@Monirzadeh correct, it looks like one of the remote resources for one of the tests is returning a 404, as per #66

Regarding this PR, I'm having trouble understanding what it's for. We already have a method to set page-progression-direction. Isn't that enough for RTL support?

Thanks!

Based on method you send it should work like this

book.SetPpd("rtl")

but not work(ltr rtl and not even default). Am I wrong?

@bmaupin
Copy link
Owner

bmaupin commented Aug 9, 2023

Ah, I might've misunderstood, maybe page-progression-direction is just the direction that the pages turn, and not the way the characters are ordered.

At any rate, I want to get this fixed but I'd like to better understand the problem first.

  1. When you say it doesn't "work", what does that mean?
  2. Do you have a sample RTL EPUB I could take a look at? Preferably one with dir (and PPD if possible) set. I'm curious to see for myself what the different options do.
  3. What Epub reader are you using?

Thanks!

@Monirzadeh
Copy link
Contributor Author

Monirzadeh commented Aug 9, 2023

@bmaupin it is not about pages turn.

When you say it doesn't "work", what does that mean?

right now RTL show like this

نوشته راست به چپ که به درستی نمایش داده نشده حضور زبان LTR در نوشته ترتیب قرارگیری کلمات را به هم می‌ریزد.

but it should be like this

‏‏نوشته راست به چپ که به درستی نمایش داده شده حضور زبان LTR در نوشته ترتیب قرارگیری کلمات را به هم نمی‌ریزد.

you can read more about that here or here

2.Do you have a sample RTL EPUB I could take a look at? Preferably one with dir (and PPD if possible) set. I'm curious to see for myself what the different options do.

this two epub file create with and without this PR ebook-sample.zip if you see chapter 2 you can see difference. RTL should start from Right so chapter 2 in without-rtl-support.epub has wrong direction

3.What Epub reader are you using?

i use calibre

@bmaupin
Copy link
Owner

bmaupin commented Aug 10, 2023

Ah ok, I think I understand the problem now. The characters of the words are all in correct RTL order, but without dir, the text is improperly justified and even out of order.

I was going through the EPUB 3.3 spec, and it looks like dir can also be set in the package document file. Did you try that? https://www.w3.org/TR/epub-33/#attrdef-dir

@Monirzadeh
Copy link
Contributor Author

Monirzadeh commented Aug 11, 2023

i set dir="auto" or "rtl" but it not affect direction of ebook
something like this in EPUB/package.opf do you have anythings in mind?

<package xmlns="http://www.idpf.org/2007/opf" unique-identifier="pub-id" version="3.0" dir="rtl">

@Monirzadeh
Copy link
Contributor Author

@bmaupin if you merge #65 to the main i can merge once again and all test will pass.

@bmaupin
Copy link
Owner

bmaupin commented Aug 14, 2023

@owulveryck Ideally I'd prefer to do some more testing with this one on different eReaders, or maybe even add this as a parameter, but I don't really have the time. I think I'm okay just merging it. If it causes problems we can always revert later. dir=auto seems like a sensible default:

auto, which lets the user agent decide. It uses a basic algorithm as it parses the characters inside the element until it finds a character with a strong directionality, then applies that directionality to the whole element.

(https://developer.mozilla.org/docs/Web/HTML/Global_attributes/dir)

If I understand correctly, it also seems to be the default value used by web browsers.

Does that sound okay to you?

@owulveryck
Copy link
Collaborator

I don't like the idea of merging something that will break the tests, but my understanding is that it is unrelated to this change.
Therefore, I am ok to merge.

@bmaupin
Copy link
Owner

bmaupin commented Aug 16, 2023

I don't like the idea of merging something that will break the tests

Agreed. @Monirzadeh if you can merge or rebase this off the main branch, we can merge it.

I won't be available for the rest of the week but @owulveryck feel free to merge it if the tests are passing.

Thanks!

@Monirzadeh
Copy link
Contributor Author

Monirzadeh commented Aug 16, 2023

i fix test in #65 if you merge that to the main i will merge that again and test will pass

@coveralls
Copy link

Coverage Status

coverage: 88.0% (+0.06%) from 87.94% when pulling b8a0c47 on Monirzadeh:RTL-support into c8df5da on bmaupin:main.

@Monirzadeh
Copy link
Contributor Author

@bmaupin @owulveryck i think it is ready :)

@owulveryck owulveryck merged commit 45f45a4 into bmaupin:main Aug 16, 2023
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants