I went in to tabopen feeling a bit suspicious that the positioning code
was in some way duplicating `_get_new_tab_idx` in the parent class, or
if this code could be moved out to a common helper method or called by a
signal.
When a new tab is opened we need to position it in the tree structure.
For related and sibling tabs we need to know the tab that the new one is
opened "from". The tab related code deals with three new "new_position"
settings that the parent method doesn't know about.
For pulling it out behind a signal, we have the exiting `new_tab`
signal, but that doesn't say what the previous tab was. We could maybe
use the `TabbedBrowser._now_focused` or `TabbedBrowser.tab_dequeue` for
that, but that seems like it might be unfairly adding some new
responsibility onto those attributes? Even then we still would have no
way to know whether the user had requested a `related` or `sibling` tab.
So it seems all the code around tab positioning left here is specific to
tree tabs, assuming all the new "new_position" settings are needed, and
it's integrating with the existing code in a fine way, assuming we are
aiming for keeping the new code in subclasses. But that all begs the
question of how would an extension proper do it? I think it would do it
much the same way, but instead of subclassing TabbedBrowser it would
define a new :open command and ask uses to switch to that (or replace it
if we allow that). Then that new command would call TabbedBrowser to get
a tab and then do it's own extra stuff. And hook into the `new_tab`
signal to handle a new one being created via some other means. Although
that would not let if know whether a tab was opened as related or not
(eg created from clicking a link vs loading a session). So maybe
something to work on there for the extension API.
tabopen:
* add comment explaining necessity of the method
* add some early exits - I think this is fairly important to limit the
number of possible logic paths maintainers have to keep in their heads
when working with the more logic heavy code down below
* didn't add an early exit for `idx is not None` because I'm not 100%
sure when that is set and I'm not confident enough in our test
coverage to change it right now
position_tab:
* change to work with just Nodes instead of tabs - makes testing easier
* change the initial duplicate avoidance code to be more clear to me,
probably it would be even more clear is `parent` was called
`new_parent`
test_treetabbedbrowser
* add some tests for position_tab - not too happy with the mocking,
should probably see how it looks with a proper tab, they just feel a
bit heavy weight with the amount of mocking they bring with them
7a0f023a02