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

[RTL UI] Bidi wrapping tweaks #5694

Merged
merged 3 commits into from Dec 17, 2019
Merged

[RTL UI] Bidi wrapping tweaks #5694

merged 3 commits into from Dec 17, 2019

Conversation

@poire-z
Copy link
Contributor

poire-z commented Dec 17, 2019

Some followup tweaks to #5667:

  • Alias everything to Bidi.nowrap() when in LTR UI, as using LTR isolates seems uneeded when already LTR. This might avoid unforeseen side effects for LTR languages (safer for the coming up release, but we might then notice general wrapping issues only in RTL UI, so less quickly :)
  • Better wrapping of RTL filename by using auto direction and LTR-isolating only the suffix so it's always on a side. That's option C in #5359 (comment), used in the screenshot below.
  • menu.lua: handle bidi_wrap_func outside getMenuText(), which may be used in other contexts. See #5693 (comment).
  • GetText/Bidi: wrap untranslated strings in LTR. See #5667 (comment)
  • util.getFriendlySize(): remove leading space, which will fix:
    image

This change is Reviewable

@poire-z poire-z force-pushed the poire-z:bidi_wrap_tweaks branch from db61b79 to 2f3327a Dec 17, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Dec 17, 2019

@Frenzie: I'll be removing the leading space and your with minimum field width alignment from #3650 - as I checked we don't need this left padding anywhere (and the %4.1f was my code, and probably not intended and cut and pasted from somewhere else - and wrong anyway as it should have been %5.1f to have 123.4 and 1.2 right aligned similarly).
Or I can make it an option, even if we don't use it for now.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Dec 17, 2019

OK, adding it as an option. For now RTL UI will be as on the left, but we can switch to like on the right if prefered:
image vs image

edit: but with RTL, the KB/MB should be on the left of the number, so we'll be on the left aligned case, with no left padding needed

Left align by default, but allow right alignment by
padding left with spaces.
@poire-z poire-z force-pushed the poire-z:bidi_wrap_tweaks branch from 2f3327a to f7e6cb1 Dec 17, 2019
poire-z added 2 commits Dec 17, 2019
- Alias everything to Bidi.nowrap() when in LTR UI, as using
  LTR isolates seems uneeded when already LTR.
- Better wrapping of RTL filename by using auto direction and
  LTR-isolating only the suffix so it's always on a side.
- menu.lua: handle bidi_wrap_func outside getMenuText(), which
  may be used in other contexts.
- Add BD.filepath() and BD.dirpath()
@poire-z poire-z force-pushed the poire-z:bidi_wrap_tweaks branch from f7e6cb1 to d4562c2 Dec 17, 2019
@Frenzie Frenzie added this to the 2019.12 milestone Dec 17, 2019
-- It should do nothing when the UI language is LTR.
GetText.wrapUntranslated_nowrap = function(text) return text end
GetText.wrapUntranslated = GetText.wrapUntranslated_nowrap
-- Note: this won't be possible if we switch from our Lua GetText to

This comment has been minimized.

Copy link
@Frenzie

Frenzie Dec 17, 2019

Member

I'm sure it should be possible, it's just sneakily slightly incompatible. ;-)

@poire-z poire-z merged commit 6596e5d into koreader:master Dec 17, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z poire-z deleted the poire-z:bidi_wrap_tweaks branch Dec 17, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Dec 18, 2019

edit: but with RTL, the KB/MB should be on the left of the number, so we'll be on the left aligned case, with no left padding needed

OK, they will. But they are not currently because of the added (by this PR) wrap untranslated strings in LTR. Once they are translated, they should be displayed like the following (but just for showing, I removed the GetText.untranslated > LTR, so now the N items in directory looks stranger):

image

image

Still to understand why the ellipsis when truncating is on the wrong side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.