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

Term:cout clog cerr cin #271

Merged
merged 57 commits into from
Jul 27, 2023
Merged

Term:cout clog cerr cin #271

merged 57 commits into from
Jul 27, 2023

Conversation

flagarde
Copy link
Collaborator

@flagarde flagarde commented Jul 8, 2023

First step to split terminal from "iostream" on it. @TobiasWallner Maybe there is some improvements on speed in Windows. You can have a try

@flagarde
Copy link
Collaborator Author

flagarde commented Jul 8, 2023

@certik Hi, This would solve #260 too. Could you check #108 too ?

@TobiasWallner
Copy link
Contributor

I will have a try. However, you may have to wait until the 19th for me to get to it.

@flagarde
Copy link
Collaborator Author

flagarde commented Jul 8, 2023

@TobiasWallner No problem, thanks for the time you involve on it. I do some basic test on Linux, Windows and less on Macos but but it's always nice to have more tries from people using the library. I'm still focusing on improving the library before using it on moving to it on my other projects

@flagarde
Copy link
Collaborator Author

Yes I think it's because you flush each time right ? I think you can save a lot of time using the buffer and just let the stream write to console by itselft. OS call are the most expensive operation I think.

Maybe but that is not viably for me. See, i render the whole screen first into a string and then pass the 8kB or something to the stream. So there is only this one and only line of code that will onle be executed once after an event happened.

Also I need the flush, because, what if the stream decides to not print everything, then the missing part will not get updated until the user invokes some kind of event, which is not viable.

Furthermore, thinking of if. You, of corse know more about it, since you just spend some time on it. I already have the whole string to print. So the string is kinda my whole frame of data, nothing more nothing less. Also I guess that a lot of cli will probably use similar approaches, like in a gui or gaming, where you calculate a frame an then just display that frame. So if you do it like that you do not need a buffered output, because I already have the whole buffer in that string.

Would it be possible to have a function, that just takes my string as a whole and flushes it and only calls the necessary system calls once? Maybe even asycronously?

Just for clarification, this is my main loop:

void Lime::run_main_loop(){
	// reserve memory in advance
	std::string outputString; 	
	outputString.reserve(1024 * 10); 
	
	// initial render of the whole screen
	this->render(outputString);
	this->draw(outputString); // <-- call to ::cout
	
	// main loop
	while(this->main_loop_continue){
		auto event = Term::read_event();
		this->prozess_event(std::move(event));
		outputString.clear();
		this->render(outputString);
		this->draw(outputString); // <-- call to ::cout
	}
}

like std::println() one ?

@flagarde
Copy link
Collaborator Author

And I think some kind of setting permanently changed withing my windows console when quitting the program. Because now after I quit and am back in the comman prompt instead of up, down, left, right I get ^[[A^[[A^[[A^[[B^[[D^[[C^[[A^[[D^[[B^[[D

Could it be that before you had some kind of virtual console and now you do everything in the 'real' console?

But when quitting the optinons from Therm::set_options are still present?

@TobiasWallner I checked this and it seems it appeared before this PR so it's not a regression of this PR but it is a bug we need to fix on the future.

@TobiasWallner
Copy link
Contributor

like std::println() one ?

I am not quite sure, if I can follow your thought prozess. If I am not mistaken then the Idea of ptrintln is to have a newline character and a flush at the end.

If your Idea of println is that the provided string will get printed directly and not written to a in between buffer before finally printing it, then yes.

That, I would say, is something of lower priority. It would be a nice to have but is not necessary.

@flagarde
Copy link
Collaborator Author

like std::println() one ?

I am not quite sure, if I can follow your thought prozess. If I am not mistaken then the Idea of ptrintln is to have a newline character and a flush at the end.

If your Idea of println is that the provided string will get printed directly and not written to a in between buffer before finally printing it, then yes.

That, I would say, is something of lower priority. It would be a nice to have but is not necessary.

Yes as soon as this is merged this kind of function will be easily added. I agree this could be very useful

@flagarde
Copy link
Collaborator Author

@TobiasWallner I found the bug, thx for your report. I will push it and you can have a try

@flagarde
Copy link
Collaborator Author

@TobiasWallner The bugs you mentioned should be fixed by the last commits

@flagarde
Copy link
Collaborator Author

flagarde commented Jul 24, 2023

@TobiasWallner Could you confirm the bugs are gone ? I made some basic tests on the 3 OS but it's always nice to have cross-checks.

@certik What do you think about this PR ?

@TobiasWallner
Copy link
Contributor

And I think some kind of setting permanently changed withing my windows console when quitting the program. Because now after I quit and am back in the comman prompt instead of up, down, left, right I get ^[[A^[[A^[[A^[[B^[[D^[[C^[[A^[[D^[[B^[[D

Could it be that before you had some kind of virtual console and now you do everything in the 'real' console?

But when quitting the optinons from Therm::set_options are still present?

This is still present on my windows machine

I am on commit: 2d8f9e0
of branch: remotes/external-packages/file

@flagarde
Copy link
Collaborator Author

Strange i fixed it but today it appeared again on my laptop. Need more investigation

@TobiasWallner
Copy link
Contributor

I will have a look tomorrow

@flagarde
Copy link
Collaborator Author

Should be fixed now (I hope :) )

@TobiasWallner
Copy link
Contributor

I had a check of commit ddd4113 and the console closes propperly now 👍

@flagarde
Copy link
Collaborator Author

@TobiasWallner you didn't find any problem integrating with your software ? If not I think I should merged it to allow a polishing later. The basic is here

@TobiasWallner
Copy link
Contributor

TobiasWallner commented Jul 26, 2023

I only needed to change the header files from #include <cpp-terminal/io.hpp> to #include <cpp-terminal/input.hpp> and use Term::cout instead of Term::terminal.
So basically plug and play

@flagarde
Copy link
Collaborator Author

Nice, for now we don't assure API so it's ok I think the name iostream.hpp help people to understand what they espect to have including it

@flagarde
Copy link
Collaborator Author

I will merge then. I really want to have this mainstream because with a bit of work it would allow to have color with old terminal in windows and all other escape code.. we need to parse the string but at least it can be done.

@flagarde flagarde merged commit 6fbc24f into jupyter-xeus:master Jul 27, 2023
282 of 283 checks passed
@flagarde flagarde deleted the file branch July 27, 2023 02:14
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.

2 participants