refactor: reduce duplication in `TreeTabbedBrowser`

[toofar]

refactor: reduce duplication in `TreeTabbedBrowser`

[pinusc]

I have been looking into TreeTabbedBrowser and TreeTabWidget, and while there is plenty of room for improvement in the way the parent and subclass interact with each other... I could not find any actual code duplication. Maybe I missed something? Could you point to specific examples? I did find two TODO comments saying to remove duplication, but they are actually obsolete: the relevant bits were already rewritten to not duplicate code.

Aside from that, both tabopen and undo in TreeTabbedBrowser feel quite hacky, and small changes in the parent class could easily break the current code.

[toofar]

I won't have time to do a more detailed writeup until Sunday but from what I remember it wasn't so much about duplication within TreeTabbedBrowser or TreeTabWidget but between them and their parent classes. Here are a couple of examples from my notes relating to treetabbedbrowser, I hope they make sense to you!

• in __init__() the widget set by the parent class is replaced (but not deleted, so it ends up hanging around for the lifetime of the parent) and the work of connecting signals is done again. If we moved the widget instantiation in the parent into a _create_widget() method (a factory?) we could just override that instead of overriding the whole of __init__() and re-doing some work from the parent class. This is what I meant by pulling logic that differs out to a helper method.
• in _remove_tab() an undo entry is created and then the parent method is called with add_undo=False. This is kind of "white box" behaviour that relies on knowledge of the parent class and creates more coupling than is needed. Instead of doing that could we add an _add_undo_entry() to the parent class (like you've already added to the new class!) and override that? This is an example of the subclass working around the behaviour of the parent.

there is plenty of room for improvement in the way the parent and subclass interact with each other

Please take some time to share your ideas! I don't want to get too much into lecture mode because that can be quite off-putting, but this isn't a race where we are trying to check a bunch of boxes as fast as we can. We are trying to make this easier to maintain in the long run so we can keep the project healthy while adding features. I think it helps to put your reviewer hat on and look at the code with a critical eye. Instead of looking for places where the code is obviously wrong, look at places where the code is working fine and think of other ways it could be implemented that might be easier to work with for someone responsible for the code base as a whole.

Aside from that, both tabopen and undo in TreeTabbedBrowser feel quite hacky, and small changes in the parent class could easily break the current code.

Yeah, I think I can see that. Part of the reason for the attributes on the project board is that I don't want to get distracted by polishing absolutely everything, and I don't have these pieces in my head at the moment so I can't weigh in on how much they do or don't need changing. But I think every opportunity to align the child and parent classes could be valuable. I do have a couple of points from my notes on them if you want some backseating:

spoilers: A couple of half baked ideas for improvements to undo and tabopen, don't read if you don't want to be told what to do.

• tabopen: should we be overriding _get_new_tab_idx() for some of all of this instead?
• undo: could we remove some duplication by teaching the undo entry how to restore itself?

Anyway, I'm really glad to see you are taking the time to survey what could be changed before jumping in. I think it can really help to plan out what could be covered by an issue so we can avoid it dragging out in periodic bursts of attention.

[toofar]

Hi @pinusc, I should have a bit more time than usual to put into this project over the next week so let me know if you want to continue with this ticket, and if you want to discuss any potential "duplication" (possibly not the clearest term) in depth.

[toofar]

Oops, forgot to close when I merged the PR. See #8122 for commentary.