| Age | Commit message (Collapse) | Author |
|---|
|
|
|
Various lint fixes. The ones we will likely revisit later are:
* unused sibling arg in parent TabbedBrowser class
* seperate-but-similar UndoEntry implementations
* variables with two leading underscores in notree
* `_tree_tab_give()` implementation
qutebrowser/browser/commands.py:499:9: F841 local variable 'oldroot' is assigned to but never used
In `_tree_tab_give()` there is a check to see if a node being moved needs to
be re parented. That used to use `is not oldroot` and I changed it in
c2bc549605a3c to be `parent.uid in uid_map`, eg is this a child node of the
tree being moved. Now we don't need oldroot anymore but I forgot to clean it
up.
PS: not sure why that check is so complicated. Won't it only ever be false for
the top node being moved? So we can just say `if node !=
current_widget.node:`? I guess that's irrelevant if that's all going to be
re-written anyhow.
qutebrowser/mainwindow/treetabbedbrowser.py:83:1: D202 No blank lines allowed after function docstring
qutebrowser/mainwindow/treetabwidget.py:77:22: C419 Unnecessary list comprehension passed to all() prevents short-circuiting - rewrite as a generator.
qutebrowser/mainwindow/treetabwidget.py:78:13: C1804: "self.widget(idx).url().toString() == ''" can be simplified to "not self.widget(idx).url().toString()", if it is striclty a string, as an empty string is falsey (use-implicit-booleaness-not-comparison-to-string)
qutebrowser/mainwindow/treetabwidget.py:132:16: R5501: Consider using "elif" instead of "else" then "if" to remove one indentation level (else-if-used)
Ran out of energy at the double negative. Hope that's okay.
qutebrowser/mainwindow/treetabbedbrowser.py:131:35: W0613: Unused argument 'idx' (unused-argument)
Add an unused-argument ignore line because, while we aren't using index (tab
undo entries remember their parents instead of their index) I think we'll be
pulling this method up to the parent class later which does use it.
qutebrowser/mainwindow/treetabbedbrowser.py:198:4: W0237: Parameter 'idx' has been renamed to 'sibling' in overriding 'TreeTabbedBrowser.tabopen' method (arguments-renamed)
Urghgh, I have no idea why this would be like this. I looked through the
codebase both for `sibling` and `tabopen` and didn't see this being called
with positional arguments beyond the url. So seems safe to change it.
Probably some linter will be complaining that it still doesn't match the
signature of the parent...
qutebrowser/mainwindow/treetabbedbrowser.py:19:0: W0611: Unused log imported from qutebrowser.utils (unused-import)
qutebrowser/mainwindow/treetabbedbrowser.py:214:0: I0021: Useless suppression of 'arguments-differ' (useless-suppression)
And I guess this is the complaint about the arguments differing. But it says we
don't need the suppression anymore? We'll, I'll add it to the parent and see
what that does.
qutebrowser/misc/notree.py:60:4: C0103: Class constant name "PRE" doesn't conform to snake_case naming style (invalid-name)
qutebrowser/misc/notree.py:61:4: C0103: Class constant name "POST" doesn't conform to snake_case naming style (invalid-name)
qutebrowser/misc/notree.py:62:4: C0103: Class constant name "POST_R" doesn't conform to snake_case naming style (invalid-name)
I assume these are copied from anytree. Not a huge fan, why are the variables
names so much less readable than the values?
qutebrowser/misc/notree.py:221:16: R5501: Consider using "elif" instead of "else" then "if" to remove one indentation level (else-if-used)
qutebrowser/misc/notree.py:289:12: W0719: Raising too general exception: Exception (broad-exception-raised)
`promote()` is only called from one place in commands.py currently. So we are
essentially validating config, so this error "shouldn't happen". If this was
going to be called from more places we should probably make an enum in notree
and maybe generate the config from that? Or validate it against the config (in
a test?) or something.
qutebrowser/misc/notree.py:256:4: W0238: Unused private member `Node.__add_child(self, node: 'Node[T]')` (unused-private-member)
qutebrowser/misc/notree.py:260:4: W0238: Unused private member `Node.__disown(self, value: 'Node[T]')` (unused-private-member)
qutebrowser/misc/notree.py:190:12: W0238: Unused private member `Node.__modified` (unused-private-member)
I assume these are copied from anytree. The internet says variables with two
leading underscores are mangled by the interpreter to be
`__classname__variable` so that you don't have to worry about child and parent
classes fighting over variable names.
We don't have any examples of this pattern in the codebase currently and this
Node class seems designed for composition, not inheritance. So I think I would
prefer to change them to have a single underscore. That seems like something
that people might want to discuss though, so for now silence pylint as it's
presumably comparing some mangled vs unmangled name (eg if I rename them it's
happy).
qutebrowser/misc/notree.py:68:0: I0021: Useless suppression of 'invalid-name' (useless-suppression)
|
|
In 8aa88b83e I added a dependency in the TabWidget in
`parent.is_shutting_down` (assuming parent is a TabbedBrowser, but
without updating the type hints...).
Since the benchmark test wasn't setting a parent it's been breaking
since then, oops.
That's going to be breaking all the branches based off of the tree tabs
branch, so we should probably cherry pick this back to the
tree-tabs-integration branch. And consider updating the
TabWidget parent kwarg type hint (and make the dummpy parent a
mock/spy/double of the TabbedBrowser I guess).
Not sure where the size of 640x480 came from before hand but when I
added a parent it went to 100x30.
|
|
|
|
|
|
|
|
The command was renamed a while back.
|
|
|
|
- Don't split the pattern in weird places used for str.join, instead
build up the individual groups and then "".join them, which seems more
readable to me.
- Don't reuse "val" for both the user-input pattern and the regex pattern.
|
|
With the change in #7955, if the regex did not match, it ended up retrying the
lookahead at every position of the string (expected), but then *also* repeated
that process for every position in the string. Thus, a non-matching pattern
ended up in O(n^2) performance (with n = length of URL).
Instead, anchor the pattern at the beginning of the string. This doesn't change
behaviour as we use .* at the beginning of every lookahead anyways, but it means
we end up with O(n) instead of O(n^2) performance.
Co-authored-by: toofar <toofar@spalge.com>
|
|
This reverts commit 6a149901fbd91ac2b8b6cb39fbc835a777319c2e.
|
|
This reverts commit 45d483479aebb6af5a53aea97d65228d8621ad86.
|
|
Typo fix
|
|
Trivial typo fix
|
|
This reverts commit dbd0bfbe8e19b173e39b1536a52907eb28557773.
See previous commit
|
|
This reverts commit fe1faa14b91253db99e9b4451ffb1a479feff1db, reversing
changes made to 04af4c657d3725fd2d3b556d9b88b44d3ada0be1.
For my setup with 310 quickmarks and 94 bookmarks, this makes qutebrowser hang
for around a minute on every keypress.
|
|
https://docs.pytest.org/en/7.4.x/deprecations.html#py-path-local-arguments-for-hooks-replaced-with-pathlib-path
https://docs.pytest.org/en/7.4.x/reference/reference.html#std-hook-pytest_ignore_collect
8.0.0 removes the "path" compatibility alias we used.
|
|
As shown in detail by toofar in dff25d10bcc8a398a77ef260c33e51ec5c70f482,
mesa 23.3 seems to trigger some new error messages.
On my setup locally, it also triggers the warning that's ignored here.
Why that's not the case on CI or for others is unclear to me, I gave up
trying to understand graphics stuff on Linux.
I'll just presume this is the same underlying cause, and refer to the
commit message of the commit above for more details.
|
|
Update dependencies
|
|
|
|
TODO:
* convert to asciidoc or make scripts/asciidoc2html.py support markdown
input too.
* for now I'm using `pandoc -f markdown -t html -o qutebrowser/html/doc/treetabs.html doc/treetabs.md`
* should be able to asciidoc with pandoc too
* the last section and the rest of the second to last
* review and refactor
* post on the PR and figure out how we can contribute to it collectively
(put on main branch so people can open PRs? create new integration
repo where people can open issues, discussions and PRs?)
* maybe add more examples showing the effect of various operations
* (when including in a release) consider removing the implementation
section, or refactoring it to be more high level broad strokes and
less low level stuff that will inevitably drift from the actual
implementation
These docs are not supposed to be in their final state. My plan for them
so far is to provide a guide to the changes for PR contributors. I think
it's currently difficult to hold the scope of the changes in your head
the description on the PR is currently very limited.
I would like to encourage discussion of usability and feature set as
well as highlighting some implementation details to point out upsides,
downsides, implications and alternatives.
I've modified asciidoc2html to copy all images over. It was only doing
specific ones before. But there are more images in there, why are they
in doc/img if they aren't images used in the documentation? Anyway,
storage is cheap, none of the images are sensitive and not having to
hardcode individual files names is good.
|
|
I was looking through some commit messages from last week and noticed I
was having trouble keeping track of what these traverse orders meant. So
I tried to adjust the descriptions to be more clear. Hopefully it
worked?
Probably could still use a visual aid though, or the enum value names
could even be changed to be more clear, like "parent-first",
"child-first", "child-first-reverse/-right".
The test I used to verify this description was:
simple_tree = Node('parent')
left = Node('left', simple_tree)
Node('grandleft-left', left)
Node('grandleft-right', left)
Node('middle', simple_tree)
right = Node('right', simple_tree)
Node('grandright', right)
# parent
# ├─ left
# │ ├─ grandleft-left
# │ └─ grandleft-right
# ├─ middle
# └─ right
# └─ grandright
@pytest.mark.parametrize('order, expected', [
(TraverseOrder.PRE, "parent left grandleft-left grandleft-right middle right grandright",),
(TraverseOrder.POST, "grandleft-left grandleft-right left middle grandright right parent",),
(TraverseOrder.POST_R, "grandright right middle grandleft-right grandleft-left left parent",),
])
def test_simple(order, expected):
assert expected == " ".join(
[n.value for n in simple_tree.traverse(order)]
)
|
|
When recursive closing a tree group you will be prompted if you want to
close any tabs that are pinned. Except we passed the topmost tab to the
prompt method every time instead of the tab we were going to close.
This fixes that. It does mean that if there are multiple pinned tabs in
a tree it gives you an identical prompt for each one, which no
indication that they are separate prompts. It looks like nothing is
happening when you answer it. But that's a separate problem (probably
the solution is to either prompt once or to put URLs and/or tab IDs in
the prompt).
|
|
Seems a step from the existing code path wasn't applied to the
new tree-specific code. I've pulled it out to be applied to both paths.
Previously if you ran gD (tab-give without a positional argument) it
would open a new window but you couldn't see it!
|
|
We shouldn't really be closing tabs in multiple places[^1], but while we
are, lets not promulgate copypasta we don't need.
layout.unwrap() was previously called as a workaround for some upstream
behaviour in Qt < 5.12. That workaround was removed in c067a96f79fd56e9
[^1]: I want to make it so nodes know how to close tabs, that we we only have to worry about
either widget stuff or tree stuff, not both
|
|
The root node never is associated with a tab.
|
|
I think when removing nodes it's less work to remove leaves first, so that
children don't have to be reparented. We also don't need to save everything to
a list. I don't know what the difference out of POST and POST_R, maybe if you
remove the last sibling first it's less work? Not sure.
Also take care of a fixme comment which is fixed (the comment it refers to is
still there, but add_undo_entry is no longer modifying the tree).
|
|
Hopefully it saves someone else from having to interpret it in the
future.
Also change the last line into an assert because is should be redundant.
This method seems more complex than it should be. Should, or is, some of
this functionality be available in the notree data structure?
|
|
Two issues here:
1. transplanted trees could get rendered upside down depending on the value of the
tab.new_position.tree.new_child setting, I think. The parent and child
attributes of the nodes looked correct but it was being rendered upside
down in the tab bar, weird. Anyway, we are adding new tabs at the top level
and them setting their parents based on the mapping. We shouldn't be
opening them as related to the currently focused tab
2. moving a subtree was crashing because the parent of the top node wasn't in
the mapping. Which is fine because that's not being moved over. I changed
it to not explicitly change the parent of such nodes (so they remain under
the tree root).
Two other tidy ups:
* the initialization of "uid_map" as it didn't seem to need the 1:1 in there.
* remove tabs explicitly in reverse order rather than repeatedly
removing the current tab. This avoids re-work in the tree because it
does a bunch of stuff to re-parent children if you delete the parent.
Also I have debug logging in notree currently warning of children being
"orphaned" that was going off
TODO items around tab moving:
* collapsed state is lost
* moved tab isn't focused in destination window (I think that's a thing that
happens normally? Could be wrong)
* this method might be a bit simpler and consistent with other methods
(like undo) if it did more stuff in one go using traverse order to
keep things consistent, instead of the map. It seems to work fine as
is but not re-using patterns makes it harder to see opportunities for
refactoring
The structure I was testing with was:
1. one
2. two
3. three
4. four
5. five
1. six
Then focusing "three" and `:tab-give -r 1`.
|
|
Little bit of tidy up. This was already being used in the conditional branch
for collapsed nodes, I have no idea why it wasn't being used for the
un-collapsed case.
|
|
When a collapsed tab group was being deleted the nodes were being removed
from the tree by a sneaky line in `_add_undo_entry()`, which made it difficult
to reason about the code. The nodes are removed from the tree by setting
`node.parent = None`, the setting goes and calls `parent.disown()`, which is a
bit magical but that's a job for another day.
This commit moves the line where the node is removed from the tree to be up into
the same block as where the tab is deleted and adjusts the undo entry to
remove the child uids from it instead. The problem there is that
`UndoEntry.from_node()` iterates over a nodes children, previously
children were being removed from the tree first so they weren't showing
up here. We don't want children uids in the undo nodes because the
restore code will look for an existing child to re-attach when
re-creating a node. But tabs are restored parent first. And since we do
have a parent uid in the undo entry that is enough to restore the full
structure.
|
|
This resolves an issue in update_tab_titles() when deleting a tab with
collapsed children. Since we were doing all of the super() action before
removing the node from the tree the super() method was calling
on_current_changed() before we had done our bit of updating the tree.
update_tab_titles() was getting called from that which was calling into
get_tab_fields() in TreeTabWidget which was complaining that the tree and
widget didn't match up.
We don't have any automated tests covering this behaviour currently but moving
the super() call to the end works so far. Possibly it was that way due to some
earlier refactoring.
I've changed a local variable name since it was shadowing a method arg that is
now used further down.
TODO:
* are there other pieces of code where we should look whether we are doing
tree stuff or widget stuff first and enshrine that as a pattern?
* the note about `_add_undo_entry()` points to unfortunate code that we should
look into. It doesn't seem correct to be manipulating the tree in that
method and the explanatory comment is too vague to explain anything
|
|
This is the third case where errors have been raised from update_tab_titles()
due to a mismatch between the tab widget and node tree:
1. on startup when the tabs are being added to the widget but the tree hasn't
grown yet - identified with heuristic
2. when hiding and showing tabs - changed logic to not fire updates
mid-operation
3. when shutting down tabs get removed from the widget before they are removed
from the tree - changed logic to not fire updates mid-operation
Now there is a fourth issue: loading a session with hidden tabs. In this case
tabs will be in the widget but not in the rendered tree, because that doesn't
include hidden nodes.
I'm using "all the URLs are empty" as a proxy for "a session is loading into
this window". That combined with there being a hidden tab in the tree should
hopefully be a good enough heuristic for this case. Although it would be good
to check with 100% certainty that a session was being loaded. Or just
having something to see if the current window was still being
initialized would do it, as sessions are always loaded into new windows.
I've refactored the logic in this method so that we are matching on exact tab
value, instead of having two lists and assuming the indexes are going to line
up. This way seems a bit more deliberate.
|
|
TreeTabWidget.get_tab_fields() was getting called with indexes that didn't
match what it had in its internal data structure on shutdown and was printing
errors. With this change tab titles no longer get updated during shutdown,
avoiding those errors.
Unfortunately we couldn't connect to the `shutting_down` signal on the parent
object because that is only emitted after the tabs are removed, and it seems
it needs to stay that way for the sake of the window close undo functionality.
So I plumbed that parent object through to a member variable, hopefully mypy
is happy with that. For some reason `self.parent()` in that later on function
was returning a MainWindow instead of a TabbedBrowser? Weird.
That does beg the question of why the nodes aren't getting removed from the
tree when they are getting removed from the widget. Should
`TreeTabbedBrowser._remove_tab()` be doing things in a different order? That
says "node will already be removed from tree", so where are they getting
removed? Or is that meaning widget instead of tree?
TODO: get more certainty on alternate logic fixes
|
|
When we are hiding or showing a tree group `render()` will reflect the
`collapsed` state right away. But while `update_tree_tab_visibility()` goes and
adds and removes the tabs from the tab widget one by one we get called on each
tab removal (via `tabRemoved()` -> `update_tab_title()` ->
`get_tab_fields()`). While each of those tab removals is happening there are
more tabs in the widget than are returned by `render()` which can cause issues
in this code due to the mismatch.
So add an attribute and context manager to avoid tab title updates while
`update_tree_tab_visibility()` is doing its things, since it goes ahead and
updates all the tab titles right afterwards anyway.
|
|
Previously there was a `tree_prefix is empty!` error being printed in a couple
of situations. 1) at startup 2) when hiding a tree group with multiple tabs in
it.
The main problem with the existing code is that it was catching a generic
IndexError without having a strong indication of what the actual cause of this
error was. We get called from `update_tab_titles()` in the base TabWidget with
a valid tab index. If we don't have information for that tab index in our tree
structure that could be an indication of a real error and we should go to
some effort to identify and prevent the cause.
This commit addresses cause 1 (at startup) by checking if the tree is empty
and returning early if so. Ideally we would have an even more specific
indication of being in startup (eg self.initialised == False) but we don't
have have a variable indicating a startup is in progress currently (we
do have one for shutting down!).
TODO: figure out when the tree is constructed vs when the tab widgets are
inserted and figure out if we can construct the tree earlier.
The second part of the commit identifies and documents the cause of 2
(collapsing a group) but leaves the error messages in for now because I want
to do a more proper fix that would let us continue to identify this
situation if we got here via another call path in the future.
This error was only being printed when hiding a group, not showing it,
because when showing a group there was more entries in the tree than in
the widget. So the function was still returning wrong data, it just
wasn't being verified at all.
The two remaining error messages are using a {foo=} f-string feature that I
don't think is available in all the python versions we support.
|
|
Update dependencies
|
|
|
|
Update dependencies
|
|
In `pylint_checkers/setup.py` we define the package name as `qute_pylint`. When
setuptools was creating the egg (or wheel?) it was sanitizing the package name
replacing all problematic chars, including underscore, with a hyphen (-). So
the output of `pip freeze`, among other things, have the name with a hyphen:
`qute-pylint`. This was fixed in setuptools so underscores are no longer
sanitized: https://github.com/pypa/setuptools/issues/2522
Change the regex to include both just in case someone is running it with the
old setuptools or something. Also because I'm not able to reproduce the
hyphen version of the name locally, not sure why. Maybe it depends on your
python or importlib version too?
|
|
|
|
Mesa upgraded from 23.2.1-2 to 23.3.1-1 a couple of days ago. Since then
there has been some non-fatal InvalidLine errors in the CI jobs (eg
https://github.com/qutebrowser/qutebrowser/actions/runs/7281982007/job/19843511920)
Based on https://gitlab.freedesktop.org/mesa/mesa/-/issues/10293 I'm
assuming these log messages don't actually indicate errors and the tests
pass when they are ignore.
Weirdly, I'm only seeing these errors related to the
`tests/end2end/test_invocations.py::test_misconfigured_user_dirs` test,
I'm not sure why. It doesn't look much different from the ones around
it.
Possibly we could ignore these lines just for that test, or maybe play
round with it more to find out why it is different. But since the lines
are non fatal I'll just ignore them everywhere. I'm a little worried
about that because they are quite generic, but at least they are still
logged when they are ignored.
They might change these error logs messages to be warning log messages
based on that issue. But it'll probably still fail the tests since we
match on all log lines?
The "always try zink" change was introduced in https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25640
It looks like there might be a `LIBGL_KOPPER_DISABLE` which may skip
this behaviour. Not I'm not sure, the commit message says:
> don't load non-sw zink without dri3 support
> this is going to be broken, so don't bother trying
> also add LIBGL_KOPPER_DRI2 so people can continue to footgun if they
> really really want to
I assume we aren't trying to run with non-sw zink but with non-zink sw?
idk
Maybe we should be setting force_software_rendering=software-opengl or
LIBGL_ALWAYS_SOFTWARE instead so that it never tries to use the vulkan
backed zink?
|
|
|
|
A few more completions will now match search terms in any order:
`:quickmark-*`, `:bookmark-*`, `:tab-take` and `:tab-select` (for the quick
and bookmark categories).
|
|
qutebrowser/dependabot/github_actions/actions/upload-artifact-4
build(deps): bump actions/upload-artifact from 3 to 4
|
|
qutebrowser/dependabot/github_actions/github/codeql-action-3
build(deps): bump github/codeql-action from 2 to 3
|
|
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 3 to 4.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](https://github.com/actions/upload-artifact/compare/v3...v4)
---
updated-dependencies:
- dependency-name: actions/upload-artifact
dependency-type: direct:production
update-type: version-update:semver-major
...
Signed-off-by: dependabot[bot] <support@github.com>
|
|
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](https://github.com/github/codeql-action/compare/v2...v3)
---
updated-dependencies:
- dependency-name: github/codeql-action
dependency-type: direct:production
update-type: version-update:semver-major
...
Signed-off-by: dependabot[bot] <support@github.com>
|
|
Update dependencies
|
|
|
