psiegman / epublib

a java library for reading and writing epub files
http://www.siegmann.nl/epublib
1.04k stars 313 forks source link

RTL Support in EpubWriter #90

Open mardawi opened 8 years ago

mardawi commented 8 years ago

Created a patch to add RTL support: https://www.dropbox.com/s/n96wh0bk9fyvmtp/added_RTL_support.patch?dl=0

Hope it finds its way to the next build.

psiegman commented 8 years ago

hi Mardawi,

Thanks for the patch ! However, can you make an enum value for it instead of the two string constants ?

Secondly: I see the text direction being written, but where is it read ?

regards, Paul

timkoers commented 8 years ago

Just to give you some advice, I don’t suggest using enum’s since they are a huge performance crasher. I suggest to make these values an int value.

Van: Paul Siegmann [mailto:notifications@github.com] Verzonden: maandag 2 november 2015 16:23 Aan: psiegman/epublib epublib@noreply.github.com Onderwerp: Re: [epublib] RTL Support in EpubWriter (#90)

hi Mardawi,

Thanks for the patch ! However, can you make an enum value for it instead of the two string constants ?

Secondly: I see the text direction being written, but where is it read ?

regards, Paul

— Reply to this email directly or view it on GitHub https://github.com/psiegman/epublib/issues/90#issuecomment-153051354 . https://github.com/notifications/beacon/AEELRZdkatQI-gVGOENpO7hsnMUR2vdWks5pB3dQgaJpZM4GaJ5f.gif

psiegman commented 8 years ago

Hi Timkoers,

I've never heard of enum values being a performance problem. Can you point me to some more information about this ?

regards, Paul

timkoers commented 8 years ago

Ofcourse,

https://www.youtube.com/watch?v=Hzs6OBcvNQE

The Android Developers channel has covered this subject a while ago.

It is not a performance problem directly, but if the device is low on ram it could become one.

Van: Paul Siegmann [mailto:notifications@github.com] Verzonden: maandag 2 november 2015 16:27 Aan: psiegman/epublib epublib@noreply.github.com CC: timkoers tim.koers@live.nl Onderwerp: Re: [epublib] RTL Support in EpubWriter (#90)

Hi Timkoers,

I've never heard of enum values being a performance problem. Can you point me to some more information about this ?

regards, Paul

— Reply to this email directly or view it on GitHub https://github.com/psiegman/epublib/issues/90#issuecomment-153052259 . https://github.com/notifications/beacon/AEELRQEvvmsGE2hbRMSG4wL0v-Y1k6TUks5pB3gvgaJpZM4GaJ5f.gif

psiegman commented 8 years ago

Hi Timkoers,

I watched the movie, poked around (here for instance: http://stackoverflow.com/questions/29183904/should-i-strictly-avoid-using-enums-on-android)

And considering that an enum is better for code clarity and it's only used in one place I'm still sticking to an enum for this.

regards, Paul

mardawi commented 8 years ago

enums have a performance issue with Android only: http://stackoverflow.com/questions/5143256/why-was-avoid-enums-where-you-only-need-ints-removed-from-androids-performanc

I was trying to write some RTL ePub files. Here's the patch with reading direction for ePub: https://www.dropbox.com/s/txep4nyvut7m09g/added_read-write_RTL_support1.patch?dl=0

timkoers commented 8 years ago

That’s true, shoot my fault. I thought that this was an Android library.

Van: Ammar Mardawi [mailto:notifications@github.com] Verzonden: maandag 2 november 2015 16:41 Aan: psiegman/epublib epublib@noreply.github.com CC: timkoers tim.koers@live.nl Onderwerp: Re: [epublib] RTL Support in EpubWriter (#90)

enums have a performance issue with Android only: http://stackoverflow.com/questions/5143256/why-was-avoid-enums-where-you-only-need-ints-removed-from-androids-performanc

I was trying to write some RTL ePub files. Here's the patch with reading direction for ePub: https://www.dropbox.com/s/txep4nyvut7m09g/added_read-write_RTL_support1.patch?dl=0

— Reply to this email directly or view it on GitHub https://github.com/psiegman/epublib/issues/90#issuecomment-153056700 . https://github.com/notifications/beacon/AEELRc4Vie8cEU6TDV7wSXmwd1HoLqTGks5pB3t3gaJpZM4GaJ5f.gif

psiegman commented 8 years ago

epublib is both a regular java and an android library.

In this case though I think the memory penalty is worth it. It's a single enum value per book. I can live with that.

Paul