-
Notifications
You must be signed in to change notification settings - Fork 166
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
Make task expanding more efficent #741
Make task expanding more efficent #741
Conversation
Code LGTM, I could never reproduce the bug though so @nekohayo is better suited to test |
Try switching to the closed tab view, then restart GTG (with nekos dataset), click on the + top left, then click "Open Parent" on the newly opened task. |
Previously, when expanding a task (either by the user or by liblarch itself, which itself tries to remember collapsed nodes and expand other ones), GTG would for some reason visit all nodes again just to check each node again, and expand nodes that should've been expanded. This additionally has the problem when creating a new parent task but you haven't visited the 'open' tab yet, then all nodes are collapsed (which is the default of Gtk.TreeView). When adding a new parent task, liblarch sees that a new first child node has been inserted into the tree (probably used by TreeView to make them easier to whenever to display the expand arrow thing), and since liblarch expands all nodes by default, it expands it. However, now the old handler then goes on and expands all tasks, but each collapsed task it finds it'll call itself again. So for every collapsed task (or task with children), one recursion level occurs. If you have a lot of (sub)tasks such as nekohayo, this can cause the stack to be exhausted: RecursionError: maximum recursion depth exceeded Fatal Python error: Cannot recover from stack overflow. Because of this and the general inefficiency, we now just expand the direct child nodes. Since I expect the task depth to be rather small, I'd expect it not to run into the recursion depth error easily. Also, for some reason the flat work view also has the handler attached, yet the handler explicitly expands nodes from the 'active' tree. For simplicity, I just removed the signal connection of the work tree. Note that a refilter operation on the 'active' task tree causes expand handler be called for each node while it builds the tree, but it doesn't have the recursion issue because when it gets to the hundreds of tasks in the tree, everything has already been expanded already by the previous calls.
075b7ee
to
0487655
Compare
Force-pushed since I accidentally had a variable |
Tested and works, thanks! |
Thank you for your fabulous work, Neui, I really appreciate the effort you took in investigating and fixing this! I was able to confirm the fix works as advertised. For the record, if anyone out there is looking at this and wants to know how to trigger/reproduce the issue, some instructions are in #595 (comment) |
Previously, when expanding a task (either by the user or by liblarch
itself, which itself tries to remember collapsed nodes and expand other
ones), GTG would for some reason visit all nodes again just to check
each node again, and expand nodes that should've been expanded.
This additionally has the problem when creating a new parent task but
you haven't visited the 'open' tab yet, then all nodes are collapsed
(which is the default of Gtk.TreeView). When adding a new parent task,
liblarch sees that a new first child node has been inserted into the
tree (probably used by TreeView to make them easier to whenever to
display the expand arrow thing), and since liblarch expands all nodes by
default, it expands it.
However, now the old handler then goes on and expands all tasks, but
each collapsed task it finds it'll call itself again. So for every
collapsed task (or task with children), one recursion level occurs.
If you have a lot of (sub)tasks such as nekohayo, this can cause the
stack to be exhausted:
Because of this and the general inefficiency, we now just expand the
direct child nodes. Since I expect the task depth to be rather small,
I'd expect it not to run into the recursion depth error easily.
Also, for some reason the flat work view also has the handler attached,
yet the handler explicitly expands nodes from the 'active' tree.
For simplicity, I just removed the signal connection of the work tree.
Note that a refilter operation on the 'active' task tree causes expand
handler be called for each node while it builds the tree, but it doesn't
have the recursion issue because when it gets to the hundreds of tasks
in the tree, everything has already been expanded already by the
previous calls.
Testing appreciated.
Fixes #595 and fixes #724 (which is basically the same issue, but seems to add stuff when removing things? Haven't really looked into that yet...)