zed icon indicating copy to clipboard operation
zed copied to clipboard

Search in selections

Open kshokhin opened this issue 1 year ago • 8 comments

Release Notes:

kshokhin avatar Apr 22 '24 06:04 kshokhin

Hello, Zed team!

Still WIP, couple of questions here:

  1. Currently one of the available icons is used for the button. What is the workflow to get the proper icon? I don't think I'm able to draw it myself
  2. Current implementation launches searches in all selections concurrently. Is it ok, or a straightforward loop over the search ranges, like the search is implemented for buffer excerpts, is more preferable?

Things TBD:

  1. Remove auto-filling of the search editor with the selected content. Though, I'm not quite sure, what the exact solution should be here - do not auto-fill if multiple lines are selected?
  2. Selections are cleared on every matches set update, which is kind of annoying, when I'm trying to search in a selection. So I'm thinking of disabling this behaviour, when "search-in-selections" mode is toggled. Please, share your thoughts if you think of anything better.

kshokhin avatar Apr 22 '24 06:04 kshokhin

@kshokhin Hopefully @iamnbutler can help with getting the icon right. You might be able to reuse "quote" for now (as that's similar to VScode's icon; though pretty hard to interpret if you are not used to it :D) or "filter"? Either way we'll need a good tooltip.

I'd love to pair with you on this as that's probably more effective than me guessing at how it should work in a comment :D – if you have time let's chat next week: https://calendly.com/conradirwin/pairing.

We're also running into some overlap with @cthulhua's work on #10709; so I may need to reconsider what I put there too...

Some thoughts in no particular order:

  • I agree with not prefilling the search bar with a multiline query.
  • I am not sure that the default when opening a search with a multiline selection should be to "find within selection". I think it makes sense to use a separate action for this.
  • When you are in a search with a specific range, we should store that range on the editor so it can highlight them using editor.highlight_background (or possibly highlight_rows, but it seems "neater" to preserve the exact selections). This will require some changes to the SearchableItem trait, but will at least mean that you can still use the editor as normal without affecting your search range.
  • When you are in this mode, it makes sense to show an indicator in the search bar that "searching is restricted to a range". Though that indicator should be outside of the text box (and hidden in narrow_mode). Clicking on the indicator (or closing search) would clear the ranges.
  • (Bikeshedding...) The search bar can be used at quite small sizes, and I'd like to avoid adding an extra button when you're not in this (relatively uncommon) mode. Maybe we could make the entry point to this in the editor when you have a multiline selection? Screenshot 2024-04-25 at 20 06 29
  • Also happy to leave this for later work, as that is significant scope creep!

ConradIrwin avatar Apr 26 '24 02:04 ConradIrwin

This PR is extremely useful when you are aware that theres a secondary way to do editor: select all matches, which is search: select all matches. editor: select all matches is incredibly powerful, but you are stuck with getting everything in the entire document. Being able to highlight a block, toggle this selection mode, and then run search: select all matches is huge!

JosephTLyons avatar Apr 26 '24 07:04 JosephTLyons

If you are able to get this to search inside of multiple disjoint selection, that would be killer.

JosephTLyons avatar Apr 26 '24 07:04 JosephTLyons

In https://github.com/zed-industries/zed/pull/10709 we ended up allowing the editor to store "search within" ranges (though in that case we clear them as soon as we're done).

I think we can build on that to make this work without having to be so careful about the selection.

ConradIrwin avatar Apr 30 '24 00:04 ConradIrwin

@kshokhin How're you getting on with this? Happy to take a look if you think it's ready

ConradIrwin avatar May 14 '24 20:05 ConradIrwin

@ConradIrwin Sorry for the delays, not always manage to devote as much time as I want, please bear with me. I'm finally done with the multi-buffer support, so I think it's ready to be reviewed, while I'm proceeding with writing some tests

  1. Search-in-selections mode button moved out of the query editor to the search panel
  2. Highlighted ranges cleared on search panel dismiss, search-in-selections mode is disabled as well
  3. Do not autofill search query editor with multiline selection
  4. Search-in-selections works with multi-buffers, including the case when the selection spans over multiple excerpts

kshokhin avatar May 19 '24 13:05 kshokhin

No problem – this seems to work great! I think the final thing we need is the default keybinding (in VSCode it's cmd-alt-l on Mac (presumably ctrl-alt-l on linux) to toggle the selection on and off.

If you want help with tests, let me know.

ConradIrwin avatar May 21 '24 02:05 ConradIrwin

@ConradIrwin Hi! I've added the bare minimum of unit tests, so to me PR seems to be ready for review

kshokhin avatar May 30 '24 17:05 kshokhin

Hi @ConradIrwin !

Thanks for patches, I'll check why the test fails, though I do not see any difference in behaviour of the latest zed release and this version.

Is there any way I can trigger the CI checks myself?

kshokhin avatar Jun 05 '24 07:06 kshokhin

It’s not currently possible for you to trigger a run, but you can run the test locally with cargo test -p vim test_non_vim_search. If it’s easier I can take a look at the test too

On Wed, Jun 5 2024 at 01:57, kshokhin @.***> wrote:

Hi @ConradIrwin https://github.com/ConradIrwin !

Thanks for patches, I'll check why the test fails, though I do not see any difference in behaviour of the latest zed release and this version.

Is there any way I can trigger the CI checks myself?

— Reply to this email directly, view it on GitHub https://github.com/zed-industries/zed/pull/10831#issuecomment-2149133228, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAXAQGSJDVSFSN7D3JHT6TZF3AGDAVCNFSM6AAAAABGSDYYIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBZGEZTGMRSHA . You are receiving this because you were mentioned.Message ID: @.***>

ConradIrwin avatar Jun 05 '24 14:06 ConradIrwin