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

Display pathnames correctly in a Right-to-Left context (bsc#1128091) #873

Merged
merged 9 commits into from May 12, 2021

Conversation

mvidner
Copy link
Member

@mvidner mvidner commented Mar 14, 2019

Problem

In Arabic, the device table displays /dev/sda1 as dev/sda1/

Solution

Use Unicode BiDi formatting characters in the source (creating a machine "translation" of the string).

Disabled for TUI: libyui-ncurses does not handle the BiDi characters well, truncating the strings. TUI does not have the general RTL direction anyway.

If you start off with a JEOS and only install the bare minimum of packages needed to test this, you may end up seeing dotted box characters in place of the BiDi controls. Installing noto-sans-fonts.rpm helps.

Testing

  • Added a new unit test
  • Tested manually
  • Fix and test handling /

Screenshots

beforeafter

@mvidner mvidner force-pushed the rtl-pathnames branch 3 times, most recently from a3017c2 to 8709057 Compare March 15, 2019 12:59
# then it would display as dev/sda1/ in a RTL context.
# Unicode BiDi Left-to-right embedding (LRE),
# then Pop Directional Formatting (PDF)
format("\u{202A}%s\u{202C}", s)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the actual fix. The rest of the PR 1) works around a libyui-ncurses bug 2) adjusts the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the "after" screenshot I see boot/efi/. Is that the expected outcome? Whenever a user tries to use that path for anything, there will probably be an error since the system expects /boot/efi.

I understand this must be difficult for users of such an RtoL language (Arabic or Hebrew); if your natural reading direction is right-to-left, how is a user supposed to read such a technical path specification that is really still entirely Western-style? They might still start reading that part from the right, only then to realize that this is really left-to-right; but boot/efi/ seems wrong in both worlds.

Looks like I had opened the "before" screenshot twice. My fault. Disregard.

@coveralls
Copy link

coveralls commented Nov 19, 2020

Coverage Status

Coverage decreased (-0.02%) to 97.721% when pulling 4015fbd on rtl-pathnames into 6595d26 on master.

@mvidner
Copy link
Member Author

mvidner commented Nov 22, 2020

The PR rules demand screenshots... and that's a good thing because comparing the screenshots made me realize I have a bug in handling the root / path.

Martin Vidner added 5 commits May 6, 2021 09:53
Problem: In Arabic, the device table displays /dev/sda1 as dev/sda1/

That's because / is in general a weak character but pathnames should be
treated as overall left-to-right while isolated by the slashes.

Solution:
Use Unicode BiDi formatting characters in the source
(creating a machine "translation" of the string).
Copy link
Contributor

@shundhammer shundhammer left a comment

Choose a reason for hiding this comment

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

LGTM, and AFAICS it fixes this particular bug.

But I fear we will need this in more places, so in the long run, we will probably need those Bidi routines in a more central place (yast-yast2 package?) so they are accessible from other packages as well.

Also, we will probably need this in C++ as well; libstorage-ng comes to mind. We have places where libstorage-ng generates user-visible messages that include both translated text and paths: The action graph and the subvolume actions. They will have the same problem; unless the Arabic and the Hebrew translator inserted those bidi markers manually in each message.

@mvidner
Copy link
Member Author

mvidner commented May 12, 2021

we will probably need those Bidi routines in a more central place

Yup, that's likely, so I named the library so that it is easily movable without needing to change the usage places. I've even checked for collisions with other bidi gems (which are solving different problems, so merging is unlikely).

@mvidner
Copy link
Member Author

mvidner commented May 12, 2021

Actually, most of the other places do not need special handling like this, because they can be solved with the usual special handling, which is using BiDi control characters in the translated messages. The storage table is special in having cells containing just filesystem paths, which noone has thought would need translation.

src/lib/bidi_markup.rb Show resolved Hide resolved
context "when the path is root /" do
let(:device_name) { "/dev/sda" }

it "returns the mount path with wrapped with LTR Isolate" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover with.

# >
# > Memory: he said "(RLI)I NEED WATER!(PDI)", and expired.
# > Display: he said "!RETAW DEEN I", and expired.
module BidiMarkup
Copy link
Member

Choose a reason for hiding this comment

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

Is this yast2-storage-ng specific problem? I think this should be moved to yast2, just in case we need the same solution elsewhere....

Copy link
Member Author

Choose a reason for hiding this comment

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

In the end it looks like it might indeed be specific to storage, but the file is designed to be easily movable once we find out we need it.

Martin Vidner added 4 commits May 12, 2021 17:09
to make independent of y2partitioner,
Also not to collide with https://rubygems.org/gems/bidi
(which cares only about visual rendering)
add missing licenses to Ruby and Bash sources
skip non-programs
skip PDFs
@mvidner mvidner merged commit 825398b into master May 12, 2021
@mvidner mvidner deleted the rtl-pathnames branch May 12, 2021 15:26
@yast-bot
Copy link

✔️ Public Jenkins job #370 successfully finished
✔️ Created OBS submit request #892537

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.

None yet

6 participants