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 in core and `search_dialog` RPC command #688

Merged
merged 8 commits into from Jun 6, 2018

Conversation

@scholtzan
Copy link
Member

scholtzan commented Jun 2, 2018

This depends on #687

This pull request implements the search_dialog RPC command to signal to the core whether the search dialog is open or closed. This enables to store the search query only in the core.

I also added this to xi-mac for testing: https://github.com/scholtzan/xi-mac/tree/async-find

@googlebot googlebot added the cla: yes label Jun 2, 2018
@cmyr

This comment has been minimized.

Copy link
Member

cmyr commented Jun 3, 2018

Bikeshed:

What exactly is the behaviour that changes based when this is sent? Is it entirely related to the rendering of inactive highlights, or are there other changes?

In general I have a preference for RPC names that communicate behaviour, rather than UI state. In other words, if this flag indicates whether a partial find match should be shown, we might want to call it highlight_partial_find.

@scholtzan

This comment has been minimized.

Copy link
Member Author

scholtzan commented Jun 3, 2018

In addition to that, the core responds with the find_status when this is sent.

@scholtzan

This comment has been minimized.

Copy link
Member Author

scholtzan commented Jun 3, 2018

But in general, I agree, showing/hiding the highlights for the find matches is the main thing that happens. Though, why not call it highlight_find?

@scholtzan

This comment has been minimized.

Copy link
Member Author

scholtzan commented Jun 4, 2018

Okay, I renamed it to highlight_find

@cmyr

This comment has been minimized.

Copy link
Member

cmyr commented Jun 5, 2018

@scholtzan okay this diff got a bit messy: it's really just adding the highlight_find RPC, correct? What's a good strategy for merging this? Do we want to do it all at once, along with the xi-mac PR, with the comments removed?

@scholtzan

This comment has been minimized.

Copy link
Member Author

scholtzan commented Jun 5, 2018

This should be merged after #687. I am currently trying to fix the stack overflow problem but after that find_status should be ready to go.

@cmyr
cmyr approved these changes Jun 5, 2018
Copy link
Member

cmyr left a comment

looks good!

Copy link
Member

raphlinus left a comment

Thanks!

@cmyr cmyr merged commit b8d7cdb into xi-editor:master Jun 6, 2018
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.