-
-
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
refactor: reduce duplication in TreeTabbedBrowser
#8078
Comments
I have been looking into Aside from that, both |
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!
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.
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.
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. |
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. |
Oops, forgot to close when I merged the PR. See #8122 for commentary. |
There are two new tree tab specific subclasses introduced: TreeTabbedBrowser and TreeTabWidget. Similar to the investigation on commands.py it would be good to keep any duplicated tree tab specific code paths to a minimum. Additionally having logic intermingled between the parent and sub class could be tricky.
I would like to see if we can make the jobs of these classes easier, and reduce their coupling, by pushing some logic up into the base classes.
Ideas:
The text was updated successfully, but these errors were encountered: