-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
DynamicPicker: Recalculate column widths for new options #6004
DynamicPicker: Recalculate column widths for new options #6004
Conversation
This fixes blank row text in a DynamicPicker which is initially given no options. This can happen for language servers which respond to the workspace symbol request for an empty query with an empty list of symbols, and that behavior is somewhat common since returning all symbols as the spec suggests is very expensive. For empty options, `Picker::new` calculated the widths of each column as 0. We can recalculate the column widths when the new options are set to fix this. This refactor is also a good opportunity to formalize setting new options on a picker: besides setting the new options and calculating column widths we also want to reset the cursor and rescore the options.
A small note: This is a bit inconsistent with the normal (non dynamic) picker because the table width is updated when filtering now. So if we have one item with a lot of text for one column (and very little text for the others) the coumn width will be adjusted when that item is filtered out in the dynamic picker but not in the normal picker. I think we should land this as is fix regardless but we might think whether updating the colum widths in the normal picker on filtering might be desirable in the future. This is not that important right now because no This looks good to me and would be good to land before the next release to avoid introducting a regression 👍 |
Thanks for this fix! I was also looking into this yesterday, since I had this issue not only with symbol picker, but also with picker introduced in #4687, which I am trying (works great so far btw.). I am really out of my depth here, but I have couple of questions to better educate myself... If you have time to answer.
Btw. by this change I tried if the global search picker would filter the results with init value from |
The symbol picker would not use WalkBuilder - it's rendering a list of symbols sent by the language server rather than crawling files. Ideally we should be able to display any list of information in the pickers so I don't think we should include WalkBuilder in the picker code itself. |
I can confirm this fixes #5920 for both GDScript and c++ for me. Thanks! |
EDIT: The crash below is unrelated to this PR, I opened #6183 I've been running this for two weeks and it works great! I did recently hit a crash that seems like it might be related:
It happened while editing https://github.com/godotengine/godot/blob/master/editor/project_converter_3_to_4.cpp, and using the workspace symbol picker to search for |
I'm not totally sure but I think from the backtrace that the panic is unrelated to these changes. I'm guessing that the godot language server is returning an incorrect range for a symbol and when we try to show that in the file preview we panic because we're slicing outside of the document. If you can reproduce the crash reliably, could you also test this against master? |
Will do. Just to clarify, this is |
My bad, this does happen even without the patch. I opened #6183 |
…r#6004) This fixes blank row text in a DynamicPicker which is initially given no options. This can happen for language servers which respond to the workspace symbol request for an empty query with an empty list of symbols, and that behavior is somewhat common since returning all symbols as the spec suggests is very expensive. For empty options, `Picker::new` calculated the widths of each column as 0. We can recalculate the column widths when the new options are set to fix this. This refactor is also a good opportunity to formalize setting new options on a picker: besides setting the new options and calculating column widths we also want to reset the cursor and rescore the options.
…r#6004) This fixes blank row text in a DynamicPicker which is initially given no options. This can happen for language servers which respond to the workspace symbol request for an empty query with an empty list of symbols, and that behavior is somewhat common since returning all symbols as the spec suggests is very expensive. For empty options, `Picker::new` calculated the widths of each column as 0. We can recalculate the column widths when the new options are set to fix this. This refactor is also a good opportunity to formalize setting new options on a picker: besides setting the new options and calculating column widths we also want to reset the cursor and rescore the options.
This fixes blank row text in a DynamicPicker which is initially given no options. This can happen for language servers which respond to the workspace symbol request for an empty query with an empty list of symbols, and that behavior is somewhat common since returning all symbols as the spec suggests is very expensive.
For empty options,
Picker::new
calculated the widths of each column as 0. We can recalculate the column widths when the new options are set to fix this. This refactor is also a good opportunity to formalize setting new options on a picker: besides setting the new options and calculating column widths we also want to reset the cursor and rescore the options.Fixes #5920