Search panes #1521

msirringhaus · 2022-06-17T06:41:04Z

Current draft, before I start working on it in earnest in 2 weeks.

…/backwards

…rch-term

…already know it

…arch'-colors

… most cases)

…d selections and scroll correctly

imsnif · 2022-07-04T15:39:52Z

Hey @msirringhaus - I took an initial look and this looks great! I'm going to test it and review it over the next few days and give feedback.

imsnif

Okay, I've had a chance to do a full review. First thing, let me say: this is top-notch work. You dived head-first into a non-trivial code-base, figured it out and managed to add a very non-trivial feature. Your changes are concise, not touching more than they absolutely have to, and I was personally very impressed by your eye for edge cases an UX details.

I left comments in the code, mostly about moving stuff around and breaking large functions down to make them more maintainable (I realize I'm holding you to higher standards than some of the code-base, but I have to be sure I'm able to effortlessly maintain this on my own if I have to).

Additionally, some UX comments and minor bugs:

In the case of terminals as opposed to the web for example, I think the next/previous commands should be inverse. 99% of the time the user will search for something while being on the last line of the viewport and is looking up. The next instance should be the one above them.
A small bug: search results that are at the end of the line color the background of the whole line until the end
The various search toggles need some sort of UI indication. It's impossible for me as a user to know whether my current search is case-sensitive or whole-word. See next point for a suggestion.
I think the search term should appear in the pane frame while searching as well. Something like SEARCHING FOR: my-search-term, in the future we could add the occurrence too, eg. SEARCHING FOR: my-search-term 5/10. This can also be a good place to indicate the search feature toggles, eg. SEARCHING FOR: my-search-term [case-sensitive, whole-word] - I'm not tied to the square bracket, just a suggestion.
One big snag here is that search is unusable without pane frames (eg. when we toggle them off with ctrl-p + z). The best solution here would be to somehow move the search UI somewhere else (such as the status-bar), but I realize that can be a tall order. We could probably get away with temporarily enabling pane-frames while searching and disabling them again when exiting search. Not ideal... unless you have a better idea?

As we discussed last week on our Matrix chat, I'll take it upon myself to implement changing the foreground color of searches to make them more readable. But I think I'll wait until you implement these changes to avoid merge conflicts.

Once again - awesome work. Looking forward to seeing this merged and released.

(EDIT) Almost forgot:
We also need to fix the e2e tests. I think they're failing because the UI here was changed. They also use cargo insta - you can find more info about running them here: https://github.com/zellij-org/zellij/blob/main/CONTRIBUTING.md#running-the-end-to-end-tests
And also: the CI is complaining about rustfmt and clippy. We'll also need to fix these before merging.

zellij-server/src/panes/grid.rs

zellij-server/src/panes/plugin_pane.rs

zellij-server/src/panes/terminal_pane.rs

zellij-server/src/screen.rs

zellij-server/src/panes/grid.rs

…ve duplicate code

imsnif · 2022-07-13T16:01:36Z

Hey @msirringhaus - I merged main as promised. I still owe you fixing the foreground styles of the marked results, but I think it's not blocking for you, right? I'll try to get to it ASAP

msirringhaus · 2022-07-14T06:08:14Z

Hey @msirringhaus - I merged main as promised. I still owe you fixing the foreground styles of the marked results, but I think it's not blocking for you, right? I'll try to get to it ASAP

Wow, thanks! And no, it's not blocking. I'll try to get to the rest of the feedback ASAP as well, so we can hopefully land this thing

imsnif · 2022-07-14T13:46:44Z

Right, @msirringhaus - I just implemented the search foreground. Let me know what you think. Also, I took the liberty of updating the e2e snapshots to get that off your plate. Let me know if I can help with anything else!

…nd reuse as much as possible

msirringhaus · 2022-07-14T18:58:27Z

Awesome about the merge and implementation of the frontcolor! It looks perfect now! I love it.
I tried to address all inline comments and AFAIK, there is only one thing left:

A small bug: search results that are at the end of the line color the background of the whole line until the end

I'll try to take a look at this ASAP, but not today anymore :-)

Edit: About renaming forward/backward to up/down: I was very happy I did that, before I started refactoring the search-functions. It is so much more clear which direction I'm currently looking at :-)

…the whole line

