-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Extract widget enable/dislable logic from StatusBar #8203
base: main
Are you sure you want to change the base?
Conversation
Currently we only handle the widgets with special setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of the approach. It involves multiple inheritance and custom metaclasses... all in all, this will make it very difficult to understand what's going on behind the scenes for someone who isn't familiar with Python on a quite deep level.
Maybe it'd be a compromise to use @abc.abstractmethod
but without the metaclass shenanigans? That won't give us runtime validation, but I don't mind much: Both pylint as well as mypy check this kind of thing statically even if the metaclass isn't used. I believe pylint even does so if you name your class AbstractStatusBarWidget
or somesuch, and do raise NotImplementedError
in the abstract methods.
But even then, I try to avoid multiple inheritance when I can. Have you seen the list of approaches I had in #7407? I suspect the .widget
one would work quite nicely? Composition over inheritance is often nice IMHO.
Firstly, my own disclaimer. I'm more in a writing mood than a reading one. So I haven't fully groked your PR yet and am probably making some assumptions. Also this is getting I gotta say I'm also not a huge fan of this ABC + multi-inheritance pattern. Mostly because I'm unfamiliar with it, I've seen it around but have never had to use it. I'm not sure what the value of this meta stuff is? It would indeed be nice to see something that uses a more familiar pattern for this codebase like normal classes (which are abstract) with stub methods for common ones with The multi-inheritance probably stands out a bit much here because you've got it in places you don't need it? For example A couple of general pieces of advice before I attempt to discuss some details:
Now some actual stuff from the PR:
|
First of all thank you so much for your elaborate feedback. Very valuable!
Yeah, admittedly I got carried away with the ABC stuff. Thank you for broadening my horizon on the fact that his code needs to address a wide audience with different abilities. I suppose I suffer from tunnel vision from working in a completely different environment.
I used an ABC mainly for the runtime validation. But now that argument does not feel convincingly strong when weighed against the downsides (e.g. custom metaclass complexity).
I did! First I thought the composition approach with
I like to use an ABC with only one level of inheritance: the ABC defines the interface, one subclass for each implementation. My selling point (so far) was the runtime validation (e.g. a class derived from ABC cannot be instantiated unless all of its abstract methods and properties are overridden; IIRC a
I agree that consistency with existing patterns is key. No argument there!
It's a rough edge. I left it rough on purpose, but did not communicate that properly. Sorry about that.
Thank you very much for the advice! I am still struggling to communicate effectively and I feel it makes collaboration not as smooth as it could be.
Agree that it needs more work. I left it drafty on purpose to get some early feedback (however did not communicate that properly).
Yeah, I am also not happy with that solution. It's more of an EAFP hack than a clean solution. I will tinker with it when blueprinting the compositional approach.
The main difference is that some widgets only show/hide on tab change and some don't.
I don't know. Maybe a common base class is enough, maybe not. I have (and want) to play a bit more with the code to understand the underlying problem better. Using multiple mixins sure doesn't sound that appealing.
Thanks. I didn't pay much attention to that part, but I could definitely see how that would have bit me later on. Next steps: I will sketch the compositional approach taking your feedback into account. Let's see if I can come up with something cleaner this time! |
I finally got around to look into this again and only now did I properly understand @The-Compiler's proposal in #7407 (comment):
That refactor turns out to be a bit more meaty than I anticipated. I will think about a strategy how to safely make the changes (e.g. by writing a few characterization tests). |
@The-Compiler, following your approach
I am wondering where to draw the line between widget and StatusBarItem.
Would |
For the ones that inherit from TextBase I would vote that they keep inheriting from TextBase, but ideally don't talk to its QLabel widget directly. (Although it's not the end of the world if they do.) I think every status bar item needs to have some kind of QWidget underneath, so they can draw stuff. But it's convenient to have a common base. For example if all the ones that just want to draw text can just inherit from TextBase and use So we've got something like a top level StatusBarWidget with QWidget level abstract methods then child classes like TextBase that implement that and present a simpler interface for for their children. Looking at what idk, this talk about helper classes seems like its getting a bit complicated. If you are feeling stuck and are looking for a way forward from the current state I would clean up the places where you have children of TextBase inheriting both from their parent and grandparent and where they are overriding .enable()/.disable() where they don't need to (in fact them not having to do that themselves is the point of this PR!). See below for inspiration ;) And then probably look at how you could make TextBase and Progress do whatever they need to in |
Disclaimer
I haven't tested any of these changes (despite the ones covered by the added unit tests).
They are very drafty and shall only serve the purpose to gather some feedback before going all-in with this approach.
Watch out for rough edges!
Essentially this refactoring consists of two parts:
config.statusbar.widgets
(hereStatusBarWidget.from_config()
) encapsulating some special setup logic (e.g.setText()
fortext:
widgets)enable()
anddisable()
methodsI am a fan of ABCs to enforce interfaces. However, in this case this leads to repeating ourselves quite a bit with
self.show()
andself.hide()
.Relates to #7407