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

Feature Request: add 'close' method to IGUI interface #1200

Open
mdickinson opened this issue Feb 21, 2023 · 3 comments
Open

Feature Request: add 'close' method to IGUI interface #1200

mdickinson opened this issue Feb 21, 2023 · 3 comments

Comments

@mdickinson
Copy link
Member

Some downstream IGUI implementations allocate system resources, or modify global state, at instance creation time. (For example, one GUI subclass that we have in a downstream project changes the asyncio global event loop.) As a result, we've added close methods to those implementations to allow those resources to be released, and global state changes to be reverted. It's particularly important to have this functionality available in test suites, where many IGUI instances are being created and destroyed within a single process.

To allow polymorphic cleanup, it would be useful to add a close method to the IGUI interface itself, and the GUI base class. The base class implementation would do nothing.

This isn't without backwards compatibility risks: as always, an interface is a point of rigidity in a system, and in particular adding a close method would invalidate any existing implementations of IGUI that don't already inherit from theGUI base class. However, I'm not aware of any such implementations in practice, and I think the risk here is manageable.

On details: we'd need to decide whether it's also useful to have an is_closed property. The minimal useful interface would be just the close method, together with a guarantee that close is idempotent, so that it's always safe to call close even if you're not sure whether the IGUI object is already closed.

@corranwebster
Copy link
Contributor

corranwebster commented Feb 27, 2023

This seems reasonable, although it may be worth matching the create/destroy naming of the IWidget interface for consistency, although that would be a more invasive change to shift any initialization code out of __init__ and into a create method, so I'm not 100% sure of this. However the fact that this is to deal with global state makes me think that a side-effect free __init__ may be beneficial.

In terms of semantics, what should be the behaviour after close() is called? Which methods, if any, should be expected to raise?

In terms of an is_closed attribute, I'm not sure that I would expose anything publicly, and I suspect any internal use may be able to be handled by things like setting application objects or other global state to None and using that as the flag.

@mdickinson
Copy link
Member Author

In terms of semantics, what should be the behaviour after close() is called? Which methods, if any, should be expected to raise?

Most of them, I'd expect - the gui object would be unusable after closing. In the case of interest, we have an associated asyncio event loop and the GUI subclass close method closes that event loop, after which it's not useful.

@corranwebster
Copy link
Contributor

An idea that's been percolating for a few days around this: if we add start/stop methods then they could explicitly set Traits "ui" dispatch (or a better architected replacement) to use their call_later on start and then restore it on stop.

Probably can't do it with the current GUI object, unfortunately, as the usage is too chaotic (because they explicitly aren't singletons but give access to the QApplication singleton, we tend to create them and throw them away or even just use the class); we might need to be a new IEventLoop or something which the GUI objects reference to do their stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants