Skip to content

Conversation

@staxfur
Copy link
Collaborator

@staxfur staxfur commented Nov 22, 2021

This PR splits the missing headers into source files and finally removes base_terminal.h and terminal.h.

@staxfur staxfur mentioned this pull request Nov 22, 2021
@staxfur
Copy link
Collaborator Author

staxfur commented Nov 23, 2021

The changes are done now. Just have to test the library on windows. @certik could you test the example programs meanwhile on macos? The input would be the most important thing to test.

@wolfv
Copy link
Member

wolfv commented Nov 23, 2021

I just tested a bit on macOS. The only issue I could find is that the prompt example, trying to add a new line with ALT+Enter ends the program with Runtime error: Invalid byte in UTF8 encoded string

@staxfur
Copy link
Collaborator Author

staxfur commented Nov 23, 2021

I have seen that too, but that was like this before. Also it's only after a few new lines - or is it directly causing a runtime error for you?

@certik
Copy link
Collaborator

certik commented Nov 23, 2021

The Alt-Enter has to be fixed, but that already happens in master:

(lf) repos/cpp-terminal(master) $ ./examples/prompt 
Interactive prompt.
  * Use Ctrl-D to exit.
  * Use Enter to submit.
  * Features:
    - Editing (Keys: Left, Right, Home, End, Backspace)
    - History (Keys: Up, Down)
    - Multi-line editing (use Alt-Enter to add a new line)
> 4                                                                      1,2   ]
Submitted text: 4

> 4+4                                                                    1,4   ]
Submitted text: 4+4

> Unknown error.                                                         1,1   ]
(lf) repos/cpp-terminal(master) $ ./examples/prompt
Interactive prompt.
  * Use Ctrl-D to exit.
  * Use Enter to submit.
  * Features:
    - Editing (Keys: Left, Right, Home, End, Backspace)
    - History (Keys: Up, Down)
    - Multi-line editing (use Alt-Enter to add a new line)
> Unknown error.                                                         1,1   ]

So it is unrelated to this PR.

@certik
Copy link
Collaborator

certik commented Nov 23, 2021

This PR compiles fine and seem to work on Apple M1.

However, the prompt example now starts in an empty screen. The behavior it should do is to start in the user terminal, like before. The full screen mode is tested by other examples such as menu or menu_screen, so let's use the prompt to test that it works in a regular terminal.

@certik
Copy link
Collaborator

certik commented Nov 23, 2021

Besides the prompt screen issue, the PR looks good to me, I went over it. Let's fix the prompt and after that I think we can merge and polish in master with subsequent PRs if needed.

@staxfur
Copy link
Collaborator Author

staxfur commented Nov 23, 2021

I'll fix the empty screen for the prompt example quickly. WIndows works fine!

@staxfur
Copy link
Collaborator Author

staxfur commented Nov 23, 2021

@certik the prompt is broken on windows. But in the master branch as well. That must be fixed as well.

On runtime, I get the error:

Debug Assertion Failed!
Expression: c >= -1 && c <= 255

in file minkernel/crts/ucrt/src/appcrt/convert/jsctype.cpp

@staxfur
Copy link
Collaborator Author

staxfur commented Nov 23, 2021

Also the reloading of the old screen is a bit broken somehow on windows. Will create Issues on those.

@staxfur
Copy link
Collaborator Author

staxfur commented Nov 23, 2021

I have created issues on the found problems. Once the unit tests continue, you should look one last tme over the changes and once you approve this PR, I'll merge.

@staxfur staxfur requested review from certik and wolfv November 23, 2021 14:44
@staxfur staxfur changed the title WIP: Splitting the rest Splitting the rest Nov 23, 2021
Copy link
Collaborator

@certik certik left a comment

Choose a reason for hiding this comment

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

The prompt now works. I think this is good to merge as is.

Then we need to fix all bugs, on Windows as well as the alt-enter on a mac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants