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

Refactor results-view, upgrade context lines and multi-results lines #1002

Merged
merged 6 commits into from May 12, 2018

Conversation

Projects
None yet
6 participants
@PoignardAzur
Copy link
Contributor

PoignardAzur commented Mar 14, 2018

Description of the Change

This PR refactors results-view, that is, the part of find-and-replace that displays the matches for the "Find in Project" feature.

The main drive behind this refactor is to implement issue #935, "Combine multiple results on one line into one result". This PR also implements a more coherent rendering of context lines between adjacent results (see screenshots below).

Considered alternative and rationale behind the refactoring

Ideally, it would have been better/simpler (at least on the short term) to patch the aforementioned features into the existing code.

Concretely, the dependency chain goes like this:

  • I need coherent rendering of context lines before I can merge results
  • I need to update the selection code to accommodate merged results (what happens when you press up or down?)
  • I need to update the size calculation code to account for overlapping context lines

The problem is, that, as it is, the code for size calculation and selection cursor is a mess.

  • The cursor update calculation is complicated, with a lot of implicit invariants.
  • While most of it is located in lib/project/results-view.js, some of it is split in lib/project/results-model.js, which makes it harder to reason about.
  • A lot of code violates Don't Repeat Yourself; the same functionality is implemented twice in lib/project/result-view.js and lib/project/list-view.js
  • A lot of methods in lib/project/results-view.js dealing with the selection invariants (what must happen when moving the cursor up or down) are extremely complicated, with a lot of potential for subtle errors. For instance, selectResultAtPosition is 54 lines long, with a lot of similar code dealing with special cases.

In summary, the code is too spread out and too hard to reason about. This is a problem because it introduces some subtle, hard to predict errors like #916 and #997.

For these reasons, implementing the overlapping context lines feature and the result merging feature would be difficult and error-prone with the current codebase.

Benefits

  • Fixes a few cache invalidation bugs (including a few undocumented bugs that I found while developing this feature).
  • Makes a lot of the code easier to read and reason about.
  • Implements better rendering for context line overlap, and multi-results lines:

Before
screenshot_20180314_225259
After commit 136048c
screenshot_20180314_225825
Now (with multi-matches lines):

Drawbacks

  • The proposed implementation is somewhat less scalable for very high numbers of files. However, the current implementation is already not scalable, and this PR would make the performances dependent on fewer bottlenecks.

    • The number of DOM Nodes used remains roughly the same (DOM Nodes are only created for results in the visible part of the screen, and are cycled when scrolling). This is unchanged.

    • The props used for each row, including paths, matches and context lines, are all stored in one array. In the existing implementation, the results for each file were stored in an array, each with their own array of matches. This means that, given N files with M lines, adding/removing/updating the results for one file meant splicing the array of files, which were respectively O(N) and O(1) operations. It's now a O(N * M) operation, which may cause unresponsiveness for projects with many large files (although the existing implementation would presumably have the same problem for projects with any single very large file).

    • Making the implementation more scalable would be a matter of:

      • Using scalable data structures for any variable that needs to store a project-dependent amount of data.

      • Updating TextEditor.scan and results-model to be able to give diffs when scanning the same file again. (eg: this result was added, these results were removed, all others stayed the same)

  • The PR is a deep refactoring, which is somewhat disruptive for other people working on this part of find-and-replace.

  • This PR causes one regression, folding-then-unfolding a file when a result was selected inside used to keep the same result selected. Folding-unfolding a file now results in the file's row being selected. The reason for this regression is that keeping track of which match is selected inside a folded file is harder post-refactoring; the way the current implementation keeps track of this information is complex and error-prone, and I think the added complexity is too expensive compared to how infrequent the use cases are.

Work left

