fix(ui): stop the passcode page telling the user things it cannot know
Four claims on the Bolt passcode page were not backed by anything the receiver reports. The page decided that "This receiver does not report entry progress" from a three-second wall-clock timer since the page appeared. The receiver being silent and the user still reading the sequence are indistinguishable from there, and the user has ten numbered steps to read first, so the sentence fired on essentially every pairing, including receivers that do report progress, and was then contradicted by the first press. It also dropped the highlight to None, so the strip visibly lost and regained its current-step ring on the normal path. Drop the timer and the claim: the count stands at zero and step one stays highlighted until the receiver says otherwise. The status line counted "clicks" on keyboards too, so someone typing digits and pressing enter was told how many clicks had registered, and translators were asked to translate a sentence that is wrong for half the devices it appears on. Pick the wording from the device instead, spelling both variants out in full so each reaches the catalogs as a whole sentence. For mice the literal left/right sequence survived only as a tooltip and an accessible description on a drawing area, so it was in no label on the page at all, which is exactly what the code's own comment said had to be avoided. Keep it as a dim label under the strip, and name the drawing area after it as well, since screen readers announce the name before the description. The unreadable-passcode fallback told the user to type the passcode and press enter. Keyboards always produce steps, so that branch is only ever reached by a device with no keys, and the raw passcode it printed is by definition unprintable garbage. Say that the passcode could not be read and point at the retry instead. Adds regression tests for all four; each one fails against the previous behaviour.
This commit is contained in:
Daniel Banariba
parent
b4a8344533
commit
4c91549a22
|
|
@ -35,7 +35,6 @@ logger = logging.getLogger(__name__)
|
|||
|
||||
_PAIRING_TIMEOUT = 30 # seconds
|
||||
_STATUS_CHECK = 500 # milliseconds
|
||||
_PROGRESS_GRACE = 3 # seconds to wait for a first progress notification before giving up on them
|
||||
|
||||
# steps the user has to perform to enter a passkey
|
||||
_STEP_LEFT = "left"
|
||||
|
|
@ -251,6 +250,22 @@ def _passkey_description(steps, passkey, authentication):
|
|||
return _("Press %(code)s\nand then press left and right buttons simultaneously.") % {"code": passcode}
|
||||
|
||||
|
||||
def _entry_progress_text(steps, done, total):
|
||||
"""Describes how much of the passkey the receiver has accepted, in the words of the device.
|
||||
|
||||
A mouse is clicked and a keyboard is typed on, so the two cannot share a sentence.
|
||||
Both wordings are spelled out in full rather than assembled from fragments, so that
|
||||
each one reaches the translators as a complete sentence.
|
||||
"""
|
||||
if steps[-1] == _STEP_BOTH: # mice and touchpads enter the passcode by clicking
|
||||
if done <= 0:
|
||||
return _("Waiting for the first click…")
|
||||
return _("Clicks registered by the receiver: %(done)d of %(total)d") % {"done": done, "total": total}
|
||||
if done <= 0:
|
||||
return _("Waiting for the first key press…")
|
||||
return _("Key presses registered by the receiver: %(done)d of %(total)d") % {"done": done, "total": total}
|
||||
|
||||
|
||||
@dataclass
|
||||
class _StepCell:
|
||||
"""Placement of a single step inside the step strip."""
|
||||
|
|
@ -398,8 +413,8 @@ def _draw_step_strip(area, cr):
|
|||
if not steps:
|
||||
return False
|
||||
progress = getattr(area, _PROGRESS, None)
|
||||
# without progress notifications no step is singled out, and the strip is
|
||||
# simply the whole sequence, numbered and always readable
|
||||
# the strip is the whole sequence, numbered and always readable, whether or not a
|
||||
# count is known; without one no step is singled out and nothing is marked done
|
||||
done = 0 if progress is None else min(progress, len(steps))
|
||||
current = None if progress is None else min(progress, len(steps) - 1)
|
||||
cells, separator_y, width, _height = _step_strip_cells(steps)
|
||||
|
|
@ -455,10 +470,13 @@ def _create_step_strip(steps, description):
|
|||
_cells, _separator_y, width, height = _step_strip_cells(steps)
|
||||
area.set_size_request(width, int(math.ceil(height)))
|
||||
area.connect(GtkSignal.DRAW.value, _draw_step_strip)
|
||||
# a drawing area is invisible to screen readers, so the sequence has to
|
||||
# stay available as text as well
|
||||
# a drawing area carries no text of its own, so name it after the sequence it
|
||||
# draws; screen readers announce the name first, and the caller also keeps the
|
||||
# sequence in a label of its own
|
||||
area.set_tooltip_text(description)
|
||||
area.get_accessible().set_description(description)
|
||||
accessible = area.get_accessible()
|
||||
accessible.set_name(description)
|
||||
accessible.set_description(description)
|
||||
return area
|
||||
|
||||
|
||||
|
|
@ -480,9 +498,14 @@ def _create_passcode_page(assistant, receiver, passkey):
|
|||
intro_text = _("%(receiver_name)s: pair new device") % {"receiver_name": receiver.name}
|
||||
page_text = _("Enter passcode on %(name)s.") % {"name": name}
|
||||
steps = _passkey_steps(passkey, authentication)
|
||||
if steps is None: # unreadable passkey, so fall back to the plain instructions
|
||||
if steps is None:
|
||||
# every step is derived from the passcode, so an unreadable one leaves nothing
|
||||
# to instruct with. Only non-keyboards can get here with a passcode at all, so
|
||||
# say what happened instead of naming an action the device cannot perform.
|
||||
page_text += "\n"
|
||||
page_text += _("Type %(passcode)s and then press the enter key.") % {"passcode": passkey}
|
||||
page_text += _("Solaar cannot read the passcode this receiver sent, so it cannot show how to enter it.")
|
||||
page_text += "\n"
|
||||
page_text += _("Press the pairing button on your device again and retry.")
|
||||
elif authentication & 0x01: # for keyboards the passcode is meant to be read
|
||||
page_text += "\n"
|
||||
page_text += _passkey_description(steps, passkey, authentication)
|
||||
|
|
@ -495,9 +518,19 @@ def _create_passcode_page(assistant, receiver, passkey):
|
|||
)
|
||||
if steps is None:
|
||||
return page
|
||||
strip = _create_step_strip(steps, _passkey_description(steps, passkey, authentication))
|
||||
description = _passkey_description(steps, passkey, authentication)
|
||||
strip = _create_step_strip(steps, description)
|
||||
page.pack_start(strip, False, False, 0)
|
||||
status = Gtk.Label(label=_("Waiting for the first click…"))
|
||||
if not authentication & 0x01:
|
||||
# the strip is drawn, not written, so the sequence also stays on the page as a
|
||||
# sentence: a tooltip needs a pointer and an accessible description is not text
|
||||
sequence = Gtk.Label(label=description)
|
||||
sequence.set_line_wrap(True)
|
||||
sequence.set_max_width_chars(_LABEL_WIDTH_CHARS)
|
||||
sequence.set_halign(Gtk.Align.CENTER)
|
||||
sequence.get_style_context().add_class("dim-label")
|
||||
page.pack_start(sequence, False, False, 0)
|
||||
status = Gtk.Label(label=_entry_progress_text(steps, 0, len(steps) - 1))
|
||||
status.set_line_wrap(True)
|
||||
status.set_max_width_chars(_LABEL_WIDTH_CHARS)
|
||||
status.set_halign(Gtk.Align.CENTER)
|
||||
|
|
@ -520,7 +553,6 @@ def _create_passcode_page(assistant, receiver, passkey):
|
|||
page.show_all()
|
||||
page.strip = strip
|
||||
page.status = status
|
||||
page.started = time.monotonic()
|
||||
return page
|
||||
|
||||
|
||||
|
|
@ -532,24 +564,18 @@ def _update_passcode_page(page, receiver):
|
|||
steps = getattr(strip, _STEPS)
|
||||
total = len(steps) - 1 # the receiver counts presses, not the final combined one
|
||||
entered = receiver.pairing.passkey_entered
|
||||
# a receiver that reports nothing and a user who has not pressed anything yet look
|
||||
# exactly alike from here, so never claim either: the count stands at zero and the
|
||||
# first step stays highlighted until the receiver says otherwise
|
||||
if receiver.pairing.passkey_complete:
|
||||
progress = len(steps)
|
||||
text = _("The receiver is checking the sequence…")
|
||||
elif entered > 0:
|
||||
elif entered >= total and steps[-1] == _STEP_BOTH:
|
||||
progress = entered
|
||||
if entered >= total and steps[-1] == _STEP_BOTH:
|
||||
text = _("Finally, press the left and right buttons at the same time.")
|
||||
else:
|
||||
text = _("Clicks registered by the receiver: %(done)d of %(total)d") % {
|
||||
"done": min(entered, total),
|
||||
"total": total,
|
||||
}
|
||||
elif time.monotonic() - page.started < _PROGRESS_GRACE:
|
||||
progress = 0
|
||||
text = _("Waiting for the first click…")
|
||||
else: # this receiver never reports progress, so highlight nothing at all
|
||||
progress = None
|
||||
text = _("This receiver does not report entry progress. Follow the numbered steps in order.")
|
||||
text = _("Finally, press the left and right buttons at the same time.")
|
||||
else:
|
||||
progress = entered
|
||||
text = _entry_progress_text(steps, min(entered, total), total)
|
||||
page.status.set_label(text)
|
||||
setattr(strip, _PROGRESS, progress)
|
||||
strip.queue_draw()
|
||||
|
|
|
|||
|
|
@ -285,6 +285,28 @@ def test_step_strip_cells(steps, expected_cells):
|
|||
assert width > 0 and height > cells[-1].y
|
||||
|
||||
|
||||
def _page_labels(page):
|
||||
return [child.get_label() for child in page.get_children() if isinstance(child, Gtk.Label)]
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"passkey, authentication, forbidden",
|
||||
[("50", 0x02, "key press"), ("000918", 0x01, "click")],
|
||||
)
|
||||
def test_entry_progress_text_uses_the_words_of_the_device(passkey, authentication, forbidden):
|
||||
"""A mouse is clicked and a keyboard is typed on, so neither may borrow the other's
|
||||
vocabulary — the wrong one reaches the translators as well as the user."""
|
||||
steps = pair_window._passkey_steps(passkey, authentication)
|
||||
total = len(steps) - 1
|
||||
|
||||
waiting = pair_window._entry_progress_text(steps, 0, total)
|
||||
counted = pair_window._entry_progress_text(steps, 3, total)
|
||||
|
||||
assert forbidden not in waiting.lower()
|
||||
assert forbidden not in counted.lower()
|
||||
assert "3" in counted and str(total) in counted
|
||||
|
||||
|
||||
@pytest.mark.skipif(not gtk_init, reason="requires Gtk")
|
||||
def test_show_passcode_appends_a_single_page():
|
||||
"""The periodic check keeps running while the passkey is shown, so the page has to
|
||||
|
|
@ -350,6 +372,52 @@ def test_show_passcode_clamps_overshoot():
|
|||
assert getattr(page.strip, pair_window._PROGRESS) == 99 # clamped when drawn, not when stored
|
||||
|
||||
|
||||
@pytest.mark.skipif(not gtk_init, reason="requires Gtk")
|
||||
def test_show_passcode_never_blames_the_receiver_for_a_slow_user(monkeypatch):
|
||||
"""A receiver that reports nothing and a user who has not pressed anything yet look
|
||||
exactly alike from the page, so no amount of elapsed time may turn one into a claim
|
||||
about the other, and the current step must keep its highlight throughout."""
|
||||
r = Receiver(
|
||||
"passcode",
|
||||
"bolt",
|
||||
True,
|
||||
receiver.Pairing(lock_open=True, device_passkey="50", device_authentication=0x02),
|
||||
)
|
||||
assistant = Assistant(True)
|
||||
|
||||
pair_window._check_lock_state(assistant, r, 0)
|
||||
page = getattr(assistant, pair_window._PASSKEY_PAGE)
|
||||
waiting = page.status.get_label()
|
||||
|
||||
real_monotonic = time.monotonic
|
||||
monkeypatch.setattr(time, "monotonic", lambda: real_monotonic() + 3600)
|
||||
pair_window._update_passcode_page(page, r)
|
||||
|
||||
assert page.status.get_label() == waiting
|
||||
assert getattr(page.strip, pair_window._PROGRESS) == 0
|
||||
|
||||
|
||||
@pytest.mark.skipif(not gtk_init, reason="requires Gtk")
|
||||
def test_passcode_page_keeps_the_click_sequence_as_text():
|
||||
"""The strip is a drawing area, so it carries no text: the sequence has to stay on
|
||||
the page as a sentence, since a tooltip needs a pointer to be seen at all."""
|
||||
r = Receiver(
|
||||
"passcode",
|
||||
"bolt",
|
||||
True,
|
||||
receiver.Pairing(lock_open=True, device_passkey="50", device_authentication=0x02),
|
||||
)
|
||||
assistant = Assistant(True)
|
||||
|
||||
pair_window._check_lock_state(assistant, r, 0)
|
||||
|
||||
page = getattr(assistant, pair_window._PASSKEY_PAGE)
|
||||
steps = pair_window._passkey_steps("50", 0x02)
|
||||
sequence = pair_window._passkey_description(steps, "50", 0x02)
|
||||
assert sequence in _page_labels(page)
|
||||
assert page.strip.get_accessible().get_name() == sequence
|
||||
|
||||
|
||||
@pytest.mark.skipif(not gtk_init, reason="requires Gtk")
|
||||
def test_show_passcode_survives_unreadable_passkey():
|
||||
r = Receiver(
|
||||
|
|
@ -365,6 +433,26 @@ def test_show_passcode_survives_unreadable_passkey():
|
|||
assert len(assistant.pages) == 1
|
||||
|
||||
|
||||
@pytest.mark.skipif(not gtk_init, reason="requires Gtk")
|
||||
def test_unreadable_passkey_does_not_ask_a_mouse_to_type():
|
||||
"""Keyboards always yield steps, so this path is only ever reached by a device with
|
||||
no keys to type on and no enter key to press."""
|
||||
r = Receiver(
|
||||
"passcode",
|
||||
"bolt",
|
||||
True,
|
||||
receiver.Pairing(lock_open=True, device_passkey="oops", device_authentication=0x02),
|
||||
)
|
||||
assistant = Assistant(True)
|
||||
|
||||
pair_window._check_lock_state(assistant, r, 0)
|
||||
|
||||
page = getattr(assistant, pair_window._PASSKEY_PAGE)
|
||||
text = "\n".join(_page_labels(page))
|
||||
assert "enter key" not in text
|
||||
assert "cannot read the passcode" in text
|
||||
|
||||
|
||||
@pytest.mark.skipif(not gtk_init, reason="requires Gtk")
|
||||
def test_pair_device_issued_once(mocker):
|
||||
r = Receiver("discovered", "bolt", True, receiver.Pairing(discovering=True, device_address=2, device_name=5))
|
||||
|
|
|
|||
Loading…
Reference in New Issue