Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Ability to see context of project search results - More lines #702

Closed
wants to merge 8 commits into from

Conversation

MartinMuzatko
Copy link

@MartinMuzatko MartinMuzatko commented Apr 8, 2016

Closes #190
This is my first contribution to an official core atom package.

I know that this PR is missing the ability to configure the amount of lines to see.
Any hints on how to improve this PR is very much appreciated.

I acknowledge the importance of this package to work properly, so please point out any required improvements.

The additions are not tested via Jasmine/Mocha or whatever testing engine you use. No specs provided.

Features

  • Show extra lines of context
  • Correct indentation
  • Overlapping lines should resolve to only show what is required
  • Gaps between code in the same file should be expandable (max gap: 3 lines?)
  • Set amount of lines to see via config
  • Results on the same line or context should appear on that result
  • Remove empty lines in result (maybe)
  • Syntax highlighting (maybe)

Fixing known issues

  • Arrow Key navigation
  • Even if there are results, the statusbars say "No results for ..."
  • Only buildBuffer or save lines that are required within the model

Current Status
image

@DouweM
Copy link

DouweM commented Apr 8, 2016

@MartinMuzatko Can you add some screenshots?

And a few questions:

  • What happens with overlap? Say we have results on line 12 and 15. We would expect to see 10..17, without 13 and 14 repeated
  • What happens with gaps? Say we have results on line 8 and 15. We would expect to see 6..10 and 13..17. Between those, it would be nice to have a line with .., like Sublime does, see Extra lines of context above/below search results #190 (comment).

Also, is indentation shown correctly? It seems to be missing in #190 (comment).

@MartinMuzatko
Copy link
Author

@DouweM that is true, there is nothing implemented to detect line collisions as shown below:

image

Is there a way to detect tabs? I do have the space in the lines, but I have to find out what other options are in the buffer class. Maybe also the identation

Also what bugs me, is that the matches are not separated. it is just a blog of code with the highlight in the middle.

The .. feature is also missing.

I'll continue working on this. Thank you very much for your input @DouweM !

@DouweM
Copy link

DouweM commented Apr 8, 2016

Is there a way to detect tabs? I do have the space in the lines, but I have to find out what other options are in the buffer class. Maybe also the identation

This is likely simply a CSS issue where consecutive spaces are collapsed into one, note that in your screenshot, line 35 appears to have one space of indentation even though it should have more. Try white-space: pre;

Also what bugs me, is that the matches are not separated. it is just a blog of code with the highlight in the middle.

I don't mind that, actually. It's the same way Sublime did it: #190 (comment)

@DouweM
Copy link

DouweM commented Apr 8, 2016

@MartinMuzatko Another suggestion while you're at this: Have the search results use the same monospace font as elsewhere in text editors.

@MartinMuzatko
Copy link
Author

Now I need to deal with Promises, because I need to build the buffer in order to get the lines:

atom.project.buildBuffer(filePath)

@MartinMuzatko
Copy link
Author

Currently I'm learning how to deal with js scope in coffeescript. Any help to solve the promise problem is very much appreciated.

class MatchView extends View
  @content: (model, {filePath, match}) =>
    linesPromise = atom.project.buildBuffer(filePath)
    linesPromise.then (lines) ->
      @getContent(model, {filePath, match, lines})

  getContent: (@model, {filePath, match}) ->
    range = Range.fromObject(match.range)
    matchStart = range.start.column - match.lineTextOffset
    ......

It seems like the scope of the promise has no access to @ (this)

@DouweM
Copy link

DouweM commented Apr 11, 2016

@MartinMuzatko Check out the fat arrow: http://coffeescript.org/#fat-arrow

@MartinMuzatko
Copy link
Author

I have another problem of finding documentation on the View Class. It seems like I have to find another way around, the fat arrow still gives me the window as scope..

I think the best thing to do, is to actually save suffix/prefix in the result as the search is done, not when the view is output. There I can retrieve all the data before I return a view.

image

@DouweM
Copy link

DouweM commented Apr 11, 2016

@MartinMuzatko Your content method is a class method, so @ will refer to MatchView (note that this != @ when using =>). Your getContent is an instance method, before you can use @getContent, you need to instantiate an instance of MatchView and work in the context of that.

I think the best thing to do, is to actually save suffix/prefix in the result as the search is done, not when the view is output. There I can retrieve all the data before I return a view.

Sounds reasonable to me.

@MartinMuzatko
Copy link
Author

Thank you for the pointers, I'll do my best to fix this :)

@MartinMuzatko
Copy link
Author

Have another look please @DouweM. The navigation doesn't work that great yet, buffer lines are now retrieved via model, where it belongs to.
I will soon work to implement the features, namely:

  • Correct indentation
  • Overlapping lines should resolve to only show what is required
  • Gaps between code in the same file should be expandable (max gap: 3 lines?)

Fixing known issues

  • Arrow Key navigation

Updated the initial text for tasks

@emitter.emit 'did-add-result', {filePath, result, filePathInsertedIndex}
bufferPromise = atom.project.buildBuffer(filePath)
bufferPromise.then (buffer) =>
result.lines = buffer.lines
Copy link

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to pass all lines rather than just those that will be used.

I'm not on the Atom team or anything though, so they may be fine with it :)

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if I can tell buildBuffer just to build the lines I need. The API Docs are really missing a lot of information unfortunately: https://atom.io/docs/api/v1.6.2/Project.

I recently researched a bit and it seems like "buildBuffer" uses text-buffer. https://github.com/atom/text-buffer/blob/master/src/text-buffer.coffee

There it looks like it is possible to pass an amount of lines to build from.

However, from the modelside, I can just use the lines required :)

@MartinMuzatko
Copy link
Author

Recently continued my work, commits are here soon. the 2 suffix and prefix lines are now added within the results-model.
I'm getting used to coffeescript, but I ran into another problem:

image

Should we just merge resutls that are in the same line @DouweM ? like

lines = match.lines

@DouweM
Copy link

DouweM commented Apr 14, 2016

@MartinMuzatko That would be really nice

@wonderbeyond
Copy link

How is the progress? I want the feature.

@MartinMuzatko
Copy link
Author

Hello there! Currently I'm pretty busy. The results so far is not quite done yet. Please feel free to build upon what I have created so far. I'll resume once I have some time again for open source projects.

Thanks

@wonderbeyond
Copy link

@MartinMuzatko You are great!
I cannot help more since I'm unfamiliar with Atom internal.
I've followed you at GitHub and Twitter.

@cvogt
Copy link

cvogt commented Jun 10, 2016

+1

@wonderbeyond
Copy link

I want you! @MartinMuzatko

@dirk-thomas
Copy link
Contributor

Please see #847 for another working proposal.

@dirk-thomas
Copy link
Contributor

With #847 being merged this can probably be closed.

@winstliu winstliu closed this Apr 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra lines of context above/below search results
6 participants