925267 - Autocomplete recently typed values in filter input

Status: NEW

(DevTools :: Console, enhancement, P3)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Someone was asking for this in the Summit and I think it's a good idea. Users commonly filter console output for their own stuff and if they keep editing and refreshing their page, typing the filter again each time gets tedious fast.

We could present the last 3-4 filters in an autocompletion list when the input box receives focus, so that the user can quickly pick the desired one just using the arrow keys or the mouse.

Comment 1

[Mihai Sucan [:msucan]]

Good idea!

Can we do something like in html forms? Remember input and provide suggestions based on that, maybe with different UI (our own completion popup).

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

That's precisely the idea.

Comment 4

[migueluseche]

I'm the guy who talked with Pano, I'm interested in working on this bug, I can do it on December.

I would like to suggest a pin button to temporary save typed value until DevTools is closed or domains is changed. What do you think?

Comment 5

[Mihai Sucan [:msucan]]

Miguel, you are welcome to work on this bug - thank you! It might be best to start with enabling the filter input to remember user-typed values, and to offer those as autocomplete suggestions.

 We should keep the implementation simple, so there's no need to remember suggestions for every domain. Suggestions should be remembered across toolbox reopens, maybe even browser restarts (Panos, what do you think?).

Pinning values should be a different patch/bug because we try to keep patches small and easy to review. Thank you!

Comment 6

[migueluseche]

Hello, I started working but I have some questions.

* I'm using the autocomplete widget as someone recommend me in the IRC chat. Is this ok?
* When the values should be saved? Currently it saves terms when textbox's text changes (change event), so only when the textbox lost focus, textbox's current text will be saved to the suggestion list, is this behavior ok? Or how should it be saved.

Comment 7

[migueluseche]

Another aproach is to use https://developer.mozilla.org/en-US/docs/XUL/Textbox_%28Toolkit_autocomplete%29 but then i Will need to create a XPCOM and i believe it will harder, but looks better on the tools.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to migueluseche from comment #6)
> Hello, I started working but I have some questions.
> 
> * I'm using the autocomplete widget as someone recommend me in the IRC chat.
> Is this ok?

This should be fine.

> * When the values should be saved? Currently it saves terms when textbox's
> text changes (change event), so only when the textbox lost focus, textbox's
> current text will be saved to the suggestion list, is this behavior ok? Or
> how should it be saved.

The textbox change event sounds right. We need to play with it, to see if we need to change the event when we save the values.


