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

Command Palette duplicate ID on input kill (meta: OptionList clearing bug) #3714

Closed
davep opened this issue Nov 20, 2023 · 8 comments
Closed
Assignees
Labels
bug Something isn't working Task

Comments

@davep
Copy link
Contributor

davep commented Nov 20, 2023

At the moment, this issue is the opposite of an MRE, but is instead a reminder to make some time to try and produce one.

While riffing with a current pet project, I've found that it's possible to get the command palette to crash with DuplicateID: Attempt made to add options with duplicate IDs -- this cropped up a few times during development, but I thought we had it nailed.

There is no exact recipe at the moment, but I have found this evening that when I can recreate it, the steps tend to be:

  • Type something that matches a handful or more commands
  • Hit Ctrl+a Ctrl+k

If the wind is in the right direction you'll be left with an empty Input but will still have commands in the results list:

Screenshot 2023-11-20 at 21 48 28

that is the main problem; for sure. For some reason the results haven't cleared down. After that, if you type something more, you'll get the DuplicateID.

My initial suspicion would be that something is going wrong with CommandPalette._input.

The first job here though is to try and narrow this down to a simple bit of code to recreate it (that's easy enough, it should just be a simple application with a command provider that has lots of hits), and a recipe to follow to get the error to happen on demand.

@davep davep added bug Something isn't working Task labels Nov 20, 2023
@TomJGooding
Copy link
Contributor

This is probably unrelated to this error, but there are possibly some issues with OptionList validation:

@davep
Copy link
Contributor Author

davep commented Dec 12, 2023

While working on a pet project, I've managed to recreate the DuplicateID issue at will, in a context other than with the command palette, and it does seem to be down to a wee bug in OptionList. Frustratingly it's proving impossible to create the issue in an isolated MRE, but the cause is very obvious in context so I suspect it's affected by how busy the message queues are.

The upshot of it seems to be this: if you try and clear and reload options for an OptionList that has no size, the list of known IDs doesn't get cleared down while other things are cleared down. I suspect the fix would be to either add a call to OptionList._clear_content_tracking to OptionList.clear_options, or to change OptionList._refresh_content_tracking so that the call to OptionList._clear_content_tracking happens before the test for self.size.width.

It's that test for size that seems to be kicking in in my current application; and I suspect this will explain why this is sometimes, but not always, seen with the command palette: there are times where it doesn't have size yet.

@davep davep changed the title Speculative/reminder: Command Palette duplicate ID on input kill Command Palette duplicate ID on input kill (meta: OptionList clearing bug) Dec 12, 2023
davep added a commit to davep/tinboard that referenced this issue Dec 12, 2023
@davep
Copy link
Contributor Author

davep commented Dec 19, 2023

As I've just run into this again a couple of times this morning in an app I'm testing, and as I think there's an easy win to be had if my assessment of the cause is correct, tagging @willmcgugan for triage.

@davep
Copy link
Contributor Author

davep commented Feb 19, 2024

I'm almost positive that this has been resolved by the changes @rodrigogiraoserrao made to OptionList and/or by #4103.

@rodrigogiraoserrao
Copy link
Contributor

I'm almost positive that this has been resolved by the changes @rodrigogiraoserrao made to OptionList and/or by #4103.

The original issue comment mentions a keybinding combo that leaves the command palette with no input but with some commands still showing.
Even if we fixed the duplicate ID error, neither my work on OptionList nor your linked PR solve that, right?

@davep
Copy link
Contributor Author

davep commented Feb 19, 2024

I suspect that will have been down to happenstance of how and when the exception occurred; moreover it wasn't an exact set of instructions for recreation of the problem, more just recording acts that happened around the time of the problem happening.

That's not to say this issue should be closed, but I'm pretty confident it's now handled.

@davep davep self-assigned this Mar 5, 2024
@davep
Copy link
Contributor Author

davep commented Mar 5, 2024

I've given this a bit of a blast with the project that originally tickled the problem in November last year. After a pretty comprehensive attempt to recreate the problem I'm utterly failing (and this is a project that can have a lot of commands drop into the command palette -- I even quickly added command discovery support to overload it as much as possible without a hint of a problem).

Given the deeper dive I did while on holiday in December and given the subsequent fix to that particular problem, I feel confident in closing this as resolved by the further work done on OptionList.

@davep davep closed this as completed Mar 5, 2024
Copy link

github-actions bot commented Mar 5, 2024

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

davep added a commit to davep/quizzical that referenced this issue Nov 23, 2024
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

No branches or pull requests

3 participants