Implement a TUI client using console_engine by VincentFoulon80 · Pull Request #1 · LoipesMas/accord

VincentFoulon80 · 2022-04-30T15:00:59Z

Hello !

I wanted to try to implement the TUI client of Accord using my own crate console_engine.
It's more of a "proof-of-concept", so I won't mind if you close this PR right away.
Feel free to comment my code if you see anything wrong, I'm still learning the language so any comment is appreciated.

Here's a changelog of what this PR changes :

🆕 Features

used console_engine to implement a TUI client for Accord

🐞 Bugfixes

Fixed Windows terminals sending \r\n as Enter key instead of the expected \n

📋 Todos (for later)

~~Implement login with the TUI crate~~
~~Implement auto scrolling~~
~~Allow moving the text cursor~~
~~handle more keyboard keys (home, end, del, ...)~~

LoipesMas

This look really nice, thanks!
I only found a few minor issues.
And if you'd finish what you mentioned in todos, I would definitely merge this, since it's clearly an improvement over what I made.

client/src/console.rs

client/src/main.rs

VincentFoulon80 · 2022-05-01T18:10:03Z

I fixed the issues, and also resolved conflicts.

I'm working on implementing the rest of the todolist, I'm changing this PR to a draft.

Thanks for your review !

VincentFoulon80 · 2022-05-01T21:35:25Z

I just finished implementing my todolist into this PR, I hope I haven't made too much of a mess, moving all this code around and writing some cryptic code like in Message::print() where I "enabled" text_wrapping by inserting \n in messages (console_engine only handles text wrapping when we try to print \n characters).

LoipesMas

Great!
The code is indeed a bit messy, but I think mainly because it has too much code. I'd suggest abstracting some functions (e.g. the wrapping), so it looks clearer.

Another thing that I would like to see is better error handling. So instead of panicking on incorrect password, just display a message and allow the user to try again.

And maybe address input should be added to login screen as well? But I'm not sure.

client/src/main.rs

client/src/console.rs

VincentFoulon80 · 2022-05-03T22:17:21Z

Thanks for the review ! I'll look into it in detail later.

All this motivated me to implement Forms into my TUI crate directly, to simplify the code here but also allow everybody to have access to this kind of input in their own application. I'd be glad if one day you could review the PR out (when I'll be done with it), you've given me some precious advice and I truly appreciate the time you take to help me. Thank you !

VincentFoulon80 · 2022-05-26T13:53:59Z

I finally finished my PR to add forms directly into the TUI crate, adding more features like checkboxes and radios, validation...

If you ever feel like taking a peek at the code, it's here.

Once the PR is merged (in a day or two, in case of someone wanting to review), I'll work on cleaning this mess up once and for all 🙂

LoipesMas · 2022-05-28T20:18:06Z

Sorry for the late reply 😅 I got kinda busy and forgot about this.
I can take a look at that PR tomorrow.

VincentFoulon80 · 2022-05-29T15:07:09Z

No worries about this, take your time and thanks a million for your help on this 😄
If you don't have time to check it it's also fine.

I'm on my way to adapt my work here to the future release, by the way. It's looking cleaner for sure 😉

Remove unused code

VincentFoulon80 · 2022-06-04T14:04:54Z

Hello ! Since I released console_engine version 2.4.0, I updated my code here to use the new features. I used clap to manage parameters (like address and port of the server)

I also took the freedom to change colors of the terminal, to mimic the GUI version

Let me know if you see anything I should change

LoipesMas

Nice!
Only a few minor issues.

Also, as I said before, I think it's better to move address (and port) input to login screen. Unless you have arguments against.

LoipesMas · 2022-06-05T10:58:30Z

client/src/console.rs

+                .chars()
+                .enumerate()
+                .flat_map(|(i, chr)| {
+                    if i != 0 && i % screen.get_width() as usize == 0 {
+                        lines += 1;
+                        Some('\n')
+                    } else {
+                        None
+                    }
+                    .into_iter()
+                    .chain(std::iter::once(chr))
+                })
+                .collect::<String>();


I'd suggest abstracting this out to a separate function for readability and easier changes.
Also I think this code will fail if there is \n in the message (which right now is not allowed because it caused me trouble, but it probably could (and should) be allowed after this PR is merged).

