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.

Sign up for GitHub

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

Jump to bottom

Implement very basic find API #704

Merged
merged 25 commits into from
Jul 13, 2017
Merged

Implement very basic find API #704

merged 25 commits into from
Jul 13, 2017

Conversation

Tyriar
Copy link
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
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 Outdated
@@ -156,6 +157,9 @@ export class SelectionManager extends EventEmitter {
this._buffer = buffer;
}

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

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
@@ -566,14 +521,22 @@ export class SelectionManager extends EventEmitter {
return charIndex;
}

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

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
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
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
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
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

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
Member Author

Tyriar commented Jul 9, 2017

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

@coveralls
Copy link

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
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

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

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
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

@parisk parisk parisk approved these changes

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