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

OptionList duplicate ID detection not working as expected #3455

Closed
davep opened this issue Oct 4, 2023 · 4 comments · Fixed by #3459
Closed

OptionList duplicate ID detection not working as expected #3455

davep opened this issue Oct 4, 2023 · 4 comments · Fixed by #3459
Assignees
Labels
bug Something isn't working Task

Comments

@davep
Copy link
Contributor

davep commented Oct 4, 2023

Consider the following code:

from textual.app import App, ComposeResult
from textual.widgets import OptionList, Log, Footer
from textual.widgets.option_list import Option

class OptionListAddBug(App[None]):

    BINDINGS = [
        (str(n), f"add({n})", f"Add {n}") for n in range(10)
    ]

    CSS = """
    OptionList, Log {
        height: 1fr;
        border: round cornflowerblue;
    }
    """

    def compose(self) -> ComposeResult:
        yield OptionList()
        yield Log()
        yield Footer()

    def action_add(self, value: int) -> None:
        try:
            self.query_one(OptionList).add_option(
                Option(f"Added {value} just fine", id=f"id-{value}")
            )
        except Exception as error:
            self.query_one(Log).write_line(f"{error!r}")


if __name__ == "__main__":
    OptionListAddBug().run()

If you run it and press keys 0-9, you'll find that corresponding options are added to the OptionList.

Screenshot 2023-10-04 at 15 26 43

However, if you start the app afresh, then press (for example) 2 twice, resulting in a logged DuplicateID exception (expected and desired), after than you can't add any more options to the list, and the attempted added option is always reported as having the original duplicate ID:

Screenshot 2023-10-04 at 15 28 53

In the above I pressed 2 twice and then a number of other number keys.

It would appear that OptionList is getting every confused in its id tracking and/or duplicate detection.

@davep davep added bug Something isn't working Task labels Oct 4, 2023
davep added a commit to davep/textual-sandbox that referenced this issue Oct 4, 2023
@TomJGooding
Copy link
Contributor

I found a quick workaround would be:

        except Exception as error:
            self.query_one(Log).write_line(f"{error!r}")

            self.query_one(OptionList)._contents.pop()

The exception is raised by _refresh_content_tracking which loops through _contents.

if content.id in option_ids:
raise DuplicateID(
f"The option list already has an option with id '{content.id}'"
)

@davep
Copy link
Contributor Author

davep commented Oct 4, 2023

Yeah, pretty sure the duplicate check needs to go waaaaaaay before anything gets added.

@davep
Copy link
Contributor Author

davep commented Oct 4, 2023

Smoking gun.

diff --git a/tests/option_list/test_option_list_create.py b/tests/option_list/test_option_list_create.py
index 7e0bbc49..98130d75 100644
--- a/tests/option_list/test_option_list_create.py
+++ b/tests/option_list/test_option_list_create.py
@@ -119,5 +119,8 @@ async def test_add_later() -> None:
 async def test_create_with_duplicate_id() -> None:
     """Adding an option with a duplicate ID should be an error."""
     async with OptionListApp().run_test() as pilot:
+        option_list = pilot.app.query_one(OptionList)
+        assert option_list.option_count == 5
         with pytest.raises(DuplicateID):
-            pilot.app.query_one(OptionList).add_option(Option("dupe", id="3"))
+            option_list.add_option(Option("dupe", id="3"))
+        assert option_list.option_count == 5```

with that change the tests it fails.

davep added a commit to davep/textual that referenced this issue Oct 4, 2023
@davep davep self-assigned this Oct 4, 2023
davep added a commit to davep/textual that referenced this issue Oct 4, 2023
Simply put: until now the OptionList was adding the new options and then
checking if the ID was a duplicate. If some code was to then catch
DuplicateID and treat it as a pass this would then have an unintended
side-effect that the duplicate had been added anyway. The ultimate effect of
this being that once there was an attempt to add a duplicate, nothing more
could be added to that OptionList.

Fixes Textualize#3455.
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants