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

Splitting the rest #132

Merged
merged 16 commits into from
Nov 23, 2021
Merged

Conversation

MCWertGaming
Copy link
Collaborator

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

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

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

@MCWertGaming
Copy link
Collaborator Author

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.

@MCWertGaming
Copy link
Collaborator Author

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

@MCWertGaming
Copy link
Collaborator Author

@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

@MCWertGaming
Copy link
Collaborator Author

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

@MCWertGaming
Copy link
Collaborator Author

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.

@MCWertGaming MCWertGaming 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.

None yet

3 participants