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

Find navigator improvements #678

Merged

Conversation

KaiTheRedNinja
Copy link
Contributor

@KaiTheRedNinja KaiTheRedNinja commented Jun 29, 2022

Description

What's been implemented:

  • Ignore/match case (UI needs fixing)
  • Regex
  • Containing term
  • Matching word
  • Starting with word
  • Ending with word

File specific changes:

Related Issue

Checklist

  • Fix Ignore/match case UI (currently looks a bit weird. Need help with this one.)
  • Check implementations of searches
  • I read and understood the contributing guide as well as the code of conduct
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • I documented my code
  • Review requested

Screenshots

The broken UI (adding spacers doesn't help push the text to the right side): now fixed
old: image fixed: image

@KaiTheRedNinja KaiTheRedNinja changed the title Find navigator improvements [WIP] Find navigator improvements Jun 29, 2022
@KaiTheRedNinja KaiTheRedNinja marked this pull request as ready for review June 30, 2022 08:40
@KaiTheRedNinja KaiTheRedNinja changed the title [WIP] Find navigator improvements Find navigator improvements Jun 30, 2022
@0xWDG 0xWDG requested a review from lukepistrol June 30, 2022 14:58
0xWDG
0xWDG previously approved these changes Jun 30, 2022
Copy link
Collaborator

@0xWDG 0xWDG left a comment

Choose a reason for hiding this comment

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

LGTM (not able to test the code ATM)

Copy link
Collaborator

@thecoolwinter thecoolwinter left a comment

Choose a reason for hiding this comment

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

Running this I'm getting a crash Swift/UnsafeBufferPointer.swift:898: Fatal error when there's a ton of results (eg searching "hi" and returning a whole ton of files).
Along with the crash it's also giving me the error
the ID Optional("--") occurs multiple times within the collection, this will give undefined results! In a ForEach body.
I think it's just that the ID for each search result needs to be unique but right now it's using the value of the result's string.

@KaiTheRedNinja
Copy link
Contributor Author

Running this I'm getting a crash Swift/UnsafeBufferPointer.swift:898: Fatal error when there's a ton of results (eg searching "hi" and returning a whole ton of files). Along with the crash it's also giving me the error the ID Optional("--") occurs multiple times within the collection, this will give undefined results! In a ForEach body. I think it's just that the ID for each search result needs to be unique but right now it's using the value of the result's string.

Turns out that this was an error in one of the files that I hadn't even touched in my PR (CodeEdit/NavigatorSidebar/FindNavigator/FindNavigatorResultFileItem.swift), and somehow the changes that I made allowed those errors to pop up. Making the ID reliant on the whole class instead of just the display name solved the issue.

thecoolwinter
thecoolwinter previously approved these changes Jul 7, 2022
Copy link
Collaborator

@thecoolwinter thecoolwinter left a comment

Choose a reason for hiding this comment

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

LGTM

0xWDG
0xWDG previously approved these changes Jul 7, 2022
@KaiTheRedNinja KaiTheRedNinja dismissed stale reviews from 0xWDG and thecoolwinter via 0ae6ad2 July 9, 2022 05:12
@austincondiff austincondiff merged commit 69a7234 into CodeEditApp:main Jul 9, 2022
@KaiTheRedNinja KaiTheRedNinja deleted the find-navigator-improvements branch July 10, 2022 01:33
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.

4 participants