(In reply to Bastian Ilsø from comment #0)
> picture showing a less destructive way to approach unknown commands.

I like it!

(In reply to Bastian Ilsø from comment #0)
> Created attachment 313034 [details]
> picture showing a less destructive way to approach unknown commands.
> 
> I just typed a longer "/me" message and then the whole message was removed
> because i forgot a space between /me and the word coming after that. It
> would be more user-friendly if we select the whole chat message and possibly

+1

I just wrote a patch for that :)

> style the entry with ".error" . Then the user can either change the command
> if he typoed it or remove the message by just pressing delete or backspace.
> See attached screenshot proposal.

There is particular problem on attaching style, When do we remove the style? There is no clear event when we should remove the error style. I must be missing something, please point it out.

(In reply to Kunaal Jain from comment #2)
> +1
> 
> I just wrote a patch for that :)

Awesome!

> There is particular problem on attaching style, When do we remove the style?
> There is no clear event when we should remove the error style. I must be
> missing something, please point it out.

Remove the style again once a change is made to the text in the entry (so it turns blue when either the whole text was removed or the command was fixed).

Created attachment 313402 [details] [review]
Select text and show styled error

Review of attachment 313402 [details] [review]:

::: src/entryArea.js
@@ +91,3 @@
             function() {
+                let isError = this._ircParser.process(this._entry.text);
+                if (isError == 0)

Eeeks. "isError" sounds like a bool, but you made process() return a number instead - this isn't C, let's use true/false. That will also make the code obvious enough to drop the additional variable:

if (this._ircParser.process(text) {
  ...
} else {
  ...
}

@@ +93,3 @@
+                if (isError == 0)
+                    this._entry.text = '';
+                else {

Style nit:
No braces when all blocks are one liners, braces for all blocks otherwise.

@@ +94,3 @@
+                    this._entry.text = '';
+                else {
+                    this._entry.get_style_context().add_class('polari-error-command');

The default theme already has a suitable class - 'error' - which should be used instead.

@@ +95,3 @@
+                else {
+                    this._entry.get_style_context().add_class('polari-error-command');
+                    this._entry.grab_focus();

The entry always has keyboard focus when activated, so please add a small comment:

this._entry.grab_focus(); // select text

::: src/ircParser.js
@@ +91,3 @@
                 if (command && knownCommands[command])
                     output = this._createFeedbackUsage(command);
+                else if (command) {

See previous comment about braces style.

Alternatively use something like:

retval = (command == null || knownCommands[command] != null);

and leave the rest as-is (or possibly reorder as:

if (!retval) // error
   output = ...
else if (command)
  output = ...
else
  output = ...

@@ +264,3 @@
                 break;
             }
+            default: {

Adding braces here is fine, just a quick explanation for the current code:
The command use braces to avoid scoping issues with local variables: Without braces, the first command to use a particular name needs to declare the variable with let, while all following command must not re-declare the same variable (i.e. no let). This makes adding additional commands cumbersome, so each uses its own local variable scope instead.
I can't think of any reason why the default case would ever need a local variable, so that's why I avoided the braces there (but again, I don't mind the addition either).

Created attachment 313593 [details] [review]
Select text and show styled error

(In reply to Florian Müllner from comment #5)
> Adding braces here is fine, just a quick explanation for the current code:
> The command use braces to avoid scoping issues with local variables: Without
> braces, the first command to use a particular name needs to declare the
> variable with let, while all following command must not re-declare the same
> variable (i.e. no let). This makes adding additional commands cumbersome, so
> each uses its own local variable scope instead.
> I can't think of any reason why the default case would ever need a local
> variable, so that's why I avoided the braces there (but again, I don't mind
> the addition either).

Didn't know that. Removed braces for now. In future if needed, we will add it.

Quick question, can I make a personal branch in wip or somewhere else to commit my changes?

(In reply to Kunaal Jain from comment #7)
> Didn't know that. Removed braces for now.

Nah, as I said, it's fine ...


> Quick question, can I make a personal branch in wip or somewhere else to
> commit my changes?

If you have commit access, sure (we've started to use a convention of personal prefixes, e.g. wip/fmuellner/some-stuff).

(In reply to Florian Müllner from comment #8)
> (In reply to Kunaal Jain from comment #7)
> > Didn't know that. Removed braces for now.
> 
> Nah, as I said, it's fine ...
> 
> 
> > Quick question, can I make a personal branch in wip or somewhere else to
> > commit my changes?
> 
> If you have commit access, sure (we've started to use a convention of
> personal prefixes, e.g. wip/fmuellner/some-stuff).

I do have. In case you missed, I have uploaded patch.

(In reply to Kunaal Jain from comment #9)
> I do have. In case you missed, I have uploaded patch.

Yes, I did miss that. Still, for review please attach patches to the appropriate bugs.

(In reply to Florian Müllner from comment #10)
> Yes, I did miss that. Still, for review please attach patches to the
> appropriate bugs.

Yes I have aready attached the patch for review. Attachment 313593 [details]

Review of attachment 313593 [details] [review]:

LGTM

::: src/entryArea.js
@@ +187,2 @@
     _onEntryChanged: function() {
+        this._entry.get_style_context().remove_class('error'); // remove error class if present

Superfluous comment

Review of attachment 313593 [details] [review]:

Pushed to master with hash 4f53516fdfc15. Please mark the bug resolved.