peterbrittain / asciimatics

A cross platform package to do curses-like operations, plus higher level APIs and widgets to create text UIs and ASCII art animations
Apache License 2.0
3.61k stars 238 forks source link

`RadioButtons` does not consider the right pallet when item is selected without focus #387

Closed jbfro closed 2 months ago

jbfro commented 3 months ago

Describe the bug When a radio button is selected but not focused the wrong pallet color is chosen.

To Reproduce

  1. Override the selected_control and selected_field pallet definition with Screen.COLOUR_YELLOW color for better observation.
  2. When the radiobuttons widget is focused and selected, the color is okay.
  3. When the radiobuttons widget losses the focus, then the selected item does not get colored in accordance with selected_control and selected_field.

wrong

Expected behavior

  1. Override the selected_control and selected_field pallet definition with Screen.COLOUR_YELLOW color for better observation.
  2. When the radiobuttons widget is focused and selected, the color is okay.
  3. When the radiobuttons widget losses the focus, then the selected item gets colored in accordance with selected_control and selected_field.

okay

Potential fix to this issue

The bellow patch is based on the master branch.

From c89160b2cb8c2503a745da6f937021bf14f121cd Mon Sep 17 00:00:00 2001
From: Jean-Baptiste FROMENTEAU <jb.fromenteau@gmail.com>
Date: Sat, 4 May 2024 17:11:53 +0200
Subject: [PATCH] Fix radiobuttons pallet selection when item is selected

Signed-off-by: Jean-Baptiste FROMENTEAU <jb.fromenteau@gmail.com>
---
 asciimatics/widgets/radiobuttons.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/asciimatics/widgets/radiobuttons.py b/asciimatics/widgets/radiobuttons.py
index 999e8bf..9d6e6e3 100644
--- a/asciimatics/widgets/radiobuttons.py
+++ b/asciimatics/widgets/radiobuttons.py
@@ -37,8 +37,8 @@ class RadioButtons(Widget):

         # Render the list of radio buttons.
         for i, (text, _) in enumerate(self._options):
-            fg, attr, bg = self._pick_colours("control", self._has_focus and i == self._selection)
-            fg2, attr2, bg2 = self._pick_colours("field", self._has_focus and i == self._selection)
+            fg, attr, bg = self._pick_colours("control", i == self._selection)
+            fg2, attr2, bg2 = self._pick_colours("field", i == self._selection)
             check = check_char if i == self._selection else " "
             self._frame.canvas.print_at(
                 f"({check}) ",
-- 
2.25.1
peterbrittain commented 3 months ago

Good spot! I need to think about how this should affect other widgets too.

jbfro commented 3 months ago

As per my investigations it only affects the RadioButtons widget. Other widgets call the _pick_colours function with the proper test i == current_entry and it works fine since the focus state is checked by _pick_palette_key.

peterbrittain commented 2 months ago

The thing that bothers me here is the inconsistent state with check boxes. How about this patch instead?

diff --git a/asciimatics/widgets/checkbox.py b/asciimatics/widgets/checkbox.py
index 13f96db..453e267 100644
--- a/asciimatics/widgets/checkbox.py
+++ b/asciimatics/widgets/checkbox.py
@@ -32,13 +32,13 @@ class CheckBox(Widget):

         # Render this checkbox.
         check_char = "✓" if self._frame.canvas.unicode_aware else "X"
-        (colour, attr, bg) = self._pick_colours("control", self._has_focus)
+        (colour, attr, bg) = self._pick_colours("control", self._has_focus or self._value)
         self._frame.canvas.print_at(
             f"[{check_char if self._value else ' '}] ",
             self._x + self._offset,
             self._y,
             colour, attr, bg)
-        (colour, attr, bg) = self._pick_colours("field", self._has_focus)
+        (colour, attr, bg) = self._pick_colours("field", self._has_focus or self._value)
         self._frame.canvas.print_at(
             self._text,
             self._x + self._offset + 4,
diff --git a/asciimatics/widgets/radiobuttons.py b/asciimatics/widgets/radiobuttons.py
index 999e8bf..9d6e6e3 100644
--- a/asciimatics/widgets/radiobuttons.py
+++ b/asciimatics/widgets/radiobuttons.py
@@ -37,8 +37,8 @@ class RadioButtons(Widget):

         # Render the list of radio buttons.
         for i, (text, _) in enumerate(self._options):
-            fg, attr, bg = self._pick_colours("control", self._has_focus and i == self._selection)
-            fg2, attr2, bg2 = self._pick_colours("field", self._has_focus and i == self._selection)
+            fg, attr, bg = self._pick_colours("control", i == self._selection)
+            fg2, attr2, bg2 = self._pick_colours("field", i == self._selection)
             check = check_char if i == self._selection else " "
             self._frame.canvas.print_at(
                 f"({check}) ",
peterbrittain commented 2 months ago

I think the pushed version is the right logic consistently applied across widgets. Feel free to reopen if you have a counterproposal.