LoipesMas · 2022-06-05T11:08:48Z

client/src/console.rs

+        self.user_list.clear();
+        for entry in new_list {
+            self.add_user(entry)
+        }


Could this be replaced with:

self.user_list = BTreeSet::from(new_list.iter()); self.dirty = true;

?
I think this would be clearer. Unless we want something to happen in add_user in the future?

LoipesMas · 2022-06-05T11:11:13Z

client/src/console.rs

+        if !self.user_list.contains(&username) {
+            self.dirty = true;
+            self.user_list.insert(username);
+        }


How about:

self.dirty |= !self.user_list.insert(username);

?
This should behave the same, because insert already checks if user_list contains the value (and returns if it was there).

LoipesMas · 2022-06-05T11:13:17Z

client/src/console.rs

+        if self.user_list.contains(&username) {
+            self.dirty = true;
+            self.user_list.remove(&username);
+        }


And here:

self.dirty |= self.user_list.remove(&username);

client/src/main.rs

LoipesMas reviewed May 1, 2022

View reviewed changes

client/src/console.rs Outdated Show resolved Hide resolved

client/src/console.rs Outdated Show resolved Hide resolved

client/src/console.rs Outdated Show resolved Hide resolved

client/src/console.rs Outdated Show resolved Hide resolved

client/src/main.rs Outdated Show resolved Hide resolved

VincentFoulon80 force-pushed the feature/rich-tui-client branch from 5e48553 to 73a53b3 Compare May 1, 2022 17:59

VincentFoulon80 marked this pull request as draft May 1, 2022 18:10

VincentFoulon80 force-pushed the feature/rich-tui-client branch from f181c8c to 59f7a95 Compare May 1, 2022 21:17

VincentFoulon80 marked this pull request as ready for review May 1, 2022 21:19

LoipesMas reviewed May 3, 2022

View reviewed changes

client/src/main.rs Outdated Show resolved Hide resolved

client/src/console.rs Outdated Show resolved Hide resolved

client/src/console.rs Outdated Show resolved Hide resolved

VincentFoulon80 added 10 commits June 4, 2022 15:50

Implement a TUI client using console_engine

da8cfb9

Replace loop with while let in console_loop

396dd3d

Remove unused code

Fix missing byte litteral operator on unix's OS_EOL

0f03b52

Handle Shift key

ee85ae6

Fix PR comments

a2f4023

Implement login, autoscrolling, cursor and handle special keyboard keys

852f781

Updated Cargo.lock

6be66f7

Handle userlist window size more properly

31858a5

Bump console_engine to version 2.4.0

e66fde3

Cargo update

f031b26

VincentFoulon80 force-pushed the feature/rich-tui-client branch from 7c678e1 to f031b26 Compare June 4, 2022 13:59

LoipesMas requested changes Jun 5, 2022

View reviewed changes

(WIP) Ask address and port into the login form

07cfae7

Conversation

VincentFoulon80 commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🆕 Features

🐞 Bugfixes

📋 Todos (for later)

Uh oh!

LoipesMas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VincentFoulon80 commented May 1, 2022

Uh oh!

VincentFoulon80 commented May 1, 2022

Uh oh!

LoipesMas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VincentFoulon80 commented May 3, 2022

Uh oh!

VincentFoulon80 commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LoipesMas commented May 28, 2022

Uh oh!

VincentFoulon80 commented May 29, 2022

Uh oh!

VincentFoulon80 commented Jun 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LoipesMas left a comment

Choose a reason for hiding this comment

Uh oh!

LoipesMas Jun 5, 2022

Choose a reason for hiding this comment

Uh oh!

LoipesMas Jun 5, 2022

Choose a reason for hiding this comment

Uh oh!

LoipesMas Jun 5, 2022

Choose a reason for hiding this comment

Uh oh!

LoipesMas Jun 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VincentFoulon80 commented Apr 30, 2022 •

edited

Loading

VincentFoulon80 commented May 26, 2022 •

edited

Loading

VincentFoulon80 commented Jun 4, 2022 •

edited

Loading