(In reply to migueluseche from comment #7)
> Another aproach is to use
> https://developer.mozilla.org/en-US/docs/XUL/
> Textbox_%28Toolkit_autocomplete%29 but then i Will need to create a XPCOM
> and i believe it will harder, but looks better on the tools.

We should probably reuse the AutocompletePopup.


Thank you!

Comment 9

[Rob Campbell [:rc] (:robcee)]

(In reply to migueluseche from comment #6)
> * I'm using the autocomplete widget as someone recommend me in the IRC chat.
> Is this ok?

Yes!

> * When the values should be saved? Currently it saves terms when textbox's
> text changes (change event), so only when the textbox lost focus, textbox's
> current text will be saved to the suggestion list, is this behavior ok? Or
> how should it be saved.

Not sure I understand this question. You're trying to save the contents of the textbox on text change? You can just stash the value in a variable onchange and add it to your list? That sounds like what you're trying to do.

(In reply to migueluseche from comment #7)
> Another aproach is to use
> https://developer.mozilla.org/en-US/docs/XUL/
> Textbox_%28Toolkit_autocomplete%29 but then i Will need to create a XPCOM
> and i believe it will harder, but looks better on the tools.

I wouldn't recommend using that.

Comment 10

[migueluseche]

Attachment: WebConsoleAutocomplete.patch

Hi, here's the patch I've created. I followed your suggestion and here is the code I wrote, I would like some feedback about code organization, bugs, tips, etc.

I haven't write any test, Should i do them too? 

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Doesn't apply
User impact if declined: User will have to retype terms on the filter box
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Might affect filter functionality
String or IDL/UUID changes made by this patch: None

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 8392878 [details] [diff] [review]
WebConsoleAutocomplete.patch

Review of attachment 8392878 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start. Thanks for your patch!

I played with the patch this is nice. I noticed the following problems:

- sometimes if I type "foo" then I type it again and the suggestion popup shows "foo". Dont show a suggestion if it's identical to what I already typed.

- I expected to be able to accept the selected suggestion by pressing the Right arrow.

- I get incomplete suggestions showing up, things I typed halfway. I dont have yet clear steps to reproduce. I will test again with an updated patch.

You will need to write tests after we get this code working correctly. Thank you!

More comments below.

::: browser/devtools/webconsole/filter-suggestions.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const promise = require("sdk/core/promise");

If you need promises, please use the toolkit Promises.jsm implementation, which reports exceptions.

loader.lazyImporter(this, "promise", "resource://gre/modules/Promise.jsm", "Promise");

However, I see promises are not used in this script. As such, please remove this require() call.

@@ +15,5 @@
> +/**
> + * Adds a list of suggestions to web console's filter box.
> + *
> + * @constructor
> + * @param nsiInputElement aInputNode

I can't find any nsiInputElement interface. You should just say nsIDOMElement.

@@ +21,5 @@
> + *        attached and from where search input will be taken.
> + * @param nsIDOMDocument aDocument
> + *        The document you want the popup attached to.
> + * @param function filterFunction
> + *        The WebConsoleFrame instance where the filter is located

This parameter description doesnt seem accurate.

@@ +23,5 @@
> + *        The document you want the popup attached to.
> + * @param function filterFunction
> + *        The WebConsoleFrame instance where the filter is located
> + */
> +function FilterSuggestion(aInputNode, aDocument, filterFunction){

nit: space before {

@@ +27,5 @@
> +function FilterSuggestion(aInputNode, aDocument, filterFunction){
> +  this.filterFunction = filterFunction;
> +  this.filterBox = aInputNode;
> +
> +  // initialize variables.

nit: is this comment needed?

@@ +30,5 @@
> +
> +  // initialize variables.
> +  this.filteredTerms = [];
> +
> +  // bind!

nit: is this comment needed?

@@ +51,5 @@
> +  this.filterPopup = new AutocompletePopup(aDocument, options);
> +
> +  // event listeners.
> +  this.filterBox.addEventListener("blur", this._saveFilterTerm, false);
> +  this.filterBox.addEventListener("keypress", this._onFilterKeypress, true);

Any specific reason why the blur event listener is not capturing while the keypress listener is capturing?

@@ +63,5 @@
> +  _showPopup: function()
> +  {
> +    let suggestedTerms = this._getSuggestedTerms(this.filterBox.value);
> +
> +    if(suggestedTerms.length > 0 && this.filterBox.value.length > 0){

Please try to follow the coding style of the webconsole.

Also read https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

@@ +77,5 @@
> +  {
> +    let items = [];
> +    let termLength = this.filterBox.value.length;
> +
> +    for(let i=0; i < this.filteredTerms.length; i++)

nit: for (let term of this.filteredTerms) { ...

@@ +107,5 @@
> +   * Saves current term in the filter box for future suggestions
> +   * @param string term   
> +   *        The term to remove from the suggestion list
> +   */
> +  _removeFilterTerm: function(term)

nit: the "a" prefix for arguments is inconsistently used in this file. Better to just drop the prefix in all cases.

::: browser/devtools/webconsole/webconsole.js
@@ +497,5 @@
>      this.outputNode = doc.getElementById("output-container");
>      this.completeNode = doc.querySelector(".jsterm-complete-node");
>      this.inputNode = doc.querySelector(".jsterm-input-node");
>  
> +    //Filter's suggestion terms popup

nit: space after //

Comment 12

[migueluseche]

Attachment: Changes after first review and included test

Hello again, I've implemented all your suggestions except

> nit: for (let term of this.filteredTerms) { ...

Because the term is stored in the value and not in the key. But the rest of your suggestion were implemented. 

Also, I've added the test case. This is my first case and basically I load some items by testing onchange event and pressing enter, then i check the navigation on the popup, if when a user navigates on the popup it changes filterbox's text and finally the selection of terms.

Waiting for feedback of what to fix/add.

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 8402872 [details] [diff] [review]
Changes after first review and included test

Review of attachment 8402872 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch. Good progress! The new test is quite nice.


General comments:

- the Right arrow should accept the selected suggestion only when the cursor is at the end of the input value. If I move the cursor elsewhere, Right arrow key should work as expected.

- patch includes trailing whitespaces, please remove them.

- please follow a consistent coding style, see more comments below about this.


Sometimes suggestions are removed after using them. Steps o reproduce:

1. focus filter input.
2. type microsoft, then enter, then ctrl-backspace.
3. type mihai, then enter, then ctrl-backspace.
4. type mozilla, then enter, then ctrl-backspace.
5. type m, now press Up and Down arrows to cycle through the three suggestions available.
6. press Enter on 'microsoft', then ctrl-backspace.
7. type m, now you see only 'mihai' and 'mozilla': 'microsoft' is missing.

::: browser/devtools/webconsole/filter-suggestions.js
@@ +7,5 @@
> +loader.lazyGetter(this, "AutocompletePopup", 
> +      () => require("devtools/shared/autocomplete-popup").AutocompletePopup);
> +
> +// Maximum number of selector suggestions shown in the filter box.
> +const FS_MAX_SUGGESTIONS = 5;

s/FS_MAX_/MAX_/

@@ +20,5 @@
> + * @param nsIDOMDocument aDocument
> + *        The document where the suggestion popup list will be  
> + *        attached to.
> + * @param function aFilterFunction
> + *        The WebConsoleFrame instance where the filter is located.

You forgot to update this parameter description. I mentioned this in my previous review.

@@ +41,5 @@
> +    position: "before_start",
> +    direction: "ltr",
> +    theme: "auto",
> +    onClick: this._onListBoxKeypress,
> +    onKeypress: this._onListBoxKeypress

We should not combine the mouse click and keypress events into one event handler. This is prone to errors.

@@ +63,5 @@
> +        return;
> +
> +    let suggestedTerms = this._getSuggestedTerms();
> +
> +    if(suggestedTerms.length && this.filterBox.value.length){

In my previous review I also pointed out the coding style guidelines.

Please add a space after 'if' and a space before '{'. Do this for all of the file, and for the test. Also, please limit all lines to a max. of 80 characters.

@@ +79,5 @@
> +  {
> +    let items = [];
> +    let curTerm = this.filterBox.value;
> +
> +    for(let i=0; i < this.filteredTerms.length; i++)

for (let term of this.filteredTerms) {

@@ +83,5 @@
> +    for(let i=0; i < this.filteredTerms.length; i++)
> +    {
> +      let term = this.filteredTerms[i];
> +
> +      if(curTerm.length < term.length && curTerm == term.substr(0, curTerm.length))

if (term.startsWith(curTerm)) {

@@ +204,5 @@
> +        this.filterBox.value = this.popup.selectedItem.label;
> +        this.filterBox.focus();
> +        this.filterFunction();
> +        this.popup.hidePopup();
> +        this._removeFilterTerm(currentValue); // solves bug of storing term when clicking the label

What do you mean with the comment?

::: browser/devtools/webconsole/test/browser.ini
@@ +229,5 @@
>  [browser_webconsole_bug_821877_csp_errors.js]
>  [browser_webconsole_bug_837351_securityerrors.js]
>  [browser_webconsole_bug_846918_hsts_invalid-headers.js]
>  [browser_webconsole_bug_915141_toggle_response_logging_with_keyboard.js]
> +[browser_webconsole_bug_925267_filter_suggestions.js]

We are deprecating the use of bug numbers in test file names. Please remove 'bug_925627_'.

::: browser/devtools/webconsole/test/browser_webconsole_bug_925267_filter_suggestions.js
@@ +1,4 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

For tests we recommend the Public Domain license boilerplate, see https://www.mozilla.org/MPL/headers/

@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Tests that the autocompletion results contain the names of JSTerm helpers.

Please update the test description.

@@ +23,5 @@
> +  testSuggestionsLoad(aHud);
> +  testPopupSuggestions(aHud);
> +  testPopupNavigation(aHud);
> +  testPopupSelection(aHud);
> +  finishUp();

You can just call finishTest(), no need for the custom finishUp() function.

@@ +31,5 @@
> +{
> +  let filterTerms = ['http', 'https', 'ht', 'xml', 'div'];
> +  let filterBoxTerms = aHud.ui.filterPopup.filteredTerms;
> +
> +  for(var term in filterTerms){

for (let term of filterTerms) {

@@ +34,5 @@
> +
> +  for(var term in filterTerms){
> +    aHud.ui.filterBox.focus();
> +    aHud.ui.filterBox.value = '';
> +    EventUtils.sendString(filterTerms[term], aHud.ui.filterBox.ownerDocument.defaultView);

Instead of that long ...defaultView, you can simply use aHud.iframeWindow. (in all cases of the test)

@@ +77,5 @@
> +
> +  textbox.focus();
> +  textbox.value =  '';
> +
> +  for(var index in actions){

for-of again, see:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

Comment 14

[Mihai Sucan [:msucan]]

(In reply to migueluseche from comment #12)
> Created attachment 8402872 [details] [diff] [review]
> Changes after first review and included test
> 
> Hello again, I've implemented all your suggestions except
> 
> > nit: for (let term of this.filteredTerms) { ...
> 
> Because the term is stored in the value and not in the key. But the rest of
> your suggestion were implemented. 

for-of should give you the values of the array, not the keys. See the docs I linked.

Comment 15

[migueluseche]

Attachment: Changes after second revision

I've made all suggestion.  I coudn't reproduce the microsoft/mozilla/mihai bug, maybe because i changed the way it worked and it was fixed. 

About the nsIDOMDocument description, I've changed it to the same definition as autocomplete-popup.js because I don't know how to explain this in another way.

Test was also updated. 

Waiting for reviews..

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8416946 [details] [diff] [review]
Changes after second revision

Review of attachment 8416946 [details] [diff] [review]:
-----------------------------------------------------------------

Mihai is away for a while, so let's reassign this. Rob could you take a look or do you want me to?

Comment 17

[Rob Campbell [:rc] (:robcee)]

(In reply to Panos Astithas [:past] from comment #16)
> Comment on attachment 8416946 [details] [diff] [review]
> Changes after second revision
> 
> Review of attachment 8416946 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mihai is away for a while, so let's reassign this. Rob could you take a look
> or do you want me to?

I'm on it. Thanks.

Comment 18

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8416946 [details] [diff] [review]
Changes after second revision

Review of attachment 8416946 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking much better. Getting close.

I'd like to run this through our try servers to see how that test behaves.

Canceling review for a bit more cleanup. Let me know if there's anything I can do to help.

::: browser/devtools/webconsole/filter-suggestions.js
@@ +19,5 @@
> + *        current term.
> + * @param nsIDOMDocument aDocument
> + *        The document you want the popup attached to.
> + * @param function aFilterFunction
> + *        The WebConsoleFrame instance where the filter is located.

this still hasn't been updated. This parameter description should probably read:

A filter function rather than WebConsoleFrame instance.

@@ +41,5 @@
> +    position: "before_start",
> +    direction: "ltr",
> +    theme: "auto",
> +    onClick: this._onListBoxClick,
> +    onKeypress: this._onListBoxKeypress

good.

@@ +54,5 @@
> +
> +FilterSuggestion.prototype = {
> +
> +  /**
> +   * Handles key up event on filter box, opulates the suggestions list

nit: s/opulates/populates/?

@@ +90,5 @@
> +        });
> +
> +        if (items.length == MAX_SUGGESTIONS) break;
> +      }
> +    }

this looks much nicer now.

@@ +247,5 @@
> +   */
> +  destroy: function() {
> +    this.filterBox.removeEventListener("change", this._onFilterChange, false);
> +    this.filterBox.removeEventListener("keypress", this._onFilterKeypress, false);
> +    this.filterBox.removeEventListener("keyup", this._onKeyUp, false);

forgot to remove the "click" listener.

::: browser/devtools/webconsole/test/browser_webconsole_filter_suggestions.js
@@ +2,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +// Check that Dead Objects do not break the Web/Browser Consoles. See bug 883649.

is this comment valid?

appears to be copied from http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_console_dead_objects.js#6.

Please update it to reflect reality and reference this bug number.

@@ +6,5 @@
> +// Check that Dead Objects do not break the Web/Browser Consoles. See bug 883649.
> +// This test does:
> +// - opens the Browser Console,
> +// - loads some terms to the popup by typing some values
> +// - tests that terms are loaded ok and popup suggests correct terms 

supernit: trailing space in this line.

::: browser/devtools/webconsole/webconsole.js
@@ +25,5 @@
>  loader.lazyGetter(this, "ConsoleOutput",
>                    () => require("devtools/webconsole/console-output").ConsoleOutput);
>  loader.lazyGetter(this, "Messages",
>                    () => require("devtools/webconsole/console-output").Messages);
> +loader.lazyGetter(this, "FilterSuggestion", 

nit: extra trailing whitespace here to remove.

Comment 19

[migueluseche]

Attachment: Changes after third revision

I've attached an updated, sorry for the trailing spaces I needed to reconfigure my editor. I just have one last question, I'm adding the test under the macos section, I'm on linux and i cannot run the test on other section. But it should also work on Windows, so when mergin (if everything is ok) could you move it the test definition to the correct place at /webconsole/test/browser.ini ?

Comment 20

[Rob Campbell [:rc] (:robcee)]

(In reply to migueluseche from comment #19)
> Created attachment 8428386 [details] [diff] [review]
> Changes after third revision
> 
> I've attached an updated, sorry for the trailing spaces I needed to
> reconfigure my editor. I just have one last question, I'm adding the test
> under the macos section, I'm on linux and i cannot run the test on other
> section. But it should also work on Windows, so when mergin (if everything
> is ok) could you move it the test definition to the correct place at
> /webconsole/test/browser.ini ?

yep, we'll move that to the right place when it's ready to land.

Just letting you know I'll get to this review tomorrow. Thanks for the update.

Comment 21

[Rob Campbell [:rc] (:robcee)]

we've also got this: bug 992594 which is related, but not blocking.

Comment 22

[Rob Campbell [:rc] (:robcee)]

Played around with this a little bit and noticed that the suggestions don't persist across sessions.

We should probably save the most recent entries in a preference so a user has them always populated.

Otherwise the implementation looks good.

Comment 23

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8428386 [details] [diff] [review]
Changes after third revision

Review of attachment 8428386 [details] [diff] [review]:
-----------------------------------------------------------------

f+ but this should have some saved history in a preference as per my previous comment.

Comment 24

[Rob Campbell [:rc] (:robcee)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #23)
> Comment on attachment 8428386 [details] [diff] [review]
> Changes after third revision
> 
> Review of attachment 8428386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ but this should have some saved history in a preference as per my
> previous comment.

hey Skatox.

I didn't get to reply to you in IRC yesterday, so leaving some comments here.

You asked about how we should save these "options".

I recommend saving them in a preference, as a comma-separated string of filter terms.

E.g., devtools.console.filter.autocompletions = "error,CSS,some longer string," etc.

You can set and retrieve prefs with the Services.prefs.setPref/getPref();

Comment 25

[migueluseche]

Sorry for the long delay, I was really busy without any time. Gonna work on this again, also, I can save options every time a user uses it. But when should i delete them besides pressing Delete key at the suggestion box? 

I don't think it will good to save a lot of options, or is it?

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

The idea is to only save the last 4 searched terms. So the moment the user searches for a new (5th) term, it will automatically cause the oldest in the list to disappear. This is often referred to as a ring buffer.

Comment 27

[migueluseche]

Attachment: Changes after fourth revision

I've updated the source code, and now it saves the last 5 suggestions. Also I've updated the test, there 2 stuff that I don't know:

* The first time, because there no information stored at Services.prefs, it throws an error, I fixed it with:

try {
    this.filteredTerms = Services.prefs.getCharPref(this._PREFERENCE_KEY).split(",");
  } catch (e) {
    this.filteredTerms = [];
  }

But, I don't know if this is the correct way.

* Also, I couldn't find a way (lack of documentation) to make a test where the browsers closes and reopens. Is this necessary to do?

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8609917 [details] [diff] [review]
Changes after fourth revision

Review of attachment 8609917 [details] [diff] [review]:
-----------------------------------------------------------------

Don't worry about testing that the terms are persisted, there are other tests that exercise the prefs implementation. One issue I did observe is that adding new entries does not remove old entries from previous sessions. I think I spotted why when reading the patch, so my comments should help you fix this. All in all good progress!

::: browser/devtools/webconsole/filter-suggestions.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +// Maximum number of selector suggestions shown in the filter box.

Nit: "filter suggestions" (there is no other reference to "selector suggestions").

@@ +21,5 @@
> + *        The document you want the popup attached to.
> + * @param function aFilterFunction
> + *        Function to execute when selecting terms at filterbox.
> + */
> +function FilterAutocompletions(aDOMElement, aDocument, aFilterFunction) {

Nit: we no longer favor "aFoo" names in devtools and since this is a new file better change all function arguments to "foo".

Also, it will be more consistent with the file name and general nomenclature to call it FilterSuggestions instead of FilterAutocompletions.

@@ +26,5 @@
> +  this.filterBox = aDOMElement;
> +  this.filterFunction = aFilterFunction;
> +
> +  try {
> +    this.filteredTerms = Services.prefs.getCharPref(this._PREFERENCE_KEY).split(",");

If you add the pref to browser/app/profile/firefox.js with an initial value of an empty string, you wouldn't need to use try/catch:

pref("devtools.console.filter.suggestions", "[]");

I used suggestions instead of autocompletion above in order to be more consistent.

@@ +60,5 @@
> +exports.FilterAutocompletions = FilterAutocompletions;
> +
> +FilterAutocompletions.prototype =
> +{
> +  _PREFERENCE_KEY: "devtools.console.filter.autocompletions",

Nit: drop the underscore for constants

@@ +64,5 @@
> +  _PREFERENCE_KEY: "devtools.console.filter.autocompletions",
> +
> +  /**
> +   * Handles key up event on filter box, populates the suggestions list
> +   * and shows the suggestion popup

Nit: missing period here and in the following method comments.

@@ +67,5 @@
> +   * Handles key up event on filter box, populates the suggestions list
> +   * and shows the suggestion popup
> +   */
> +  _onKeyUp: function(aEvent)
> +  {

Nit: opening curly brace in the same line as "function" (here and elsewhere)

@@ +70,5 @@
> +  _onKeyUp: function(aEvent)
> +  {
> +    if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE ||
> +        aEvent.keyCode == aEvent.DOM_VK_RETURN)
> +        return;

Even single-statement bodies need curly braces.

@@ +83,5 @@
> +    }
> +  },
> +
> +  /**
> +  * Find suggested terms based on filter box value from all previus saved terms

Typo: previous

@@ +91,5 @@
> +    let items = [];
> +    let curTerm = this.filterBox.value;
> +
> +    for (let term of this.filteredTerms) {
> +      if (curTerm.length < term.length && term.startsWith(curTerm)) {

What's the point of the first condition? Isn't term.startsWith(curTerm) enough?

@@ +93,5 @@
> +
> +    for (let term of this.filteredTerms) {
> +      if (curTerm.length < term.length && term.startsWith(curTerm)) {
> +        items.push({
> +          preLabel: term.substr(0, curTerm.length),

What's wrong with "preLabel: curTerm,"?

@@ +97,5 @@
> +          preLabel: term.substr(0, curTerm.length),
> +          label: term
> +        });
> +
> +        if (items.length == MAX_SUGGESTIONS) break;

Why is this necessary if we are looping over an array of maximum this.filteredTerms.length, which we guarantee is never going to be more than MAX_SUGGESTIONS?

@@ +110,5 @@
> +   */
> +  _onFilterChange: function()
> +  {
> +    if ((this.filterBox.value.length) &&
> +        (this.filteredTerms.indexOf(this.filterBox.value) < 0))

Nit: the extra parens are redundant and the opening curly brace needs to be on the same line as the if.

@@ +114,5 @@
> +        (this.filteredTerms.indexOf(this.filterBox.value) < 0))
> +    {
> +      this.filteredTerms.push(this.filterBox.value);
> +
> +      let itemsToSave = this.filteredTerms.slice(-MAX_SUGGESTIONS).join(",");

Oh, so this is why you had that check for MAX_SUGGESTIONS above. This behavior makes the filter box remember an arbitrary number of terms, but only persist MAX_SUGGESTIONS, which seems counterintuitive. When I mentioned a ring buffer previously, I alluded to the fact that the field would always remember up to MAX_SUGGESTIONS terms, not only when persisting them.

So what you need to do is push() first, shift() if length == MAX_SUGGESTIONS and afterwards simply join().

@@ +123,5 @@
> +   * Saves current term in the filter box for future suggestions
> +   * @param string aTerm
> +   *        The term to remove from the suggestion list
> +   */
> +  _removeFilterTerm: function(aTerm)

The method comment is wrong.

@@ +125,5 @@
> +   *        The term to remove from the suggestion list
> +   */
> +  _removeFilterTerm: function(aTerm)
> +  {
> +    this.filteredTerms.splice(this.filteredTerms.indexOf(aTerm), 1);

This is dangerous: what if aTerm isn't in the array?

@@ +144,5 @@
> +      case aEvent.DOM_VK_ESCAPE:
> +        this.popup.hidePopup();
> +        break;
> +
> +      case aEvent.DOM_VK_RETURN:

Nit: no empty lines between cases please.

@@ +152,5 @@
> +        break;
> +
> +      case aEvent.DOM_VK_UP:
> +        if (this.popup.isOpen && this.popup.itemCount) {
> +          this.popup.focus();

Why move the focus to the popup? This way the user can't keep typing after hitting the up or down arrows.

@@ +157,5 @@
> +          if (this.popup.selectedIndex == this.popup.itemCount - 1) {
> +            this.popup.selectedIndex = Math.max(0, this.popup.itemCount - 2);
> +          } else {
> +            this.popup.selectedIndex = this.popup.itemCount - 1;
> +          }

This doesn't work correctly when you keep typing the up arrow until the bottom-most term is highlighted. It also relies on the popup having the focus, instead of manipulating it simply by modifying this.popup.selectedIndex, which you already do anyway. The math seems bogus as well, selectedIndex is always changed to 0 or itemCount - X, when both X and itemCount are constants.

The down arrow handler has the opposite problem. You can see it clearly when there are only 2 matches and you keep hitting up (or down). The filter box value never changes.

@@ +172,5 @@
> +        break;
> +
> +      case aEvent.DOM_VK_TAB:
> +      case aEvent.DOM_VK_RIGHT:
> +        let selItem = this.popup.getItemAtIndex(this.popup.itemCount - 1),

Bad math again: if selItem refers to the selected item, then it means it will always be the last one (itemCount -1).

@@ +213,5 @@
> +  },
> +   /**
> +   * Handles keypress on suggestions richlistbox.
> +   */
> +  _onListBoxKeypress: function(aEvent)

If you never focus the popup above, then you wouldn't need this handler, right? Why not consolidate all updates in a single handler?

::: browser/devtools/webconsole/test/browser_webconsole_filter_suggestions.js
@@ +167,5 @@
> +    textbox.value =  '';
> +    EventUtils.synthesizeKey("h", {}); //Displays previous loaded suggestions
> +    EventUtils.synthesizeKey("VK_UP", {});
> +    EventUtils.synthesizeKey(key, {});
> +    

Trailing whitespace.

Comment 29

[migueluseche]

Attachment: WebConsoleFilterSuggestion.patch

Sorry for the big delay, I've fixed all the stuff that you told me to do, also I've updated the patch with current fx-team branch and the test is update.

Comment 30

[migueluseche]

These are my comments about your feedback:

* All code lint problems were solved using eslint

> What's the point of the first condition? Isn't term.startsWith(curTerm)
> enough?
> 
* Yes, you were right with starsWith.. my mistake.

> @@ +93,5 @@
> > +
> > +    for (let term of this.filteredTerms) {
> > +      if (curTerm.length < term.length && term.startsWith(curTerm)) {
> > +        items.push({
> > +          preLabel: term.substr(0, curTerm.length),
> 
> What's wrong with "preLabel: curTerm,"?
> 
* I'm using the label like that to hightlight written letters and not the whole term.

> @@ +97,5 @@
> > +          preLabel: term.substr(0, curTerm.length),
> > +          label: term
> > +        });
> > +
> > +        if (items.length == MAX_SUGGESTIONS) break;
> 
> Why is this necessary if we are looping over an array of maximum
> this.filteredTerms.length, which we guarantee is never going to be more than
> MAX_SUGGESTIONS?
> 
> @@ +114,5 @@
> > +        (this.filteredTerms.indexOf(this.filterBox.value) < 0))
> > +    {
> > +      this.filteredTerms.push(this.filterBox.value);
> > +
> > +      let itemsToSave = this.filteredTerms.slice(-MAX_SUGGESTIONS).join(",");
> 
> Oh, so this is why you had that check for MAX_SUGGESTIONS above. This
> behavior makes the filter box remember an arbitrary number of terms, but
> only persist MAX_SUGGESTIONS, which seems counterintuitive. When I mentioned
> a ring buffer previously, I alluded to the fact that the field would always
> remember up to MAX_SUGGESTIONS terms, not only when persisting them.
> 
> So what you need to do is push() first, shift() if length == MAX_SUGGESTIONS
> and afterwards simply join().
* MAX_SUGGESTIONS is the maximum terms shown in the suggestions list, because filtered terms are all suggestions, there can be more than terms saved than the suggestions for the filter, in other words, once we have enough suggestions, there's no need to add more because it exceeds the maximum quantity of terms  to show.

> > +  _removeFilterTerm: function(aTerm)
> > +  {
> > +    this.filteredTerms.splice(this.filteredTerms.indexOf(aTerm), 1);
> 
> This is dangerous: what if aTerm isn't in the array?
* I've used an ES6 solution, is that OK?

> Why move the focus to the popup? This way the user can't keep typing after
> hitting the up or down arrows.............
>......... If you never focus the popup above, then you wouldn't need this handler,
> right? Why not consolidate all updates in a single handler
* Yes, I didn't know what I did. Just rewrote the logic of the key press and removed focus events.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8769434 [details] [diff] [review]
WebConsoleFilterSuggestion.patch

Redirecting to Brian.

Comment 32

[Brian Grinstead [:bgrins]]

Thanks for coming back around to this patch.  I'm afraid we will need to put this bug on hold though since we are in the middle of actively doing a redesign of the filter controls in the console which will make the current filter box go away.  That work is happening in https://github.com/devtools-html/gecko-dev/issues/101 but it's going to merge into m-c pretty soon.

Looks like this code is really well separated from the rest of the frontend so it could be ported to the new filter UI in the future. But we’re also currently reprioritizing new features in the console so I'd like to get some UX input to see if we'll want to add this feature before we do it.  Sorry this wasn't communicated earlier - I wasn't aware of the work going on in this bug

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8769434 [details] [diff] [review]
WebConsoleFilterSuggestion.patch

Review of attachment 8769434 [details] [diff] [review]:
-----------------------------------------------------------------

See comment 32

Comment 34

[migueluseche]

Do you think it would be worthwhile to update this patch to new Developers Tools?

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

Hello Miguel!
We were talking about this this week.

The patch was written for the old frontend, which was completely deleted, so I think it would make more sense to completely rewrite it from scratch rather than try to rebase it.

Also, some people want to have "Search", as opposed to "Filter".
We may want to keep both, but the Filter input might move to another place, next to the filter buttons ("all", "error", …), where one can creates its own filters. Then they can be re-enabled easily.

Does it make sense to you?

We plan to work on this at the beginning of 2019, would you want to help us?

Comment 36

[migueluseche]

I see, yes I would like to help! I couldn't finish this so I would like to do it as a personal goal. My time is a little bit limited but I'll try to do my best. 

I don't get how different would be a Search Option from Filter.

Comment 37

[Nicolas Chevobbe [:nchevobbe]]

Hi Miguel :)

> I see, yes I would like to help! I couldn't finish this so I would like to do it as a personal goal. My time is a little bit limited but I'll try to do my best. 

No worries, this is not something urgent, so take your time (or don't do it at all if you don't have the time to :) )

> I don't get how different would be a Search Option from Filter.

Filter hides messages in the console, Searching still displays them all, but you can navigate from match to match (like Ctrl+F in Firefox basically).

Comment 38

[Patrick Brosset <:pbro>]

Hi Miguel,
Are you still interested in working on this bug?
I see it is assigned to you now but hasn't been updated for 2 months.
If you need more time, or help, no worries, let's keep it assigned and feel free to reach out to Nicolas for help.
If, however, you don't think you'll be working on this bug, please let me know so I can unassign you from it and so it becomes available to others.
Thanks!

Comment 39

[Paarmita Bhargava]

Hey Patrick,
I would like to work on this issue, if none is working.
Can you assign it to me?

Comment 40

[Patrick Brosset <:pbro>]

Hi Paarmita. Seems like Miguel isn't going to work on this after all. So I'll assign this to you.

Comment 41

[Paarmita Bhargava]

Hey Patrick,
Thanks for assigning.
As I am not very familiar with the codebase so can you please help me in finding the files and understand the problem better.
Thanks in advance.

Comment 42

[Patrick Brosset <:pbro>]

Hi Paarmita, sure, let me pass the needinfo on to Nicolas who will be better equipped than me to answer this question.

Comment 43

[migueluseche]

Hello, sorry I was moving to another country. No problem if I lost the bug, I'm here to offer any help.

Comment 44

[Nicolas Chevobbe [:nchevobbe]]

Paarmita, you have 5 other bugs assigned to you, and maybe this one would be a bit too challenging for you.
Would you mind if I give it back to Miguel (if you want to Miguel!)

Comment 45

[migueluseche]

Yes, I would like to implement it for the new Devtools

Comment 46

[Nicolas Chevobbe [:nchevobbe]]

It's yours Miguel :)

Comment 47

[migueluseche]

Thanks, is there any other component where I can look to have an idea on how to implement a search?

To confirm, I'll save recent search values so it appear in the searchbox. right?

Comment 48

[Patrick Brosset <:pbro>]

I'll add a needinfo to Nicolas for this question to make sure he sees it.

Comment 49

[Nicolas Chevobbe [:nchevobbe]]

Hello Miguel, thank you for your patience.

So the webconsole now uses the SearchBox component (devtools/client/webconsole/components/FilterBar.js#250-254 ), which should have all we need to display autocompletion items.

You can take some inspiration on how it's done in the Netmonitor for example (See devtools/client/netmonitor/src/components/Toolbar.js#368 ).

I think we'll need to modify the SearchBox component to automatically show the last 4 entries when it is focused.

Then, we need to save the filter list in some storage so we can reuse it.
We also need to decide when we should put something in the stored list (we can't do that on every key stroke).

Let me know if you need more information to start working on this, thanks!

Comment 50

[Patrick Brosset <:pbro>]

No activity for 4 months. Unassigning this bug. Anyone should feel free to claim it and resume the work here.

Comment 51

[Pranav pandey]

Hi I want to try so the main challenge with this is about , when will be storing and where will be storing it ,what i have in mind is to store them as comma separated values and some pref. which will be stored in variable during lifespan of component

componentDidMount() { this.filteredTerms = Services.prefs.getCharPref(this.PREFERENCE_KEY).split(","); } componentWillUnmount() { Services.prefs.setCharPref(this.PREFERENCE_KEY, this.filteredTerms.join(",")); }
now about updating this.filteredTerms we can use debounce in combination with filterText.split(/\s+/g);
debounce((text) => { if(text == "") return; if(text.split(/\s+/g).length != this.filteredTerms[this.filteredTerms.length -1 ].split(/\s+/g); { this.filteredTerms.shift() this.filteredTems.push(text) } else { this.filteredTerms[this.filteredTerms.length -1 ] = text; } }, 500)

we can limit the length of filteredTerm to 5 what do you say

Comment 52

[Adam Hammad]

Hey Nicolas,

I have a solution for this issue too, are you able to assign this to me?

Best,
Adam

Comment 53

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Adam Hammad from comment #52)

Hey Nicolas,

I have a solution for this issue too, are you able to assign this to me?

Best,
Adam

sure, thanks :)

Comment 54

[Adam Hammad]

Attachment: Bug 925267 - Implement search history with autocomplete on the Console filter bar. r=nchevobbe

Comment 55

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nicolas, could you please help Adam with the patch?
Honza

Comment 56

[Nicolas Chevobbe [:nchevobbe]]

reviewed the patch

Comment 57

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:nchevobbe, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 58

[migueluseche]

hello, if you need help with this I can help. I've free time to work on a new patch if needed.

Comment 59

[migueluseche]

Hi, any status about this bug? I would like to contribute on this.

Comment 60

[Nicolas Chevobbe [:nchevobbe]]

Hello Miguel, sorry I missed your previous message, I'm assigning the bug to you
I think you can try to start from what was done in https://phabricator.services.mozilla.com/D68006

Comment 61

[Julian Descottes [:jdescottes]]

Just a quick flyby comment, I see the preference in the current patch is currently handled as a char pref, I feel like for this kind of content, a string pref will be safer.

Comment 62

[migueluseche]

Attachment: Bug 925267 - [devtools] Autocomplete recently typed values in filter input. r=nchevobbe

Comment 63

[migueluseche]

Hello,

I've sent a new patch at https://phabricator.services.mozilla.com/D155931

It's based on the work made by @adamham but I made some minor changes:

• Changed pref to string as @jdescottes recommended.
• Fixed selection bug from previous patch.
• Fixed term deletion bug from previous patch.

Also, in order to make it work. On SearchBox component I need to trigger the search. I copied the same code from onChange event. But now the timeout code to initiate the search it's repeated. Do I need to make a function to search and call it from onChange and onSelect events?
.
This is the repated code is located at devtools/client/shared/components/SearchBox.js line 160.

Comment 64

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 65

[migueluseche]

Hi @nchevobbe! somehow I didn't got that notification. Seems like I got removed from this bug, What's the final status on this? Do I need to add stuff from 1801877?

Comment 66

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 67

[migueluseche]

Hi, I just wanna say that I've time to work in this issue. I've learned more about React and I'm in parental leave so I can finish this.

My only question is where could I save the limit of suggested terms? seems like the place for storage settings changed.