Before this PR can be merged, some additional work is needed

  • Some tweaks are needed to pass unit tests. Right now, all tests pass on my machine except for file-icon-related tests. I would appreciate if someone could take a look at the spec part of my PR.

  • Some bugs still need to be fixed. I don't think there are many left, though.

  • I still need to implement issue #935. I fully expect it to be done within two weeks.

  • I can't display the little arrows that indicate whether a group is folded or not (as you may have seen on the before/after screenshots). How these arrows are supposed to be drawn in the original code eludes me; they're displayed as some sort of "::before" property, but I don't know which combination of classes triggers it. Any help appreciated, it's the last hurdle.

I strongly appreciate input and code reviews in the meantime, especially from core atom developers.

Possible improvements:

  • The general appearance needs improving. In particular, the context rows should be easier to differentiate from match rows. Also, it might be nice to display little "x2" indicators next to rows that have eg x2 matches (and "x3" indicators, "x4", etc).

  • Implement the behavior when pressing Ctrl+C on a result line with multiple matches; right now, the first match is copied, but it might make more sense to copy all matches, using the same mechanism as multiple cursor copying.

Applicable Issues

I would argue this PR is relevant to #765.

Refactor results-view and upgrade context lines handling
Refactor results-view to be simpler, with fewer implicit invariants and
a better application of the Don't Repeat Yourself principle.

As a result, context rows betwen adjacent lines are rendered in a
more coherent way.

Update unit tests.
Minor changes.

@PoignardAzur PoignardAzur force-pushed the PoignardAzur:refactor-results-view branch from 02698e1 to 136048c Apr 14, 2018

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 14, 2018

PR Updated, now passes tests.

Add multi-matches lines
Update ResultRowGroup and MatchRow so that lines that include multiple
matches for the search are displayed on one row, with each match being
highlighted.

See issue #935 for details / illustration.
@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 14, 2018

Add multi-matches lines. Solves #935.

image

You may now rejoice.

@PoignardAzur PoignardAzur force-pushed the PoignardAzur:refactor-results-view branch from 4638934 to 7499008 Apr 14, 2018

@Arcanemagus

This comment has been minimized.

Copy link

Arcanemagus commented Apr 14, 2018

Could you update the main description with an updated "After" screenshot, as well as noting that this also fixes #935?

I can't display the little arrows that indicate whether a group is folded or not

Normally I would say to use git bisect to see where this broke, but as you've merged this massive set of changes down to 3 commits that's not very helpful 😆. Until this is fixed this PR definitely can't be merged, I'll try looking into this but I'm not familiar with the code at all so I'm not making any promises here.

The proposed implementation is somewhat less scalable for very high numbers of files. However, the current implementation is already not scalable, and this PR would make the performances dependent on fewer bottlenecks. (namely, row splicing)

Could you go into more detail in the main PR body as to what the tradeoffs done here are? The current bottlenecks might be useful to file as issues if they aren't already so they can be investigated at some point in the future.

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 15, 2018

@Arcanemagus Thanks for the feedback!

Could you update the main description with an updated "After" screenshot, as well as noting that this also fixes #935?

Done. Description overhauled.

Normally I would say to use git bisect to see where this broke, but as you've merged this massive set of changes down to 3 commits that's not very helpful.

I haven't squashed them or anything. The first commit is huge because I only committed once I was able to display anything on the screen (plus bugfixes and unit tests), and by that time I already had that bug.

Though I guess I did make this more disruptive than I strictly had to. Oh well.

Could you go into more detail in the main PR body as to what the tradeoffs done here are?

Done. I might file them as issues, but I haven't really tried to work with the massive file sizes where this would be a serious problem, and I don't really feel like it. So far this is only me theorizing on how scalable the code is for the sake of listing possible drawbacks.

@PoignardAzur PoignardAzur changed the title Refactor results-view and upgrade context lines handling Refactor results-view, upgrade context lines and multi-results lines Apr 16, 2018

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Apr 16, 2018

👋 I wish I had time to review this code more thoroughly right now, but wanted to give a big 👍 for this great improvement in UX.

Some thoughts on the remaining todos:

Also, it might be nice to display little "x2" indicators next to rows that have eg x2 matches (and "x3" indicators, "x4", etc).

