Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed input on wndows #181

Merged
merged 8 commits into from
Sep 16, 2022
Merged

fixed input on wndows #181

merged 8 commits into from
Sep 16, 2022

Conversation

flagarde
Copy link
Collaborator

@flagarde flagarde commented Aug 22, 2022

This PR fixes input related bugs.

Issues: #169, #168, #134, #133.

@flagarde
Copy link
Collaborator Author

Sorry to mix all of these but they are all related :)

@MCWertGaming MCWertGaming changed the title Fix #169, #168, #134(partially), #133 fixed input on wndows Aug 22, 2022
@MCWertGaming MCWertGaming self-requested a review August 22, 2022 10:16
@MCWertGaming MCWertGaming added the bug Something isn't working label Aug 22, 2022
@MCWertGaming MCWertGaming added this to the V1.0.0 milestone Aug 22, 2022
@MCWertGaming
Copy link
Collaborator

Thank you really much for all the fixing! Since I only develop on linux for most of the time the fixing was rather complicated for me. I'll test this on linux and windows to make sure that everything works (especially since kilo broke a lot in past PRs). I'll do a review then.

@MCWertGaming
Copy link
Collaborator

Also: We sadly don't have PR templates yet, but please try to add a short description and the issues in the issue description rather than the title. referencing issues are replaced with links by github to make it easier to check what they are about, which doesn't work for PR titles.

@MCWertGaming MCWertGaming requested a review from certik August 22, 2022 10:42
@MCWertGaming
Copy link
Collaborator

@certik might want to take a look at this as well since he implemented the key struct and the input parsing for various terminals and platforms. Maybe we should test this on mac as well, I sadly don't have access to one.

@flagarde
Copy link
Collaborator Author

Sure please have cross-checks...I mainly develop on linux too.. but I like to have stufs to work everywhere and Windows is so difficult... So I enjoy this kind of library wrapping difficulties 😀

There is some github repo to install a macos on virtualbox.. It could be useful for testing..

I only run windows on virtualbox by the way...

@MCWertGaming
Copy link
Collaborator

Yeah but windows console peformance is really poor. I made a chess game in the terminal for a coding challenge on a discord server with a pretty simple renderer and while it's perfectly fine on Linux, it doesn't look that well on windows as every time the screen is redered you see a little flickering. The new Window class renderer should fix that on windows as well (as it tries to render as little of the screen as possible).

The new numbers of the keys worry me through. Also you can use ascii symbols using the character when checking the input.

@flagarde
Copy link
Collaborator Author

flagarde commented Aug 22, 2022

Yes, but this is the standard ASCII table, I think it would be nice to have this.. To split and clarify the code.. I plan to do a PR with them and using them. The idea I had is to stress on the code you are using terminal. It's a lot of keys but it could be put in a dedicated header. For some you can use symbole but for the other you can't, it create a difference in the code. I would prefer to have :

cpp if(key==Key::ACK || key == Key::A ) then if(key==Key::ACK || key == 'A' )

It's a matter of taste for now but we could imagine to have enum class key. It seems to be pedantic but this is a library to be included in other code I'm more and more convinced that an enum class can save from very bad mistakes sometimes and it's always possible to static_cast... Again it can be pedantic but at least, personally, a static-cast it's good, it gives hints on two scenarios :

  • I just static_cast and i'm praying for the good.
  • I thinked twice and the static_cast is the right way.

An other very annoying thing is that char is signed or unsigned depending on the platform, to have a enum with all ANSCII could supress warning etc...

@flagarde
Copy link
Collaborator Author

It even have done some benefits now in kilo, MSVC refused to compiled because the case where identical :)

@MCWertGaming
Copy link
Collaborator

hmm yeah I see the problem. Feel free to do that PR! I'm working on the window class update right now. Will test this and the other PR when I'm done later.

@flagarde
Copy link
Collaborator Author

The next PR would be mainly cosmetic so I can wait for the PRs to be merged. By the way I have some feature, ideas I would like to work on. Would you accept some Issues to discuss if you want to have them included ?

@MCWertGaming
Copy link
Collaborator

Of course! I have opened a few ones already on cosmetic things. the new window class has some as well, like fancy boxes and things like that. Feel free to open discussions about everything that comes to your mind ^^

@flagarde
Copy link
Collaborator Author

Yeah but windows console peformance is really poor. I made a chess game in the terminal for a coding challenge on a discord server with a pretty simple renderer and while it's perfectly fine on Linux, it doesn't look that well on windows as every time the screen is redered you see a little flickering. The new Window class renderer should fix that on windows as well (as it tries to render as little of the screen as possible).

The new numbers of the keys worry me through. Also you can use ascii symbols using the character when checking the input.

Have you try using cerr which is unbuffered

Or https://en.cppreference.com/w/cpp/io/manip/unitbuf

Or even if you don't use C I/O use this trick too : https://usaco.guide/general/fast-io?lang=cpp

Maybe using fmt https://github.com/fmtlib/fmt in the cpp-terminal. It would simplify some work but cpp-terminal would stop bein a standalone library...
They claim to be faster on some benchmark but for cpp-terminal i don't know if it worth. The really nice feature is that it has color support etc so many work would be simpler

@MCWertGaming
Copy link
Collaborator

We should look into supporting fmt as it looks really nice. moved that into #185 and the performance improvements into #186. Thank you really much for pointing those out!

Copy link
Collaborator

@MCWertGaming MCWertGaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the changes on Linux and the input is broken now. for example:

  • ENTER is now CTRL+J
  • BACKSPACE is now CTRL+H
  • TAB is CTRL+I

others are broken too.

Does this fix anything on windows? Will test that tomorrow. we might need to catch different keys on different platforms.
@flagarde

@flagarde
Copy link
Collaborator Author

It's not really a bug this keys are the equivalent of enter etc i found a solution I can make the modifs...

@MCWertGaming
Copy link
Collaborator

Before you fix with the values of 1000 it worked I think, but it's possible that some keys collide. We should then make a wiki page with all problematic keys which would also include default keys of terminals (like vscode terminal shortcuts).

@flagarde
Copy link
Collaborator Author

The thing is that CTRL + keu is remaped to the first 31 case of the ASCII table so the collision is per design...

That's why I really would like to have all the ASCII table... I would rename the first 31 ASCII in key to their corresponding CTRL keys or the ENTER TAB etc... It is confusing for people that some control key are mapped to ENTER or TAB etc...
https://stackoverflow.com/questions/14615717/how-can-vim-tell-the-difference-between-ctrl-j-and-lf

@MCWertGaming
Copy link
Collaborator

I have tested it and you are right for some, but for example the master handles CTRL+H and BACKSPACE independently while you branch doesn't. We should fix that first.

@flagarde
Copy link
Collaborator Author

CTRL-H is the ASCII BS (Backspace) character.

@flagarde flagarde requested review from MCWertGaming and removed request for certik August 24, 2022 20:02
@MCWertGaming
Copy link
Collaborator

Really nice! Will test this another time, just to make sure!

@flagarde
Copy link
Collaborator Author

Sure , maybe there is some corner cases if didn't found.. I did some test on linux and windows..

Copy link
Collaborator

@MCWertGaming MCWertGaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Will merge once the merge conflict is resolved. I'm not sure it the new terminal settings can be skipped if colors are not supported.

Also enabling / disabling CTRL+C should be optional (I use it for renderer debuggung mostly, but it probably is also used by others). See

bool /*disable_ctrl_c*/)

@flagarde
Copy link
Collaborator Author

flagarde commented Sep 8, 2022

I will try to fix the merge conflicts. Concerning CTRL+C is realized that you have an option for it..

An other possibility would be to always deal ourselft with this keys and raise the corresponding signals when the keys are pressed. There would be some benefits doing this

@MCWertGaming
Copy link
Collaborator

It's rather complicated since throwing a runtime_error won't be a nice way to do that and the switch must stay there since some application developers might want to handle CTRL+C themselves (like a clean shut down of the program), but others want to keep the normal terminal behavior, like that CTRL+C terminates the terminal.

@flagarde
Copy link
Collaborator Author

flagarde commented Sep 12, 2022

We could use raise function from csignal https://cplusplus.com/reference/csignal/ it's standard.

@MCWertGaming
Copy link
Collaborator

hmm would that improve anything over just disabling CTRL+C catching? I mean CTRL+C even works when the main thread is used by something else, like when you have a main loop that blocks the main thread.

@flagarde
Copy link
Collaborator Author

I update the change needed.

@MCWertGaming
Copy link
Collaborator

Thank you really much! I'll merge once the CI run is done.

Copy link
Collaborator

@MCWertGaming MCWertGaming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good!

@MCWertGaming MCWertGaming merged commit 9496cc8 into jupyter-xeus:master Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
2 participants