WIP: Integrate the find-and-replace panel with the Julia Console #187

NHDaly · 2018-12-28T19:45:22Z

Integrates the Julia Console with Atom's built-in find-and-replace plugin!

Still a work in progress. Some remaining features that are probably necessary:

Show the correct number of matches (currently always shows 1 -- which is arbitrary)
show "No results found" if zero matches.
Allow find-previous as well as find-next
Cycling through multiple results on the same line

Finished features:

search through the whole Console output
enable/disable Regex matching
enable/disable Case-sensitive search

Some of those missing features will get easier after the refactoring planned here: xtermjs/xterm.js#1833

But still, it's not bad as-is!! It might be already at a stage where it's worth merging after we clean up the code / pare it down to just what's needed, and add unit tests...

Fixes JunoLab/Juno.jl#212.

This allows calling "Find" from the Julia Console, using the standard find-and-replace package! :) It's currently just searching a dummy `TextBuffer` that contains `"HI IT'S NATHAN"`. The next step is to hook it up to the actual xterm.js Terminal's search capability.

It currently only does `findNext`, always going forward every time you hit enter (even if you press shift-enter). It also always incorrectly reports just "1 result" (It must return at least one result to allow you to cycle through them.)

pfitzseb · 2018-12-28T20:01:45Z

Nice works and thanks for the contribution! :)

One major problem with reusing find-and-replace is that it only seems to work in the workspace center and not in a dock (those toggleable things at the sides). I don't think we can change that without upstream changes (which I'm not sure would be accepted).

Replicating a minimal UI for this should be fairly easy though, I think. I'd be happy to do that though if you don't feel like it ;)

NHDaly · 2018-12-28T20:07:42Z

Nice works and thanks for the contribution! :)

:D Thanks!

and not in a dock (those toggleable things at the sides).

Hmm, but does that matter for us? Is it currently possible to open the Console in a dock on the side? Can you show me how to do that? :)

Replicating a minimal UI for this should be fairly easy though, I think. I'd be happy to do that though if you don't feel like it ;)

Yeah, agreed! If you want to do that, I'd appreciate it, yeah! You're right, i guess there's not really a reason to reuse the UI, since we won't ever support the Replace part of find-and-replace.

That said, there's a fair bit of UI to include: the search term, buttons for Regex, case sensitivity, only in selection, and whole word, and finally find next and find previous.

And finally, unfortunately, i'm currently more stuck on integrating with xterm than with the find-and-replace UI. Their current search plugin gets stuck on things like finding more than one match on the same line.

This reverts commit 608d3a9. It breaks searching for same-line words, which means it also breaks typing mutliple characters in a row for a long word.

pfitzseb · 2018-12-28T20:18:37Z

Hmm, but does that matter for us? Is it currently possible to open the Console in a dock on the side? Can you show me how to do that? :)

That's the default:

With the config above you can search through the atom-ink terminal but not the REPL.

That said, there's a fair bit of UI to include: the search term, and buttons for Regex, case sensitivity, only in selection, and whole word, and finally find next and find previous.

True. I might get to it tomorrow, but no promises yet.

And finally, unfortunately, i'm currently more stuck on integrating with xterm than with the find-and-replace UI. Their current search plugin gets stuck on things like finding more than one match on the same line.

Yeah, but VSCode makes do with the same API for now so I guess we can too.

NHDaly · 2018-12-28T20:42:57Z

Oh, weird! Your atom looks different than mine.. I don't have the fancy bar on the left, and when i open the Console through the Packages > Julia > Open Console, it opens in the workspace! I'll investigate what's going on with my installation...

Thanks for showing me that! Yeah you're right that the find-and-replace is currently just looking for the last active Pane. It would probably require significantly more hacking to pretend to be a Pane, and it's probably not worth it. A custom UI makes sense to me then. :) Thanks!

Yeah, but VSCode makes do with the same API for now so I guess we can too.

Oh cool, i didn't realize. Yeah, makes sense then! I think with a custom UI those above problems won't be as relevant anyway. Thanks.

pfitzseb · 2018-12-29T10:28:42Z

Ok, I've taken a stab at this (UI, keybindings and integration are done):