This is an interesting idea, but I think it would be perfectly fine to ship this without any indicator.

I can't display the little arrows that indicate whether a group is folded or not (as you may have seen on the before/after screenshots).

Yeah, it's a little hard to find. The styles are defined here.

lineNumber: matchLineNumber - dy,
matchLineNumber,
}
this.dy = dy

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Apr 16, 2018

Contributor

The name dy made me think this variable represented a height value in pixels, but it's actually a number of rows, relative to the match row, right? What do you think about renaming this to something like rowOffset?

This comment has been minimized.

@PoignardAzur

PoignardAzur Apr 17, 2018

Author Contributor

Sure, I'll change it.

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 17, 2018

👋 I wish I had time to review this code more thoroughly right now, but wanted to give a big 👍 for this great improvement in UX.

Thanks! That's really nice to hear.

This is an interesting idea, but I think it would be perfectly fine to ship this without any indicator.

Yeah, I was thinking I'd maybe make a new issue about that if the PR were shipped?

Yeah, it's a little hard to find. The styles are defined here.

Yeah, I would not have found this on my own.

Also, if you don't have time to review everything, could you take a look at the unit tests? It's probably the part I'm least confident in.

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 21, 2018

Final look:

image

This PR is ready to be merged as far as I'm concerned. There are still two items on the TODO-List above, but I think I'll just write them as issues once the PR is merged.

Last bit of feedback: the specs for this project are a pain in the ass. They use the outdated default jasmine runner instead of something more flexible like mocha (there are bunch of times where having a ".only" tag would have saved me a lot of time) and they mix unit tests and integrations tests; eg tests that check whether line selection remains consistent when adding/removing results, by browsing through the DOM for the element with the '.selected' class. That makes them longer than they need to be, both in term of lines of code and how long they take to run.

@50Wliu

This comment has been minimized.

Copy link
Member

50Wliu commented Apr 21, 2018

Regarding specs: we use a Jasmine extension that allows fit to focus a spec, similar to it.only in Mocha (and Jasmine 2?). We're aware that our specs aren't exactly in the best shape though...

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Apr 24, 2018

Just did a little bit of testing. Navigating the results with the arrow keys doesn't seem to work when context lines are enabled.

  • Sometimes the selection doesn't move when I hit up, down, left or right.
  • Sometimes expanding a search result does not autoscroll to reveal the highlighted line.
  • At one point, after expanding a search result and then typing the left arrow key, I got this exception:
Uncaught TypeError: Cannot read property 'group' of undefined
    at ResultsView.collapseResult (/Users/max/github/find-and-replace/lib/project/results-view.js:457:40)
    at CommandRegistry.handleCommandEvent (/Users/max/github/atom/src/command-registry.js:384:43)
    at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (/Users/max/github/atom/node_modules/atom-keymap/lib/keymap-manager.js:617:16)
    at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (/Users/max/github/atom/node_modules/atom-keymap/lib/keymap-manager.js:408:22)
    at WindowEventHandler.handleDocumentKeyEvent (/Users/max/github/atom/src/window-event-handler.js:110:34)
@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 25, 2018

I don't have my laptop at hand, but looking at the code, I think the problem is likely that I forgot to update references to "row.dy". I'll have a look at it within a few days, but in the meantime, could you give me the steps to reproduce the exception, and the "doesn't autoscroll" bug?

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Apr 25, 2018

Here’s some rough steps:

  1. Do a project search in this repo for something common like “editor”.
  2. Set the leading and trailing context line counts to one, using the context line plus buttons.
  3. Navigate around the results list using the four arrow keys. Expand and collapse files with lots of matches, navigate between files.

I think you’ll notice the problems from there.

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented Apr 25, 2018

@PoignardAzur To reproduce the exception:

  1. Open Atom in atom/atom
  2. Perform a project wide search for editor
  3. Select the first file
  4. Run the move right command
  5. Run the move right command

