Skip to content
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

Added ExpandableResultsEntry #6

Merged
merged 7 commits into from
Sep 10, 2024
Merged

Conversation

Minenash
Copy link
Collaborator

@Minenash Minenash commented Sep 9, 2024

Adds a ExpandableResultEntry, which has a method to get child entries. When a ExpandableResultEntry is selected, it adds it's children below it (with adjusted background color) and the expandable icon is rotated. Child-entries don't sorted by usage, and this is intentional. Though the theming might need adjustment

image
image

@BasiqueEvangelist BasiqueEvangelist self-requested a review September 9, 2024 00:58
@Minenash
Copy link
Collaborator Author

Minenash commented Sep 9, 2024

Found a problem with. Because it rebuilds the list, and it was just activated, it moves the one you just click on to the top instead of inline. This also can cause problems where it doesn't fully scroll back to the top

Comment on lines 73 to 76
if (isExpanded) {
for (ResultEntry e : ((ExpandableResultEntry)entry).getChildren())
child(new ResultEntryComponent(screen, e, true, false));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this just doesn't do anything if a child entry of an expandable entry is also expandable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I probably should have documented somewhere, but this behavior is intentional. There isn't a good way to distinguish child-child entries visually (especially not one that scales to the theoretical infinite nesting), and it would greatly complicate things

screen.searchBox.onCharTyped(chr, modifiers);

return true;
if (root() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would root() be null here? aren't input events only routed to components if they're mounted?

Copy link
Collaborator Author

@Minenash Minenash Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would root() be null here?

IDK. I just now that it can, because it crashed. I'll see if this still happens after my aforementioned changes

@BasiqueEvangelist
Copy link
Contributor

Found a problem with. Because it rebuilds the list, and it was just activated, it moves the one you just click on to the top instead of inline. This also can cause problems where it doesn't fully scroll back to the top

yeah i want to make rebuilding the result list actually keep focus (mainly because I want to make it so that it's rebuilt if you run any entry that doesn't close the screen)

@Minenash
Copy link
Collaborator Author

Minenash commented Sep 9, 2024

Ah rip, I missed your review at first and completely re-did how the child entries get added due to the bug I mentioned. (I mean it doesn't fully work yet but yeah)

@Minenash
Copy link
Collaborator Author

Minenash commented Sep 9, 2024

Okay, I know there is one problem with the commit I just made, but I need to do something else and just wanted to push.

There is nothing stopping child entries from expanding, but if they do it will break the new system, so I got to make the system work with that, or make it so they aren't able to expand in the first place

@BasiqueEvangelist BasiqueEvangelist self-requested a review September 9, 2024 11:02
@Minenash
Copy link
Collaborator Author

Minenash commented Sep 9, 2024

Ready for review

@BasiqueEvangelist BasiqueEvangelist merged commit 4ddd993 into wisp-forest:1.21 Sep 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants