Skip to content

Implement very basic find API#704

Merged
Tyriar merged 25 commits intoxtermjs:masterfrom
Tyriar:553_find_api
Jul 13, 2017
Merged

Implement very basic find API#704
Tyriar merged 25 commits intoxtermjs:masterfrom
Tyriar:553_find_api

Conversation

@Tyriar
Copy link
Copy Markdown
Member

@Tyriar Tyriar commented Jun 14, 2017 •
edited
Loading

This PR exposes 2 new functions on Terminal:

findNext(term: string): boolean
findPrevious(term: string): boolean

Currently they scroll to and select the matching search term (using the selection model introduced in #670).

Fixes #553

image

@Tyriar Tyriar added this to the 2.8.0 milestone Jun 14, 2017
@Tyriar Tyriar self-assigned this Jun 14, 2017
@Tyriar Tyriar requested a review from parisk June 14, 2017 03:05
@Tyriar Tyriar mentioned this pull request Jun 14, 2017
Closed
parisk
parisk previously requested changes Jun 15, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@parisk parisk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In general the code looks great. I made a couple of documentation comments.

My main concern about this is that it is bloating the core of the library, since "search" functionality is not essentially part of a terminal emulator.

I would prefer if we could offload this somehow to an add-on or something like that that can be "excluded" at build time.

Edit: I definitely want to see this merged into the code base, but I want us to consider a different place, rather than the core.

src/SelectionManager.ts
this._buffer = buffer;
}

public get selectionStart(): [number, number] { return this._model.finalSelectionStart; }
Copy link
Copy Markdown
Contributor

@parisk parisk Jun 15, 2017

Choose a reason for hiding this comment

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

Can you document these with JSDoc please?

src/SelectionManager.ts
return charIndex;
}

public setSelection(col: number, row: number, length: number): void {
Copy link
Copy Markdown
Contributor

@parisk parisk Jun 15, 2017

Choose a reason for hiding this comment

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

Can you document this with JSDoc please?

@Tyriar
Copy link
Copy Markdown
Member Author

Tyriar commented Jun 15, 2017

👍 I can look into pulling this out into an addon. It will have to use private API. as long as we have good testing to prevent regressions that shouldn't matter though.

@Tyriar Tyriar added the work-in-progress Do not merge label Jun 15, 2017
@parisk
Copy link
Copy Markdown
Contributor

parisk commented Jun 18, 2017

Since we are the ones implementing and maintaining it, I think it's totally OK to use private APIs in add-ons.

@Tyriar
Copy link
Copy Markdown
Member Author

Tyriar commented Jun 23, 2017 •
edited
Loading

@parisk any comments on the current approach? I managed to get search working as both the first multi-file and first TypeScript addon. Right now we can't touch anything outside of addons/search, including interfaces, there's probably a way to share interfaces nicely but not sure atm.

TODO:

  • Test non-browser environments
  • Clean up the gulpfile.js
  • jsdoc

@parisk
Copy link
Copy Markdown
Contributor

parisk commented Jun 23, 2017

@Tyriar the implementation looks good to me 👍 . I also liked that this introduces the first TypeScript add-on 😄 .

A couple of to-dos after merging this PR:

  • Pulling BufferLine into the Buffer class
  • Finding a better way to write add-ons, than encapsulating everything in a closure

@Tyriar Tyriar modified the milestones: 2.9.0, 2.8.0 Jul 3, 2017
@Tyriar Tyriar force-pushed the 553_find_api branch 3 times, most recently from df0303a to 0a15877 Compare July 9, 2017 08:44
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 9, 2017

Coverage Status

Coverage decreased (-0.04%) to 69.382% when pulling 73970ec on Tyriar:553_find_api into 2b92996 on sourcelair:master.

@xtermjs xtermjs deleted a comment from coveralls Jul 9, 2017
@xtermjs xtermjs deleted a comment from coveralls Jul 9, 2017
@xtermjs xtermjs deleted a comment from coveralls Jul 9, 2017
@xtermjs xtermjs deleted a comment from coveralls Jul 9, 2017
@xtermjs xtermjs deleted a comment from coveralls Jul 9, 2017
@Tyriar
Copy link
Copy Markdown
Member Author

Tyriar commented Jul 9, 2017

@parisk ready for review, after wrestling with Travis for 30 minutes 😛

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 9, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 69.394% when pulling 4c0e7b2 on Tyriar:553_find_api into decd81b on sourcelair:master.

parisk
parisk approved these changes Jul 13, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

Amazing! 🚀

@Tyriar Tyriar removed the work-in-progress Do not merge label Jul 13, 2017
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 13, 2017

Coverage Status

Coverage decreased (-0.06%) to 69.467% when pulling af58643 on Tyriar:553_find_api into 25fc01c on sourcelair:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 13, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 69.493% when pulling 43b47ab on Tyriar:553_find_api into 2ebc926 on sourcelair:master.

@Tyriar Tyriar merged commit 87b76aa into xtermjs:master Jul 13, 2017
@Tyriar Tyriar deleted the 553_find_api branch July 13, 2017 18:12
@parisk
Copy link
Copy Markdown
Contributor

parisk commented Jul 13, 2017

Wooo 🎉 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@parisk parisk parisk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tyriar Tyriar

Labels

None yet

Projects

None yet

Milestone

2.9.0

Development

Successfully merging this pull request may close these issues.

3 participants

@Tyriar @parisk @coveralls