-
Notifications
You must be signed in to change notification settings - Fork 814
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
Fix missing Tree.NodeHighlighted message #3528
base: main
Are you sure you want to change the base?
Conversation
When the tree has no selection the cursor_line is -1, then clicking on the last node in the tree will check if the selection is different from the last selection, and do a check nodes[previous_cursor_line] != nodes[new_cursor_line] but if previous_cursor_line=-1 and new_cursor_line=len(nodes)-1 then this will fail and the message will not be emitted.
Any chance of a regression test? I'm not a maintainer of Textual, but I'm sure you'll be asked this when someone gets to reviewing this. Thanks for you PR! |
any change to review this @davep (or other maintainers) |
@robinvd We've got a lot going on so sometimes it can take us a wee while to get through PRs. Meanwhile though what Tom said is good advice: if you've identified a bug and this is a PR to fix it then a regression test would be very welcome. |
@davep ofc I understand!On 31 Oct 2023, at 09:22, Dave Pearson ***@***.***> wrote:
@robinvd We've got a lot going on so sometimes it can take us a wee while to get through PRs. Meanwhile though what Tom said is good advice: if you've identified a bug and this is a PR to fix it then a regression test would be very welcome.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Just to add too: having enabled the running of tests in CI: it looks like your change might be causing some existing tests to fail; you will want to take that into account. |
@robinvd Could you open an issue for this please. With a small example that reproduces the problem. Just to help me understand what this is fixing. |
Closing. Assumed stale. |
@willmcgugan repro example from textual.app import App
from textual.widgets import Tree
class BugApp(App):
def compose(self):
t = Tree("test")
t.show_root = False
t.root.add_leaf("node1")
t.root.add_leaf("node2")
t.root.add_leaf("node3")
yield t
def on_tree_node_highlighted(self, event):
self.notify(f"hightlight! {event}")
BugApp().run() if you now click on the last node, no notification will popup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong but this looks like an issue only when show_root=False
is set in compose
?
Currently this PR causes multiple existing tests to fail as @davep already mentioned. I wonder perhaps this just needs a check if show_root
is False.
When the tree has no selection the cursor_line is -1, then clicking on the last node in the tree will check if the selection is different from the last selection, and do a check nodes[previous_cursor_line] != nodes[new_cursor_line] but if previous_cursor_line=-1 and new_cursor_line=len(nodes)-1 then this will fail and the message will not be emitted.
Please review the following checklist.