[WIP] Added chat tab #14

fremiller · 2018-11-05T17:33:48Z

This is for #13, which is to add a better way of chatting in the interface.

I've added two methods of sending chat commands, one using the commands themselves, and one which uses a dropdown to select which user to send the command to. At the moment, the users do not display correctly in the dropdown.

Todo

Fix user display issue
Add better error messages
Fix linting errors

I did most of this work last week, so I can't remember what exact stage I'm at, but I should be able to put more work in this tomorrow.

Inei1

Looks fine for the most part, aside from code style/linter issues. The main thing to think about is if you want to support using the chat commands - my opinion is that they are unnecessary, but it's up to you. I'll look more into the details and implementation when you finish this.

For the linter errors, what IDE are you using? You can use IntelliJ ultimate's built in typescript support, and it will underline an issue with tslint as an error. If you have a .edu email address, you can easily get IntelliJ ultimate. If not, you can run tslint manually by opening command prompt in the FacadeServer-Frontend directory and executing this command:
npm run tslint

package-lock.json should be removed. If you want, you could quickly add it to .gitignore in this PR for bonus points.

Inei1 · 2018-11-06T02:26:44Z

src/tabs/chat/ChatAutocomplete.tsx

+// Tab completion engine for autocompleting commands. Inspired by the TabCompletionEngine class
+// for the Terasology in-game console.
+
+export class ChatAutoComplete {


You probably don't need this file. If you do need it for something, just use ConsoleAutocomplete from the console tab.

Inei1 · 2018-11-06T03:22:45Z

src/tabs/chat/ChatTabView.tsx

+  private autoComplete(command: string) {
+    ChatAutoComplete.setCommandList(this.state.commands);
+    this.props.model.update({ commandToSend: ChatAutoComplete.complete(command) });
+  }


Don't need this method.

Inei1 · 2018-11-06T03:29:20Z

src/tabs/chat/ChatTabModel.tsx

+            messageSendStatus: "SENT"
+          });
+        }
+        else if (msg.message.match(/User with name '[a-zA-Z0-9]+' not found./)) {


Use === operator for string comparison. Regex can quickly lead to confusing errors, so only use it if absolutely necessary.

Not sure how to do this without regex. I can remove it entirely if you don't think it's worth it.

Inei1 · 2018-11-06T03:36:19Z

src/tabs/chat/ChatTabState.tsx

+  errorMessage?: string;
+  selectedRecipient?: string;
+  commandMode?: boolean;
+  onlinePlayers?: OnlinePlayerMetadata[];


Remember that these are like global variables. Try to minimize the amount of variables in the state if you can.

Inei1 · 2018-11-06T03:42:27Z

src/tabs/chat/ChatTabController.tsx

+    public execute = () => {
+        var command: string = this.model.getState().commandToSend;
+        if (this.model.getState().commandMode) {
+            if (this.IsChatCommand(command)) {


Is there a particularly good reason why this command mode exists? Looks to me like you could remove this if statement completely and just leave what is in the else statement.

I included it in case people preferred typing commands than using a dropdown to select the recipient. But if you don't think its needed, I'll remove it.

Inei1 · 2018-11-06T03:43:36Z

src/tabs/chat/ChatTabController.tsx

+            if(!recipient){
+                recipient = "ALL"
+            }
+            if(recipient == "ALL"){


I think it would be fine if you moved the contents of this if statement into the one above.

Inei1 · 2018-11-06T03:44:20Z

src/tabs/chat/ChatTabController.tsx

+        }
+    }
+
+    private GetCommandKeyword(command: String): String {


This method should be static.

Inei1 · 2018-11-06T03:50:19Z

src/tabs/chat/ChatTabModel.tsx

+
+  public getSubscribedResourcePaths(): ResourcePath[] {
+    return [
+      ["console", "onlinePlayers"],


You don't need to subscribe to the console resource. The subscribing is only done if you need the GET resource, which in this case is the command list.

Inei1 · 2018-11-06T03:51:01Z

src/tabs/chat/ChatTabModel.tsx

+
+  public onResourceUpdated(resourcePath: ResourcePath, data: any): void {
+    if (ResourcePathUtil.equals(resourcePath, ["console"])) {
+      this.update({ commands: data as string[] });


Same thing here, don't need to do anything with the console resource.

fremiller · 2018-11-08T08:00:37Z

Hi
I've fixed most of the lint errors, but some (in ChatTabView.ts) can't be fixed, and they appear in other modules (ConsoleTabView.ts). (I think it might be because I'm not importing reactxp correctly. I'm around later to test it so I'll make sure everything still works, and I'll fix any remaining errors.
Thanks

Inei1 · 2018-11-09T03:36:37Z

Looks like you're on the right track. You can ignore the lint errors in the other files, see https://travis-ci.org/MovingBlocks/FacadeServer-frontend/builds/452264085#L523 for the errors that the linter gives.

Don't forget to remove package-lock.json.

fremiller · 2018-11-10T12:15:07Z

In ChatTabView.tsx I have long lines (29, 33) to render the messages. Because having it on one line gives an error, but having it on multiple also gives an error, should I keep it on one line, have it on multiple, or generate it inside outside the return statement, then pass it in as a variable?

Also I'll rebase all these commits into one afterwards

fremiller · 2018-11-10T13:47:30Z

Here are some screenshots of it working:

I'm still struggling to get it work when whispering. If I try to whisper either nothing happens or the server crashes... I'll continue to invesigate...

Inei1 · 2018-11-11T05:28:36Z

For the lines that are too long, you can create a function somewhere else in the file and call it in the rendering code. See this and the function called on line 80 for an example. You could also do something similar for the block of code starting at line 41 on ChatTabView.

It looks like the PR is trying to delete package-lock.json. We don't want this, and you can bring it back by recreating a file with the same name and extension and pasting the contents of this into it.

I can confirm that it works on my end. I think the user experience could be improved if the message input box cleared itself after sending a message, like the command input does.

As for whisper not working, the error is because you are trying to send the message to yourself. The way the system works, connecting to the game and the web frontend at the same time results in undefined behavior. It is possible to send a whisper to yourself through the in-game console, but it appears to not work if you do so from the frontend to the game. If you connect with a different client, such as by running the 2nd client configuration in Intellij, and send a whisper to it then it works correctly. Maybe you could add a check to prevent sending a whisper to yourself.

fremiller · 2018-11-17T15:21:16Z

Hi
Sorry for the delay, thanks for being patient.
It works with whispers, however, whispers don't appear in the chat tab. They come up as type CONSOLE rather than CHAT I can create an issue for this if you'd like.

I couldn't figure out how to get information about the current player to make sure they don't whisper to them self.

Thanks

fremiller · 2018-11-17T15:32:49Z

There are weird linter errors... I don't know where they come from, and they don't show up on my computer...

I'll rebase everything into one commit once you think it's ready to merge.

Thanks

Inei1

Almost done, just a few minor changes.

Whisper should absolutely go into chat. I would appreciate it if you made an issue to fix it. Preventing the user from whispering themselves could also go into an issue as well, if you don't know how to implement it. I won't be able to tell how you would prevent it without looking into it in depth myself.

As for the linter errors, I have absolutely no idea what is going on with them. When I pulled master branch and then pulled this PR back, package-lock.json changed for some reason. I'm guessing it has something to do with package-lock.json. I don't know how you would fix this other than closing the PR and opening a new one with the same changes and not touching package-lock.json. Pinging @gianluca-nitti to see if he has any idea about what's going on with the linter errors.

Inei1 · 2018-11-18T04:17:23Z

src/tabs/chat/ChatTabState.tsx

@@ -0,0 +1,16 @@
+import { OnlinePlayerMetadata } from "common/io/OnlinePlayerMetadata";


Use a relative path here to be consistent with the rest of the project.

Inei1 · 2018-11-18T04:21:54Z

src/tabs/chat/ChatTabModel.tsx

+          this.update({
+            messageSendStatus: "SENT",
+          });
+        } else if (msg.message.match(/User with name '[a-zA-Z0-9]+' not found./)) {


You can use else if (msg.message === "User with name '[a-zA-Z0-9]+' not found.") instead of the regex match.

fremiller · 2018-11-18T13:30:01Z

Ok, I'll make a new PR and see if that works

fremiller · 2018-11-18T13:42:05Z

I tried rebasing all the commits, and it said package-lock had been modified.... I don't know why it didn't show up in the diff in the PR

fremiller · 2018-11-18T13:42:57Z

Passed!

fremiller · 2018-11-18T14:07:55Z

I've made issue #15

fremiller · 2018-11-18T14:16:17Z

Hmm... Turns out that commit modified package-lock.json
I've reverted it back to the one before, and it doesn't work...

Does the most recent PR pass if you run the test again?

fremiller · 2018-11-18T14:20:05Z

I tried deleting my node_modules folder, and it downloaded all the dependencies and modified the package-lock.json file, but surely travis would do that...

Inei1 · 2018-11-20T03:12:10Z

It might work if you create a new branch with the same commit: git branch -b branchname. This will copy the commits from whatever branch you are on to a new branch. Then you can open a new PR for merging the new branch into master. PRs don't cost anything, so if we can't figure out what's going on with travis ci then we might as well just try it.

gianluca-nitti · 2018-11-21T22:20:59Z

Hi, sorry for the delay - thank you @fishfred for the code and @Inei1 for the in-depth review.

I did some quick investigation to find out why the build fails, it wasn't related to this PR but to changes in Travis (also restarting an old successful build from master caused it to fail). Unfortunately I haven't kept myself updated about Node and Travis in the last months but from what I can see now Travis runs npm ci (clean install) instead of the plain old npm install, which if I understand correctly forces it to install the packages at the exact versions they are listed in package-lock.json. Most likely our package-lock.json was outdated (referencing deprecated package version I guess - didn't investigate this in-depth yet), since running npm install on my machine updated it. I now committed (3959ffb) the updated version and the old build of master I had restarted and was failing is now succeeding.

So @fishfred I think that if you merge 3959ffb in your master the build for this PR should fix itself as well.

fremiller · 2018-11-27T16:22:43Z

Thanks @Inei1 @gianluca-nitti for all your help!

Inei1

This will (hopefully) be the final review.

Again, just a few small changes.

Inei1 · 2018-11-28T03:03:13Z

src/tabs/chat/ChatTabController.tsx

@@ -0,0 +1,29 @@
+import { TabController } from "../TabController";
+import { ChatTabState, Message } from "./ChatTabState";


Message is an unused import.

Inei1 · 2018-11-28T03:15:49Z

src/tabs/chat/ChatTabView.tsx

+    return "rgb(" + r.toString() + "," + g.toString() + "," + b.toString() + ")";
+  }
+
+  private handleKeypress(event: any, controller: ChatTabController) {


This can be static.

Inei1 · 2018-11-28T03:45:42Z

src/tabs/chat/ChatTabController.tsx

+                data: `whisper ${recipientName} ${command}`,
+                method: "POST",
+                resourcePath: ["console"],
+            });


I've noticed in testing that whispering to a player doesn't give any feedback to the sender if on the frontend. I think it would be helpful, and it can be done by updating the state of the messages:
this.model.update({messages: this.model.getState().messages.concat({type: "CHAT", message: "message"})});
The in-game console uses the format "You -> {playername}: {message}".

Inei1 · 2018-12-30T20:44:21Z

@fishfred any follow up? If you don't want to work on this anymore let me know and I can finish the rest. It shouldn't take more than an hour.

fremiller changed the title ~~[WIP~~ [WIP] Added chat tab Nov 5, 2018

fremiller force-pushed the master branch from 2fc775c to aaacb29 Compare November 5, 2018 17:38

Inei1 reviewed Nov 6, 2018

View reviewed changes

Inei1 reviewed Nov 18, 2018

View reviewed changes

fremiller force-pushed the master branch from c22660e to d9118fe Compare November 18, 2018 13:41

fremiller force-pushed the master branch from d9118fe to 66f71f4 Compare November 18, 2018 14:12

Added chat tab

19bf0b6

fremiller force-pushed the master branch from 8b88e5f to 19bf0b6 Compare November 27, 2018 16:18

Inei1 reviewed Nov 28, 2018

View reviewed changes

		@@ -0,0 +1,16 @@
		import { OnlinePlayerMetadata } from "common/io/OnlinePlayerMetadata";

		@@ -0,0 +1,29 @@
		import { TabController } from "../TabController";
		import { ChatTabState, Message } from "./ChatTabState";

[WIP] Added chat tab #14

Are you sure you want to change the base?

[WIP] Added chat tab #14

Conversation

fremiller commented Nov 5, 2018 • edited Loading

Todo

Inei1 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

Choose a reason for hiding this comment

fremiller commented Nov 8, 2018

Inei1 commented Nov 9, 2018

fremiller commented Nov 10, 2018

fremiller commented Nov 10, 2018

Inei1 commented Nov 11, 2018 • edited Loading

fremiller commented Nov 17, 2018

fremiller commented Nov 17, 2018

Inei1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fremiller commented Nov 18, 2018

fremiller commented Nov 18, 2018

fremiller commented Nov 18, 2018

fremiller commented Nov 18, 2018

fremiller commented Nov 18, 2018

fremiller commented Nov 18, 2018

Inei1 commented Nov 20, 2018

gianluca-nitti commented Nov 21, 2018

fremiller commented Nov 27, 2018

Inei1 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

Inei1 commented Dec 30, 2018

fremiller commented Nov 5, 2018 •

edited

Loading

Inei1 commented Nov 11, 2018 •

edited

Loading