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

Initial Implementation of Autocomplete #77

Merged
merged 32 commits into from
Oct 22, 2018
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ecde679
Added autocomplete box that will display options
dnachum Aug 23, 2018
a902410
hide and display the autocomplete box
dnachum Aug 23, 2018
0266595
add the matching strings to the autocomplete box
dnachum Aug 23, 2018
523caf3
fixed bug when adding new lines
dnachum Aug 24, 2018
b95a844
added matching prefixes being bolded
dnachum Aug 24, 2018
5b8313f
Merge branch 'master' of https://github.com/Cal-CS-61A-Staff/dart_sch…
dnachum Aug 26, 2018
d1766ca
_wordMatches now compares all documented forms
dnachum Aug 26, 2018
f0245e0
fixed the width and made the height of the autocomplete box variable
dnachum Aug 26, 2018
c0d5ea5
fixed border around words in autobox
dnachum Aug 26, 2018
569f2ba
Make the autobox invisible when displaying docs
dnachum Aug 26, 2018
1f5f0fd
docs stay up after one space to handle cases like cons vs cons-stream
dnachum Aug 26, 2018
de7ac76
now looks at the last symbol next to an open parens to decide what do…
dnachum Sep 1, 2018
324e280
fixed autobox disappearing on empty parens
dnachum Sep 1, 2018
238618c
rewrote and cleaned up the _currOp function
dnachum Sep 3, 2018
54a499f
finds both the last op and the second to last op
dnachum Sep 3, 2018
0d426a9
changed the logic for determining if an operation represents a string…
dnachum Sep 5, 2018
a361b10
wrapped the autobox with a render element
dnachum Sep 5, 2018
86f01fd
removed the wrapper autBox instead of just the child nodes
dnachum Sep 9, 2018
7e50b67
allow autocomplete to be toggled on and off
dnachum Sep 9, 2018
40aab92
autocomplete toggle remembered in different sessions
dnachum Sep 9, 2018
f09f963
will complete word on tab if the docs for that word are displayed
dnachum Sep 9, 2018
2b66225
fixed spelling error with local storage of autocomplete preference
dnachum Sep 16, 2018
8ffb398
scroll the autobox into view if it expands below the current window
dnachum Sep 19, 2018
de1031b
made some syntax/naming fixes
dnachum Oct 17, 2018
126f612
cleared up stuff with local storage
dnachum Oct 17, 2018
68d95fd
cleaned up comment formatting and removed unnecesary code
dnachum Oct 17, 2018
d2f762d
Used Tuple2 instead of a generic List
dnachum Oct 17, 2018
a4fdd3a
changed some variable names
dnachum Oct 17, 2018
3fc75ba
fixed some comments and fixed bug when there exists a space after a o…
dnachum Oct 17, 2018
49e1f90
fixed some variable names and cleared up some comments
dnachum Oct 17, 2018
a21f8d3
added dependency and fixed an error with a space before the name of a…
dnachum Oct 17, 2018
c1e48d1
fixed boolean variable naming
dnachum Oct 22, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
131 changes: 130 additions & 1 deletion lib/src/web_ui/code_input.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import 'dart:async';
import 'dart:html';

import 'package:cs61a_scheme/cs61a_scheme.dart';
import 'package:cs61a_scheme/cs61a_scheme_web.dart';

import 'highlight.dart';

Expand All @@ -14,25 +15,40 @@ const List<SchemeSymbol> noIndentForms = [
SchemeSymbol("define-macro")
];

bool _enableAutocomplete = false;

class CodeInput {
Element element;
//Create an element that contains possible autcomplete options
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure what this comment is describing exactly. If there's a better phrasing, then use that; otherwise I think it's fine to omit this comment.

Also, style-wise, it's preferred to have a space between // or /// and the actual start of the comment (e.g. // Create)

Element _autoBox;
Element _wrapperAutoBox;
Copy link
Owner

Choose a reason for hiding this comment

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

Effective Dart advises that the most descriptive noun should generally go last in a variable name.

I think _autoBoxWrapper is cleaner here.

Map<String, Docs> wordToDocs;
bool _active = true;
final List<StreamSubscription> _subs = [];
String tabComplete = "";

Function(String code) runCode;
Function(int parens) parenListener;

CodeInput(Element log, this.runCode, {this.parenListener}) {
CodeInput(Element log, this.runCode, Frame env, {this.parenListener}) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to keep CodeInput separate from the interpreter, so I'd rather not have a Frame passed in here.

I'd recommend calling allDocumentedForms in the Repl and passing wordToDocs in here directly.

element = SpanElement()
..classes = ['code-input']
..contentEditable = 'true';
_autoBox = DivElement()
..classes = ["docs"]
..style.visibility = "hidden";
_wrapperAutoBox = DivElement()
..classes = ["render"]
..append(_autoBox);
_subs.add(element.onKeyPress.listen(_onInputKeyPress));
_subs.add(element.onKeyDown.listen(_keyListener));
_subs.add(element.onKeyUp.listen(_keyListener));
log.append(element);
log.append(_wrapperAutoBox);
element.focus();
parenListener ??= (_) => null;
parenListener(missingParens);
wordToDocs = allDocumentedForms(env);
}

String get text => element.text;
Expand All @@ -49,11 +65,15 @@ class CodeInput {
void deactivate() {
_active = false;
element.contentEditable = 'false';
_wrapperAutoBox.remove();
for (var sub in _subs) {
sub.cancel();
}
}

void enableAutocomplete() => _enableAutocomplete = true;
void disableAutocomplete() => _enableAutocomplete = false;

Future highlight(
{bool saveCursor = false, int cursor, bool atEnd = false}) async {
if (saveCursor) {
Expand Down Expand Up @@ -101,6 +121,43 @@ class CodeInput {
parenListener(missingParens);
}

/// Determines the operation at the last open parens
List _currOp(String inputText, int position, [int fromLast = 1]) {
Copy link
Owner

Choose a reason for hiding this comment

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

It's usually best not to use a list to return multiple values of different types, since we lose type checking.

I'm okay with it if we include a detailed description of the two values (including types) in the doc comment, but I think the better way to go would be to use something like the tuple package

List<String> splitLines = inputText.substring(0, position).split("\n");
//The first item indicates the word that was matched, the second item indicates
Copy link
Owner

Choose a reason for hiding this comment

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

What do first and second item refer to here? If it's the return values, this should be described in the doc comment above the function, not inline.

//if that word is the full string(true) or is in progress of being written out(false)
bool multipleLines = false;
String refLine;
int totalMissingCount = 0;

for (refLine in splitLines.reversed) {
totalMissingCount += countParens(refLine) ?? 0;
// Find the first line with an open paren but no close paren
if (totalMissingCount >= fromLast) break;
multipleLines = true;
}
//if there are not enough open parentheses, return the default output value
if (totalMissingCount >= fromLast) {
int strIndex = refLine.indexOf("(");
while (strIndex != -1 && strIndex < (refLine.length - 1)) {
int nextClose = refLine.indexOf(")", strIndex + 1);
int nextOpen = refLine.indexOf("(", strIndex + 1);
// Find the open paren that corresponds to the missing closed paren
if (totalMissingCount > fromLast) {
totalMissingCount -= 1;
} else if (nextOpen == -1 || nextClose == -1 || nextOpen < nextClose) {
//Assuming the word is right after the parens
List splitRef =
refLine.substring(strIndex + 1).split(RegExp("[ ()]+"));
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure what this regular expression is actually doing. I recommend including a comment describing the meaning whenever you use all but the most trivial regexes, since they are notoriously difficult to understand.

//Determine if op represents the full string
return [splitRef[0], splitRef.length > 1 || multipleLines];
}
strIndex = nextOpen;
}
}
return ["", true];
}

/// Determines how much space to indent the next line, based on parens
int _countSpace(String inputText, int position) {
List<String> splitLines = inputText.substring(0, position).split("\n");
Expand Down Expand Up @@ -145,8 +202,75 @@ class CodeInput {
return strIndex + 2;
}

///Find the list of words that contain the same prefix as currWord
List<String> _wordMatches(String currWord, bool fullWord) {
List<String> matchingWords = [];
int currLength = currWord.length;
for (String schemeWord in wordToDocs.keys) {
if (((schemeWord.length > currWord.length) && !fullWord) ||
schemeWord.length == currWord.length) {
if (schemeWord.substring(0, currLength) == currWord) {
Copy link
Owner

Choose a reason for hiding this comment

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

The declared name in line 208 isn't really much shorter and doesn't save any repeated work (plus we already use currWord.length in the two lines above), so just replace currLength with currWord.length here and eliminate the declaration.

matchingWords.add(schemeWord);
}
}
}
return matchingWords;
}

///Finds and displays the possible words that the user may be typing
void _autocomplete() {
//Find the text to the left of where the typing cursor currently is
int cursorPos = findPosition(element, window.getSelection().getRangeAt(0));
List<String> inputText =
element.text.substring(0, cursorPos).split(RegExp("[(]+"));
Copy link
Owner

Choose a reason for hiding this comment

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

This regex should probably be commented too

List<String> matchingWords = [];
//Find the last word that was being typed
int currLength = 0;
List output = _currOp(element.text, cursorPos);
List output2 = _currOp(element.text, cursorPos, 2);
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a comment explaining the difference between output and output2?

String findMatch = output[0];
Copy link
Owner

Choose a reason for hiding this comment

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

Noun phrases are generally preferred for non-boolean variables.
A better name would perhaps be foundMatch or just match

bool fullWord = output[1];
Copy link
Owner

Choose a reason for hiding this comment

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

Likewise, Dart prefers that booleans are named with a form of "to be" most of the time.

isFullWord would probably be better here.

if (findMatch.isEmpty) {
findMatch = output2[0];
fullWord = output2[1];
}
if (findMatch.isNotEmpty && inputText.length > 1) {
matchingWords = _wordMatches(findMatch, fullWord);
currLength = findMatch.length;
}
//Clear whatever is currently in the box
_autoBox.children = [];
_autoBox.classes = ["docs"];
if (matchingWords.isEmpty) {
//If there are no matching words, hide the autocomplete box
tabComplete = "";
_autoBox.style.visibility = "hidden";
} else if (matchingWords.length == 1) {
//If there is only one matching word, display the docs for that word
render(wordToDocs[matchingWords.first], _autoBox);
_autoBox.style.visibility = "hidden";
_autoBox.children.last.style.visibility = "visible";
tabComplete = matchingWords.first.substring(currLength);
} else {
tabComplete = "";
//Add each matching word as its own element for formatting purposes
for (String match in matchingWords) {
_autoBox.append(SpanElement()
..classes = ["autobox-word"]
//Bold the matching characters
..innerHtml =
"<strong>${match.substring(0, currLength)}</strong>${match.substring(currLength)}");
}
_autoBox.style.visibility = "visible";
}
_autoBox.scrollIntoView();
}

_keyListener(KeyboardEvent event) async {
int key = event.keyCode;
if (_enableAutocomplete) {
_autocomplete();
}
if (key == KeyCode.BACKSPACE) {
await delay(0);
parenListener(missingParens);
Expand All @@ -155,6 +279,11 @@ class CodeInput {
await delay(0);
parenListener(missingParens);
await highlight(saveCursor: true);
} else if (key == KeyCode.TAB) {
event.preventDefault();
int cursor = findPosition(element, window.getSelection().getRangeAt(0));
element.appendText(tabComplete);
await highlight(cursor: cursor + tabComplete.length + 1);
}
}
}
24 changes: 21 additions & 3 deletions lib/src/web_ui/repl.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class Repl {
int historyIndex = -1;

final String prompt;

Repl(this.interpreter, Element parent, {this.prompt = 'scm> '}) {
if (window.localStorage.containsKey('#repl-history')) {
var decoded = json.decode(window.localStorage['#repl-history']);
Expand All @@ -38,6 +37,12 @@ class Repl {
status = SpanElement()..classes = ['repl-status'];
container.append(status);
buildNewInput();
if (window.localStorage.containsKey('autocomplete')) {
Copy link
Owner

Choose a reason for hiding this comment

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

To maintain consistency with the rest of the project, prefix this property with #

Background:
The old interpreter had some data stored under user-chosen properties. I needed a way to keep the user from modifying the interpreter's own internal state, so I prefixed all of my properties with a #, since the character is not allowed in Scheme symbols. I then carried this practice over to the new interpreter.

Eventually (probably as part of #48), I'd like to set up some sort of proper namespacing for local storage (or maybe just switch to IndexedDB), but for now, I'd like to keep new interpreter state properties consistent.

String val = window.localStorage['autocomplete'];
if (val == 't') {
Copy link
Owner

Choose a reason for hiding this comment

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

Since we can't use actual booleans here due to limitations of localStorage, I'd prefer to use more descriptive properties here like disabled and enabled.

activeInput.enableAutocomplete();
}
}
interpreter.logger = (logging, [newline = true]) {
var box = SpanElement();
activeLoggingArea.append(box);
Expand Down Expand Up @@ -79,6 +84,19 @@ class Repl {
autodraw = false;
return undefined;
}, 0);
addBuiltin(env, const SchemeSymbol('autocomplete'), (_a, _b) {
logText('While typing, will display a list of possible procedures.\n'
'(disable-autocomplete) to disable\n');
activeInput.enableAutocomplete();
window.localStorage['autocomplete'] = 't';
return undefined;
}, 0);
addBuiltin(env, const SchemeSymbol('disable-autocomplete'), (_a, _b) {
logText('Autocomplete disabled\n');
activeInput.disableAutocomplete();
window.localStorage['autocomplete'] = 'f';
return undefined;
}, 0);
}

buildNewInput() {
Expand All @@ -90,8 +108,8 @@ class Repl {
..text = prompt
..classes = ['repl-prompt'];
container.append(activePrompt);
activeInput =
CodeInput(container, runCode, parenListener: updateParenStatus);
activeInput = CodeInput(container, runCode, interpreter.globalEnv,
parenListener: updateParenStatus);
container.scrollTop = container.scrollHeight;
}

Expand Down
5 changes: 5 additions & 0 deletions web/assets/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ body {
}
}

.autobox-word {
margin: 10px;
display: inline-block;
}

.code-input {
display: inline-block;
}
Expand Down