The first move right command should restore selection of the result that was selected when collapsing the file. Or select the first result if nothing was selected before collapsing.

The second move right should be a noop because the file is already expanded.

With this PR the last selected result is never selected and instead moving right seems to cause an invalid selection.

This also reproduces if you after the first move right run the move left command.

Here is a gif where I do down,up,right,right movement. Down and up is just to confirm that the cursor is on the first file.
group of undefined

Another way to reproduce is:

  1. Open Atom in atom/atom
  2. Perform a project wide search for editor
  3. Collapse all results
  4. Click to expand one of the results
  5. Move right or left

Clicking to expand one of the results should select the file you click but it seems to cause the same invalid selection.

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented Apr 25, 2018

Adding to Max notes about keyboard navigation:

  • core:move-to-bottom does not select the last result when context lines are enabled and the results are expanded
  • core:page-up and core:page-down navigation does not work when context lines are enabled and the results are expanded
@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 26, 2018

The first move right command should restore selection of the result that was selected when collapsing the file. Or select the first result if nothing was selected before collapsing.

This behavior is no longer possible in this PR (which I should have listed as a regression, my bad). The intended behavior of the PR is that folding-unfolding a file now results in the file's row staying selected.

Long story short, it's a trade-off: keeping track of which match is selected inside a folded file is possible, but it leads to complicated code, subtle cache-invalidation-type bugs, and I think that added complexity is too expensive compared to how infrequent the use cases are.

  • core:move-to-bottom does not select the last result
  • core:page-up and core:page-down navigation does not work

I'll check these out and the other bugs this evening (Korean time). By the way, how did you try the "move-to-bottom" one? Did you type "move-to-bottom" in the command panel, or did you try the "ctrl-end" keybinding? I ask because the keybinding is currently broken in master (see #1015).

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented Apr 26, 2018

By the way, how did you try the "move-to-bottom" one? Did you type "move-to-bottom" in the command panel, or did you try the "ctrl-end" keybinding? I ask because the keybinding is currently broken in master

Yeah I used the command from the command-palette. I wouldn't say the command is broken on master. It's just not bound to any keyboard shortcut by default. You could bind it in your personal keymap.

I'll ask the team to take a look at #1015 to discuss if this is something we want to include by default.

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented Apr 26, 2018

And... done.

To confirm, the bug came from a previous commit where I replaced dy by rowOffset, but forgot two occurrences in the code. So, my bad, and thanks for catching it. PR should now work as intended.

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented Apr 26, 2018

@PoignardAzur @maxbrunsfeld I can confirm that the issues I saw are fixed by the latest commit.

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented May 3, 2018

@maxbrunsfeld Any news?

@Ben3eeE Ben3eeE added the needs-review label May 3, 2018

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented May 10, 2018

@maxbrunsfeld I don't mean to be rude, but the PR has been ready to merge (as far as I'm aware) for two weeks; I get that you have other things going on, but I'd really like get something, even if it's just "hey, we haven't forgotten about you, will look at the code again next month".

@Ben3eeE

This comment has been minimized.

Copy link
Member

Ben3eeE commented May 10, 2018

@PoignardAzur We haven't forgotten about this PR.

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented May 12, 2018

Tested again and looks good. Thanks @PoignardAzur!

@maxbrunsfeld maxbrunsfeld merged commit 4264c9c into atom:master May 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PoignardAzur PoignardAzur deleted the PoignardAzur:refactor-results-view branch May 13, 2018

@PoignardAzur

This comment has been minimized.

Copy link
Contributor Author

PoignardAzur commented May 13, 2018

\o/ I am happiness and hugs!

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented May 14, 2018

This will ship in Atom 1.28.0. The beta release, 1.28.0-beta0, will come out tomorrow.

@simonrepp

This comment has been minimized.

Copy link

simonrepp commented Jun 1, 2018

@PoignardAzur Fantastic stuff, very much looking forward to using it, thanks so much :) (also to everyone who reviewed and tested!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.