Extract widget enable/dislable logic from StatusBar

[pylbrecht]

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:

1. Some sort of factory to create widgets from config.statusbar.widgets (here StatusBarWidget.from_config()) encapsulating some special setup logic (e.g. setText() for text: widgets)
2. Handling the enable / disable logic (which can vary from widget to widget) by implementing enable() and disable() methods

I am a fan of ABCs to enforce interfaces. However, in this case this leads to repeating ourselves quite a bit with self.show() and self.hide().

---

Relates to #7407

[toofar]

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 ~a bit~ very rambly and lecture-y but I'm posting as-is because I want to sleep.

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 NotImplemented errors for mandatory ones. Although in saying that I'm mostly going off what I'm familiar with. If you want to make an argument based on what things one approach makes easier vs hard I would be interested in hearing it and I think that would help with a more nuanced discussion.

The multi-inheritance probably stands out a bit much here because you've got it in places you don't need it? For example TabIndex inherits from StatusBarWidget and TextBase but TextBase already inherits from StatusBarWidget. Not sure if there is something ABC specific that makes that necessary or if that's just a rough edge.

A couple of general pieces of advice before I attempt to discuss some details:

1. focus on the biggest wins, and one at a time. Mostly for your own sanity, if there is a few things going on in the PR and the PR is maybe losing focus because of it ruthlessly simplify the non-essential changes to make sure the focus stays on the biggest wins.
2. call out and keep an eye on the things that are hard. Similar to the above, the things that don't fit or you have to work hard at finding solutions to could end up being the fulcrum of a more elegant solution if you figure out how to deal with them.
3. clarify the scope of the PR, assuming you have an end goal in mind. The linked issue initially refers to just enable/disable methods. But then we put a bunch of more potential improvements on there. I, in particular, have aspirations for this bit of code, but if that's not where you are heading it's best to let me know so that I don't harass you with weird suggestions. (I don't like that the bar class has item specific behavior and I want to lay the foundation for adding status bar widget registration into the extension API).

Now some actual stuff from the PR:

• pulling out a factory: +1, definitely think we can get some value out of this. It needs a bit of work though, so if you want to stick with this there are some opportunities for refinement. Like something to replace the if/else block, and pushing per-widget setup stuff into the widgets.
• you've got a try/catch around the call to widget.on_tab_changed(). I think this calls out something I hadn't really though about when I was thinking to give all the widgets a common base class. How to handle some widgets caring about this hook and others not? Make it an empty stub method on the base class? Is that discoverable and foolproof enough for implementers? Or maybe widgets could register for hook functions like extensions do?
• related to that, you've called out that some widgets need different handling of the enable and disable methods (eg self.hide() vs self.enabled = False vs self.hide(); self.clear()) and I accept that, although I don't really understand why. Assuming there is necessary differences between widgets, if we had one common base class that all items inherited from and only implemented the methods they need, would that be good enough? Or would we need a tree of abstract objects or mixins for items to implement to avoid a big base class which a bunch of conditionals or duplicated code between children or an error prone implementation where people forgot to implement abstract methods all the time? idk, I'm not sure I'm a fan of the idea of a whole bunch of semi-specialised mixins, feels like I would need to write up a table of all the methods that the bar class calls on items and which items implement which method to decide though.
• currently the widgets are being initialized once in the bar constructor. If you change statusbar.widgets it looks like that wont take effect

[pylbrecht]

First of all thank you so much for your elaborate feedback. Very valuable! I will address it in a chronological order; easier for my post-workday brain.

---

@The-Compiler

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.

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.

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.

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).

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.

I did! First I thought the composition approach with .widget was great. Then I thought "if we have a StatusBarItem subclass for each widget type, why not move the logic into the widget itself and cut the middleman?". Now I see value in separating the enable/disable logic from the widget iitself by having it in the StatusBarItem classes; delegating to the widget where necessary. Makes for clearer responsibilities. I will sketch this approach next and see how it goes.

---

@toofar

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?

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 TypeError is raised).

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 NotImplemented errors for mandatory ones. Although in saying that I'm mostly going off what I'm familiar with. If you want to make an argument based on what things one approach makes easier vs hard I would be interested in hearing it and I think that would help with a more nuanced discussion.

I agree that consistency with existing patterns is key. No argument there!

The multi-inheritance probably stands out a bit much here because you've got it in places you don't need it? For example TabIndex inherits from StatusBarWidget and TextBase but TextBase already inherits from StatusBarWidget. Not sure if there is something ABC specific that makes that necessary or if that's just a rough edge.