msirringhaus · 2022-07-17T15:50:30Z

Ok, I found the highlighting-bug. Unless I overlooked something, we are all set now?

…lij-org#1586) * fix(terminal): persist cursor hide/show through alternate screen * style(fmt): rustfmt * style(clippy): make clippy happy

…ellij-org#1587) * fix(editor): handle editor/visual/configured editor with arguments * style(fmt): rustfmt

…1584) * if left click is on pane border do not forward to application * properly handle frames * fix comment * fix another comment * add tests, fix edge case

Flake lock file updates: • Updated input 'crate2nix': 'github:kolloch/crate2nix/2d20dec4ae330f39b0bebeb8eb4a201b58d2b82c' (2022-07-09) → 'github:kolloch/crate2nix/45d97c7ce62c3d53954743057ceb32e483c31acd' (2022-07-12) • Updated input 'nixpkgs': 'github:nixos/nixpkgs/b39924fc7764c08ae3b51beef9a3518c414cdb7d' (2022-07-08) → 'github:nixos/nixpkgs/4a01ca36d6bfc133bc617e661916a81327c9bbc8' (2022-07-14) • Updated input 'rust-overlay': 'github:oxalica/rust-overlay/3dfc78e42a285caaf83633224a42e7fb7dde191b' (2022-07-10) → 'github:oxalica/rust-overlay/2cd36d4aef875867ee1d7963541ccb3ae50b358c' (2022-07-16) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

imsnif · 2022-07-18T07:59:02Z

@msirringhaus - I found and fixed a minor bug: if you searched for something, then moved to a different pane without switching modes (eg. with Alt + LeftArrow), then switched back to normal mode (pressing Enter) and then came back to the original pane, the searches would still be highlighted (which is okay) and not get cleared when you scroll (which is not so nice). Fixed this by clearing the search results (providing they exist) every time the user enters characters to the terminal.

Otherwise, this looks great. I'll wait for CI to pass and then do one last go before I merge.

imsnif · 2022-07-18T08:24:14Z

...and another minor one fixed: when the pane being search was being actively updated, the search results would get out of whack as well (eg. tail -f /my/active/log/file and then search for something).

We already had a mechanism for handling this when scrolling, so I tapped into it.

imsnif

@msirringhaus - everything looks good to me and I'm about to merge this.

Thank you very much for your sustained and tremendous efforts here. This has been a greatly requested and anticipated feature for a long time and your implementation is fantastic. I'm very much looking forward to releasing it in the near future.

I also had fun working with you on this, and while I know you having time to contribute was a special circumstance, I still hope to see contributions from you to Zellij in the future.

msirringhaus · 2022-07-18T08:58:59Z

Well no. Thank you very much for your tremendous patience, guidance and for the fun experience! :-)
Hopefully, it was not the last Hackweek (usually once per year).
In the meantime, I'm trying to get official builds of zellij into openSUSE, by trying to get WASM-support for Rust active.

Martin Sirringhaus added 17 commits June 8, 2022 15:28

WIP: First draft of searching in panes.

0d144a0

Add ability to highlight search-results in viewport and move forwards…

2c7a17e

…/backwards

Clear search results when leaving search

5230acc

Search newly scrolled in lines and have live-search when entering sea…

2c9f88b

…rch-term

search_forward/backward() now doesn't get the needle again, since we …

d5e9981

…already know it

Use red and yellow from theme. No idea if we should introduce new 'se…

b6bb72d

…arch'-colors

Implement moving the viewport for searches outside the current one.

b2b0c40

Implement hacky case-insensitivity (ASCII only at the moment)

7b1b005

Implement wrap-search and prepare infrastructure for whole-word search

5828a6e

Add a bunch of tests and an embarrasing amount of bugfixes

d675d1b

Remember search selection when toggling case-sensitivity (if possible)

403f02e

New tab integration tests and make search work with floating panes

405a3c8

Make highlights work with resize (not keeping the active-selection in…

6c0d0ac

… most cases)

Switch the search-algo a bit in order to make multi-line search work

36b9835

Don't forget active selection when nothing more is found, reflow foun…

4002f32

…d selections and scroll correctly

Make all search-related function calls in plugin-pane No-ops

7b0e326

Activate whole word search (ASCII only)

f61a52e

msirringhaus marked this pull request as ready for review July 1, 2022 11:46