Would appreciate you trying this (especially since you're on a mac, right?) -- I never get keybindings right there.

NHDaly · 2019-01-02T15:04:43Z

Would appreciate you trying this (especially since you're on a mac, right?) -- I never get keybindings right there.

Yeah it works great! :) Thanks for putting this together so fast!
I agree that this plays more nicely with the interface exposed by the xterm.js search plugin, and because of the Dock thing I didn't know about, I definitely agree this is a better approach. :) Thanks for putting it together! :)

These two "missing features" i have in the top comment (reproduced here) still apply, but they're due to limitations in xterm.js's search which will hopefully improve with the redesign.

Cycling through multiple results on the same line
Second search fails for same string if only occurs once (a consequence of the previous line). This also means that if you search "Doc" and then "Documentation", the second search will fail.

I think this looks great and would be good to merge any time! :) Thanks for the fast work here, @pfitzseb! ❤️❤️❤️

NHDaly

LGTM! Thanks! 👍

NHDaly · 2019-01-02T15:06:06Z

lib/console2/console.js

@@ -3,23 +3,26 @@

 import etch from 'etch'
 import { Raw } from '../util/etch.js'
-import { CompositeDisposable, Emitter } from 'atom'
+import { CompositeDisposable, Emitter, TextEditor, TextBuffer, Disposable, MarkerLayer, Range } from 'atom'


I think we can revert these changes as well

Suggested change

import { CompositeDisposable, Emitter, TextEditor, TextBuffer, Disposable, MarkerLayer, Range } from 'atom'

import { CompositeDisposable, Emitter } from 'atom'

NHDaly · 2019-01-02T15:10:09Z

lib/console2/searchui.js

+import { closest } from './helpers'
+import { openExternal } from 'shell'
+
+export default class TerminalSearchUI {


nit: This class actually does the UI /and/ the search functionality. Would like SearchBar or SearchPanel or SearchBox be a reasonable name instead?

-- oh, wait, i guess "UI" doesn't necessarily mean "html-only"... Maybe ignore me. That said, when i first glanced through this PR i scrolled past this file expecting it just to be the HTML/CSS stuff, and expecting the functionality to be implemented somewhere else, so idk if others might have the same thought.

NHDaly · 2019-01-02T15:11:04Z

styles/terminal.less

+    &.nothingfound {
+      border-color: @background-color-error;
+    }
+  }


Wow, i'm really impressed how little code this took to add! Good job building up abstractions in this repo so reuse was so easy! :) I'm impressed! 😄

NHDaly · 2019-01-02T16:46:58Z

One problem to note: for me, when the regex doesn't parse, it opens the javascript developer console to show a javascript exception:

Uncaught SyntaxError: Invalid regular expression: /*j/: Nothing to repeat
    at RegExp (<anonymous>)
    at SearchHelper._findInLine (/Users/nathan.daly/Documents/atom-packages/atom-ink/node_modules/xterm/lib/addons/search/SearchHelper.js:76:31)
    at SearchHelper.findNext (/Users/nathan.daly/Documents/atom-packages/atom-ink/node_modules/xterm/lib/addons/search/SearchHelper.js:20:27)
    at findNext (/Users/nathan.daly/Documents/atom-packages/atom-ink/node_modules/xterm/lib/addons/search/search.js:10:41)
    at Terminal.terminalConstructor.findNext (/Users/nathan.daly/Documents/atom-packages/atom-ink/node_modules/xterm/lib/addons/search/search.js:23:16)
    at TerminalSearchUI.findNext (/Users/nathan.daly/Documents/atom-packages/atom-ink/lib/console2/searchui.js:51:31)
    at HTMLElement.inkTerminalFindNext (/Users/nathan.daly/Documents/atom-packages/atom-ink/lib/console2/console.js:51:25)
    at CommandRegistry.handleCommandEvent (/Applications/Atom.app/Contents/Resources/app.asar/src/command-registry.js:384:43)
    at KeymapManager.module.exports.KeymapManager.dispatchCommandEvent (/Applications/Atom.app/Contents/Resources/app.asar/node_modules/atom-keymap/lib/keymap-manager.js:617:16)
    at KeymapManager.module.exports.KeymapManager.handleKeyboardEvent (/Applications/Atom.app/Contents/Resources/app.asar/node_modules/atom-keymap/lib/keymap-manager.js:408:22)
    at WindowEventHandler.handleDocumentKeyEvent (/Applications/Atom.app/Contents/Resources/app.asar/src/window-event-handler.js:110:34)

Can we catch that exception and show it somehow? This is what the built-in find-and-replace package does:

pfitzseb · 2019-01-03T09:04:40Z

Good point -- fixed that in the latest commit and cleaned up the code a bit:

I think this should be good to go now.

and general code cleanup

pfitzseb · 2019-01-03T09:30:48Z

And with the new xterm.js release it's possible to find multiple items on the same line. 🎉

NHDaly · 2019-01-03T16:15:42Z

Good point -- fixed that in the latest commit and cleaned up the code a bit:

Looks beatiful! :) Thanks!

And with the new xterm.js release it's possible to find multiple items on the same line. 🎉

Haha woohoo that's awesome! :D :D Perfect timing! :)

:D Thanks again for picking this up and doing it so fast! Looking forward to it dropping! XD

pfitzseb · 2019-01-09T09:53:24Z

Alright, new release is out!

NHDaly · 2019-01-10T04:32:19Z

Looks great!! :D Thanks again! :)

NHDaly added 2 commits December 28, 2018 13:34

Integrate Find with xterm.search!

ee9f33f

It currently only does `findNext`, always going forward every time you hit enter (even if you press shift-enter). It also always incorrectly reports just "1 result" (It must return at least one result to allow you to cycle through them.)

NHDaly changed the title ~~WIP: Find match in the Julia Console~~ WIP: Integrate the find-and-replace panel with the Julia Console Dec 28, 2018

Show "no result found" if zero matches

608d3a9

Revert "Show "no result found" if zero matches"

2aeef79

This reverts commit 608d3a9. It breaks searching for same-line words, which means it also breaks typing mutliple characters in a row for a long word.

terminal search UI

386e065

fix focus issues

375b333

NHDaly commented Jan 2, 2019

View reviewed changes

pfitzseb added 2 commits January 3, 2019 10:04

regex error handling

099d03b

and general code cleanup

update to xterm 3.10

141a74e

pfitzseb merged commit ca602ee into JunoLab:master Jan 3, 2019

NHDaly deleted the find-and-replace branch January 3, 2019 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Integrate the find-and-replace panel with the Julia Console #187

WIP: Integrate the find-and-replace panel with the Julia Console #187

NHDaly commented Dec 28, 2018 •

edited

pfitzseb commented Dec 28, 2018

NHDaly commented Dec 28, 2018 •

edited

pfitzseb commented Dec 28, 2018

NHDaly commented Dec 28, 2018

pfitzseb commented Dec 29, 2018 •

edited

NHDaly commented Jan 2, 2019

NHDaly left a comment

NHDaly Jan 2, 2019

NHDaly Jan 2, 2019

NHDaly Jan 2, 2019

NHDaly commented Jan 2, 2019 •

edited

pfitzseb commented Jan 3, 2019

pfitzseb commented Jan 3, 2019

NHDaly commented Jan 3, 2019

pfitzseb commented Jan 9, 2019

NHDaly commented Jan 10, 2019 •

edited

	import { CompositeDisposable, Emitter, TextEditor, TextBuffer, Disposable, MarkerLayer, Range } from 'atom'
	import { CompositeDisposable, Emitter } from 'atom'

WIP: Integrate the find-and-replace panel with the Julia Console #187

WIP: Integrate the find-and-replace panel with the Julia Console #187

Conversation

NHDaly commented Dec 28, 2018 • edited

pfitzseb commented Dec 28, 2018

NHDaly commented Dec 28, 2018 • edited

pfitzseb commented Dec 28, 2018

NHDaly commented Dec 28, 2018

pfitzseb commented Dec 29, 2018 • edited

NHDaly commented Jan 2, 2019

NHDaly left a comment

Choose a reason for hiding this comment

NHDaly Jan 2, 2019

Choose a reason for hiding this comment

NHDaly Jan 2, 2019

Choose a reason for hiding this comment

NHDaly Jan 2, 2019

Choose a reason for hiding this comment

NHDaly commented Jan 2, 2019 • edited

pfitzseb commented Jan 3, 2019

pfitzseb commented Jan 3, 2019

NHDaly commented Jan 3, 2019

pfitzseb commented Jan 9, 2019

NHDaly commented Jan 10, 2019 • edited

NHDaly commented Dec 28, 2018 •

edited

NHDaly commented Dec 28, 2018 •

edited

pfitzseb commented Dec 29, 2018 •

edited

NHDaly commented Jan 2, 2019 •

edited

NHDaly commented Jan 10, 2019 •

edited