It's a rough edge. I left it rough on purpose, but did not communicate that properly. Sorry about that.

A couple of general pieces of advice before I attempt to discuss some details:

1. focus on the biggest wins, and one at a time. Mostly for your own sanity, if there is a few things going on in the PR and the PR is maybe losing focus because of it ruthlessly simplify the non-essential changes to make sure the focus stays on the biggest wins.
2. call out and keep an eye on the things that are hard. Similar to the above, the things that don't fit or you have to work hard at finding solutions to could end up being the fulcrum of a more elegant solution if you figure out how to deal with them.
3. clarify the scope of the PR, assuming you have an end goal in mind. The linked issue initially refers to just enable/disable methods. But then we put a bunch of more potential improvements on there. I, in particular, have aspirations for this bit of code, but if that's not where you are heading it's best to let me know so that I don't harass you with weird suggestions. (I don't like that the bar class has item specific behavior and I want to lay the foundation for adding status bar widget registration into the extension API).

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.

Now some actual stuff from the PR:

pulling out a factory: +1, definitely think we can get some value out of this. It needs a bit of work though, so if you want to stick with this there are some opportunities for refinement. Like something to replace the if/else block, and pushing per-widget setup stuff into the widgets.

Agree that it needs more work. I left it drafty on purpose to get some early feedback (however did not communicate that properly).

you've got a try/catch around the call to widget.on_tab_changed(). I think this calls out something I hadn't really though about when I was thinking to give all the widgets a common base class. How to handle some widgets caring about this hook and others not? Make it an empty stub method on the base class? Is that discoverable and foolproof enough for implementers? Or maybe widgets could register for hook functions like extensions do?

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.

related to that, you've called out that some widgets need different handling of the enable and disable methods (eg self.hide() vs self.enabled = False vs self.hide(); self.clear()) and I accept that, although I don't really understand why.

The main difference is that some widgets only show/hide on tab change and some don't.

Assuming there is necessary differences between widgets, if we had one common base class that all items inherited from and only implemented the methods they need, would that be good enough? Or would we need a tree of abstract objects or mixins for items to implement to avoid a big base class which a bunch of conditionals or duplicated code between children or an error prone implementation where people forgot to implement abstract methods all the time? idk, I'm not sure I'm a fan of the idea of a whole bunch of semi-specialised mixins, feels like I would need to write up a table of all the methods that the bar class calls on items and which items implement which method to decide though.

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.

currently the widgets are being initialized once in the bar constructor. If you change statusbar.widgets it looks like that wont take effect

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!

[pylbrecht]

I finally got around to look into this again and only now did I properly understand @The-Compiler's proposal in https://github.com/qutebrowser/qutebrowser/issues/7407#issue-1382023382:

Make widgets StatusbarItem subclasses, and have the widget as .widget instead of inheriting from it directly - somewhat similar to what we did for TabbedBrowser in https://github.com/qutebrowser/qutebrowser/commit/e169e2165da7892c0b9731a94481aebbc867df26 essentially. Seems like the cleanest option to me.

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).

[pylbrecht]

@The-Compiler, following your approach

Make widgets StatusbarItem subclasses, and have the widget as .widget instead of inheriting from it directly - somewhat similar to what we did for TabbedBrowser in https://github.com/qutebrowser/qutebrowser/commit/e169e2165da7892c0b9731a94481aebbc867df26 essentially.

I am wondering where to draw the line between widget and StatusBarItem. Let's take Percentage as an example: https://github.com/qutebrowser/qutebrowser/blob/ab1b444d37954774240df254a4c6065bdfae9551/qutebrowser/mainwindow/statusbar/percentage.py#L14

Would textbase.TextBase be living in .widget, or QLabel (textbase.TextBase is subclassing QLabel)?

[toofar]

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 self.set_text() it means they have an easier time of it.

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 pyreverse -o png -p statusbar qutebrowser/mainwindow/statusbar/ shows there is a few more tricky methods in use like paintEvent(), resizeEvent(), sizeHint() and minimumSizeHint() that we might have to think about how we can handle them in this situation. Do we even need to implement them? Can we handle them via events instead of inheritance? (I suspect not.) Will those status bar items need to implement their own QWidget subclasses anyway so they can implement them (but as a helper class instead of implementing it themselves).

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 resizeEvent(), sizeHint() etc without inheriting from QWidget directly (I'm assuming having a helper class which inherited from the widgets instead, but idk).

Inspiration :)


[pylbrecht]

@toofar, thanks for your feedback!

FYI: I opened #8240 to play with the composition approach.