msirringhaus changed the title ~~WIP: Search panes~~ Search panes Jul 1, 2022

Merge branch 'main' into search-panes

ec0e5f6

imsnif self-assigned this Jul 4, 2022

imsnif requested changes Jul 5, 2022

View reviewed changes

Martin Sirringhaus added 5 commits July 5, 2022 16:40

Run cargo fmt

a7732f1

Make clippy happy

7c06589

Remove unneeded transferred_rows_count

17f3a59

Remove boilerplate and use macro instead

36e2cde

Add explanatory comments

bd247ce

msirringhaus force-pushed the search-panes branch from ced8e59 to bd247ce Compare July 5, 2022 15:28

Move search-related functions into SearchResults impl and try to remo…

a006723

…ve duplicate code

imsnif added 2 commits July 13, 2022 17:45

Merge branch 'main' into msirringhaus-search-panes

d9887f6

style(fmt): rustfmt

d6e93cf

imsnif added 3 commits July 14, 2022 15:28

fix(search): adjust foreground color for better readability

511e1f0

style(fmt): rustfmt

941d06b

test(e2e): update snapshots from SCROLL to SEARCH

51ac96c

Martin Sirringhaus added 3 commits July 14, 2022 19:30

Rename search directions to up/down

d870b17

Rename search-functions in tests as well

51f8ecf

Move all search-related functions out of grid.rs and into search.rs a…

db7dea5

…nd reuse as much as possible

Martin Sirringhaus added 2 commits July 17, 2022 17:38

Fix bug where searches that fall on the line-ending are highlighting …

1580a1d

…the whole line

Silence clippy on what I think is a false-positive

c2a3ba3

imsnif and others added 9 commits July 18, 2022 09:16

fix(terminal): persist cursor hide/show through alternate screen (zel…

404c2fe

…lij-org#1586) * fix(terminal): persist cursor hide/show through alternate screen * style(fmt): rustfmt * style(clippy): make clippy happy

docs(changelog): cursor show/hide alternate screen fix

220a7b1

fix(editor): handle editor/visual/configured editor with arguments (z…

897785d

…ellij-org#1587) * fix(editor): handle editor/visual/configured editor with arguments * style(fmt): rustfmt

docs(changelog): editor with arguments

decf2cf

fix(ci): quoting issues (zellij-org#1589)

7724ddb

fix(mouse): avoid forwarding click events on pane border (zellij-org#…

db83b8c

…1584) * if left click is on pane border do not forward to application * properly handle frames * fix comment * fix another comment * add tests, fix edge case

docs(changelog): mouse click on pane frame fix

e5c3a61

fix(search): clear search when entering input in non-search-mode

fa0c122

fix(search): handle searching in updating viewport

71778b4

imsnif approved these changes Jul 18, 2022

View reviewed changes

imsnif merged commit bae1548 into zellij-org:main Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search panes #1521

Search panes #1521

msirringhaus commented Jun 17, 2022 •

edited by imsnif

Loading

imsnif commented Jul 4, 2022

imsnif left a comment •

edited

Loading

imsnif commented Jul 13, 2022

msirringhaus commented Jul 14, 2022

imsnif commented Jul 14, 2022

msirringhaus commented Jul 14, 2022 •

edited

Loading

msirringhaus commented Jul 17, 2022

imsnif commented Jul 18, 2022

imsnif commented Jul 18, 2022

imsnif left a comment

msirringhaus commented Jul 18, 2022

Search panes #1521

Search panes #1521

Conversation

msirringhaus commented Jun 17, 2022 • edited by imsnif Loading

imsnif commented Jul 4, 2022

imsnif left a comment • edited Loading

Choose a reason for hiding this comment

imsnif commented Jul 13, 2022

msirringhaus commented Jul 14, 2022

imsnif commented Jul 14, 2022

msirringhaus commented Jul 14, 2022 • edited Loading

msirringhaus commented Jul 17, 2022

imsnif commented Jul 18, 2022

imsnif commented Jul 18, 2022

imsnif left a comment

Choose a reason for hiding this comment

msirringhaus commented Jul 18, 2022

msirringhaus commented Jun 17, 2022 •

edited by imsnif

Loading

imsnif left a comment •

edited

Loading

msirringhaus commented Jul 14, 2022 •

edited

Loading