Mounting duplicate widget IDs can result in a NoScreen error
#2863
Comments
Yes, this does go against all things Pythonic, but in this case it's likely less costly to do the check first; moreover it works around the problem I ran in to: Textualize#2863
* Add a notification class and a class to hold notifications This provides the core classes for holding information on a single notification, and then on top of that a class for managing a collection of notifications. * WiP: End of day/week commit to pick up post-holiday * Ask permission rather than forgiveness Yes, this does go against all things Pythonic, but in this case it's likely less costly to do the check first; moreover it works around the problem I ran in to: #2863 * Move the handling of "I've seen this" into the toast rack This way the interface becomes "here's a bunch of notifications, you go work this out". * Add a notify method to all widgets * The removal time for a toast should be the time left When it was per-screen, it made sense that it was the timeout; now that we're carrying them over between screens we're going to make sure they're only around for as long as they need to be. * Carry notifications between screens * Remove the test code * Drop the borders from the toasts Except for the title, keep that. * Provide access to the notification timeout * Remove the title panel from a Toast if the title is empty * Make the Toast CSS classes "private" Prefix with a - to reduce the chance of a clash with userspace. * Refresh a docstring * Stop widget leakage The Toasts were removing themselves, but they're wrapped inside a helper container that keeps them aligned right. So the problem was that the alignment containers were leaking. This ensures that when a Toast goes away it takes its parent with it. * Make the alignment container hidden This doesn't really make any difference, but it feels like it makes sense to hide it if there's nothing to show -- it's purely for alignment. * 🚚 Rename the toast container This is about getting the toasts to align correctly (even when you do align things, they don't really align as expected due to the way that a container aligns the bounding box of all if its children, not the individual children). However, I had this named after where it aligned them to; someone using the system may wish to change that, so let's make the name more generic. * Improve ToastRack._toast_id Add a docstring, and also change the format of the identity somewhat so that it's even "more internal". * Add some initial low-level notification testing * Add initial testing of notifications within an application * Add tests for notifying from the 3 main levels within the DOM * Add a toast example to the docs and a snapshot test This might not be the final form, but it'll do for the moment. I want to get the snapshot test in place at least. * Add a snapshot test for notifications persisting between screens * Add some documentation for a Toast This isn't going into the index, just yet. This is *technically* an internal widget so I'm not sure how and where it makes sense to document it; if at all. But let's get some documentation in here anyway. * Flesh out the docstrings for the notify methods * Add a missing docstring to the Notifications __init__ method * Add snapshot tests for persisting notifications through mode switches * Remove unused import Looks like eglot/pyright tried to be "helpful" at some point and I didn't notice. * Correct the Toast severity level classes in the docs Originally they weren't in the "internal" namespace, then I decided that they should be so there's less chance of a clash with dev-space code; but I forgot to reflect this in the docs. This fixes that. * Make the removal of notifications/toasts a two way thing The addition of the ability to dismiss a toast by clicking on it had a flaw: the notification->toast code had been written with things being one way. The expiration of notifications happened in the notification handler, and the expiration of Toasts was done in the Toast system, on purpose (notifications might end up being routed via elsewhere so this needs to be done). But... this meant that hand-removed Toasts kept coming back from the dead when a new notification was raised iff the hand-removed ones hadn't yet expired. So here I add the ability the remove a notification from the notification collection. * Remove an unhelpful comment Sort of a hangover from what was initially looking like it was going to be a longer body of code. It doesn't really need explaining any more. * Add in support to the notification collection * Change the toast rack adder to be a general "show" method This turns the method into one that further aids making the connection between the notifications and the toasts two way. Now it makes sense that if there are toasts for notifications that no longer exist, they also get removed. This makes it easier to add all sorts of clear options later on. * Add a method to clear notifications * Add an App method for clearing all existing notifications * Add a missing docstring to _refresh_notifications * Return the notification from the notify methods It can be seen as, and used as, a handle of sorts (see unnotify); so return it so people can use it. * Add some more notifications unit testing * Add some more app-level notification unit testing * Style tweaks * docs * added notifications * snapshots --------- Co-authored-by: Will McGugan <[email protected]>
|
I've just spent about 40 minutes trying to isolate this error (something not made any easier by not actually having the non-isolated code easily to hand as that was an earlier incarnation of the notification code) and have utterly failed. I'm not overly keen on fixing something for which a test can't be constructed. Should we blindly apply the suggested fix, knowing that it doesn't break existing functionality/tests? Or should we backlog this until such a time as we manage to recreate this error again? |
|
We had wondered if it might be down to the fast removal and then mounting of widgets in the same location in the DOM, but attempting to recreate that didn't come to anything too. Moving this to "blocked" for the moment; it would be best to fix this knowing what we're actually fixing. Meanwhile, if anyone gets the error reported at the start: let me know! |
|
I am seeing the same issue working on https://github.com/maxking/archive_reader. I was relying on https://github.com/maxking/archive_reader/blob/main/src/archive_reader/app.py#L370-L373 |
|
@davep That may be the fix. Let's spend a couple of hours to see if we can recreate it. |
|
This has been around for a while and we can't seem to reproduce it regardless of how hard we try. |
|
Don't forget to star the repository! Follow @textualizeio for Textual updates. |
I'm writing this one up near the end of the day, at the start of a holiday, just so it's written down and I/we don't forget. Annoyingly I can't recreate the problem in isolation, only in the code I'm working on at the moment.
It is possible to get into a situation where an attempt to
mounta widget with an ID that already exists amongst its would-be siblings results in aNoScreenerror being thrown from somewhere deep in the compositor code rather than theDuplicateIds, which is what you'd expect.The error I was seeing is follows at the end of here.
Quick testing suggests that
NodeList._appendandNodeList._insertare the issue here. Making a change akin to this:seems to address the issue.
If I can work out an isolated reproduction of this I'll add it here, but the above change does seem to make some sense I think.
The text was updated successfully, but these errors were encountered: