Initial Implementation of Autocomplete #77

dnachum · 2018-09-16T07:31:57Z

Closes #74

Initially the autocomplete feature is off until it is toggled with (autocomplete) but the window should save whether autocomplete was toggled so students won't have to keep enabling/disabling it.

The way I intended for this to work was for words to be displayed that the user could be typing. Once there is only one possible word left, the docs for that procedure is displayed. Also, for longer words, you can press tab to autocomplete the word that was being typed.

There are some things that can definitely be improved from this point. One thing is to maybe make the matching more sophisticated (right now I only match the prefixes of the word that is being typed with the possible words that can be displayed). Another thing is for something like define to not have the autocomplete box disappear when typing the function name and arguments.

into autocomplete

fixed the formatting in the autobox

added some comments

The autocomplete box does show up unless there exists a ( in front of the word

…c to show

… being currently typed vs one that is already complete

jathak · 2018-09-19T00:38:48Z

Great work on this! I started my new job yesterday, so my time's more limited now, but I'll try to get this reviewed this weekend

jathak

I've left some comments on this, mostly style-related. Feel free to disagree with my suggestions if you have a good reason for leaving them as is (just reply to the particular comment(s) letting me know).

One addition I'd recommend would be to add something about this feature to the Other Useful Commands section of the opening message. You could even add a button that users could click to enable it.

Eventually, I'd like to have this turned on by default, but I think we need some special casing around certain special forms before we do that (this should be done in a separate PR though).

I link to sections in Effective Dart a few times in my review. You don't need to take it as gospel, but I recommend reading through it and keeping it in mind when working. Feel free to correct my code to conform with it too, as there are definitely some places (particularly in older sections) where I don't.

jathak · 2018-09-22T02:11:30Z

lib/src/web_ui/code_input.dart

 class CodeInput {
  Element element;
+  //Create an element that contains possible autcomplete options


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)

jathak · 2018-09-22T02:14:16Z

lib/src/web_ui/code_input.dart


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

-  CodeInput(Element log, this.runCode, {this.parenListener}) {
+  CodeInput(Element log, this.runCode, Frame env, {this.parenListener}) {


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.

jathak · 2018-09-22T02:17:31Z

lib/src/web_ui/code_input.dart

 class CodeInput {
  Element element;
+  //Create an element that contains possible autcomplete options
+  Element _autoBox;
+  Element _wrapperAutoBox;


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

I think _autoBoxWrapper is cleaner here.

jathak · 2018-09-22T02:28:26Z

lib/src/web_ui/repl.dart

@@ -38,6 +37,12 @@ class Repl {
    status = SpanElement()..classes = ['repl-status'];
    container.append(status);
    buildNewInput();
+    if (window.localStorage.containsKey('autocomplete')) {


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.

jathak · 2018-09-22T02:33:24Z

lib/src/web_ui/repl.dart

@@ -38,6 +37,12 @@ class Repl {
    status = SpanElement()..classes = ['repl-status'];
    container.append(status);
    buildNewInput();
+    if (window.localStorage.containsKey('autocomplete')) {
+      String val = window.localStorage['autocomplete'];
+      if (val == 't') {


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.

jathak · 2018-09-24T04:11:09Z

lib/src/web_ui/code_input.dart

+        } else if (nextOpen == -1 || nextClose == -1 || nextOpen < nextClose) {
+          //Assuming the word is right after the parens
+          List splitRef =
+              refLine.substring(strIndex + 1).split(RegExp("[ ()]+"));


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.

jathak · 2018-09-24T04:11:44Z

lib/src/web_ui/code_input.dart

+    //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("[(]+"));


This regex should probably be commented too

jathak · 2018-09-24T04:19:14Z

lib/src/web_ui/code_input.dart

+    //Find the last word that was being typed
+    int currLength = 0;
+    List output = _currOp(element.text, cursorPos);
+    List output2 = _currOp(element.text, cursorPos, 2);


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

jathak · 2018-09-24T04:20:02Z

lib/src/web_ui/code_input.dart

+    int currLength = 0;
+    List output = _currOp(element.text, cursorPos);
+    List output2 = _currOp(element.text, cursorPos, 2);
+    String findMatch = output[0];


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

jathak · 2018-09-24T04:24:25Z

lib/src/web_ui/code_input.dart

+    List output = _currOp(element.text, cursorPos);
+    List output2 = _currOp(element.text, cursorPos, 2);
+    String findMatch = output[0];
+    bool fullWord = output[1];


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

isFullWord would probably be better here.

dnachum · 2018-09-24T05:36:13Z

Thanks for reviewing this! I'll be sure to get working on these things this week and I'll also read through Effective Dart.

…pen parentheses

…n operation

dnachum · 2018-10-17T19:15:25Z

Sorry it took me so long to get to this. I think I fixed most of the styling issues and also added a blurb about it in the "Other Useful Commands" section.

jathak

LGTM, aside from the one nit. Feel free to squash and merge once that's fixed. Great work!

jathak · 2018-10-21T15:54:32Z

lib/src/web_ui/code_input.dart

@@ -14,22 +17,35 @@ const List<SchemeSymbol> noIndentForms = [
  SchemeSymbol("define-macro")
 ];

+bool _isEnabledAutocomplete = false;


Nit: _isAutocompleteEnabled is probably a better name here

dnachum · 2018-10-22T07:02:52Z

Thanks for reviewing this! I'll try to work on getting this to work better for some of the special forms.

dnachum added 23 commits August 23, 2018 11:24

Added autocomplete box that will display options

ecde679

hide and display the autocomplete box

a902410

add the matching strings to the autocomplete box

0266595

fixed bug when adding new lines

523caf3

added matching prefixes being bolded

b95a844

Merge branch 'master' of https://github.com/Cal-CS-61A-Staff/dart_scheme

5b8313f

into autocomplete

_wordMatches now compares all documented forms

d1766ca

fixed the width and made the height of the autocomplete box variable

f0245e0

fixed the formatting in the autobox

fixed border around words in autobox

c0d5ea5

Make the autobox invisible when displaying docs

569f2ba

added some comments

docs stay up after one space to handle cases like cons vs cons-stream

1f5f0fd

The autocomplete box does show up unless there exists a ( in front of the word

now looks at the last symbol next to an open parens to decide what do…

de7ac76

…c to show

fixed autobox disappearing on empty parens

324e280

rewrote and cleaned up the _currOp function

238618c

finds both the last op and the second to last op

54a499f

changed the logic for determining if an operation represents a string…

0d426a9

… being currently typed vs one that is already complete

wrapped the autobox with a render element

a361b10

removed the wrapper autBox instead of just the child nodes

86f01fd

allow autocomplete to be toggled on and off

7e50b67

autocomplete toggle remembered in different sessions

40aab92

will complete word on tab if the docs for that word are displayed

f09f963

fixed spelling error with local storage of autocomplete preference

2b66225

scroll the autobox into view if it expands below the current window

8ffb398

jathak requested changes Sep 24, 2018

View reviewed changes

dnachum added 4 commits October 16, 2018 17:16

made some syntax/naming fixes

de1031b

cleared up stuff with local storage

126f612

cleaned up comment formatting and removed unnecesary code

68d95fd

Used Tuple2 instead of a generic List

d2f762d

dnachum added 4 commits October 16, 2018 18:58

changed some variable names

a4fdd3a

fixed some comments and fixed bug when there exists a space after a o…

3fc75ba

…pen parentheses

fixed some variable names and cleared up some comments

49e1f90

added dependency and fixed an error with a space before the name of a…

a21f8d3

…n operation

jathak approved these changes Oct 21, 2018

View reviewed changes

fixed boolean variable naming

c1e48d1

dnachum merged commit 95f6b11 into master Oct 22, 2018

dnachum deleted the autocomplete branch March 30, 2019 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Implementation of Autocomplete #77

Initial Implementation of Autocomplete #77

dnachum commented Sep 16, 2018

jathak commented Sep 19, 2018

jathak left a comment

jathak Sep 22, 2018

jathak Sep 22, 2018

jathak Sep 22, 2018

jathak Sep 22, 2018

jathak Sep 22, 2018

jathak Sep 24, 2018

jathak Sep 24, 2018

jathak Sep 24, 2018

jathak Sep 24, 2018

jathak Sep 24, 2018

dnachum commented Sep 24, 2018

dnachum commented Oct 17, 2018

jathak left a comment

jathak Oct 21, 2018

dnachum commented Oct 22, 2018

Initial Implementation of Autocomplete #77

Initial Implementation of Autocomplete #77

Conversation

dnachum commented Sep 16, 2018

jathak commented Sep 19, 2018

jathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnachum commented Sep 24, 2018

dnachum commented Oct 17, 2018

jathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnachum commented Oct 22, 2018