search

bczsalba / pytermgui

Python TUI framework with mouse support, modular widget system, customizable and rapid terminal markup language and more!
https://ptg.bczsalba.com
MIT License
2.23k stars 54 forks source link

`_WinSIGWINCH` implementation for non-Unix platforms #19

Closed USLTD closed 2 years ago

USLTD commented 2 years ago

Also black formatting and small optimisation

USLTD commented 2 years ago

I removed comments at import like you said

USLTD commented 2 years ago

Sorry for many commits. It looks like my editor and keyboard (I am coding on Android and sometimes it bugs) messed up formatting few times. Also when I was writing I had three projects open and I accidentally replaced to-commit code in few places but I fixed it. Please review it

USLTD commented 2 years ago

Also please note my comment on tput (even though I is outside of scopes of this PR I think it is still necessary for cross-platform functionality). Also I got Windows device and I will test this code on it too

USLTD commented 2 years ago

Better. Thank you. My previous comments still stand, from before all of the editor mishaps.

Do you mean that there are still things I should fix?

bczsalba commented 2 years ago

Yes. I have no idea where it all went to, but I can't find it anymore in this sea of comments. My biggest thing was that _WinSIGWINCH does not comply with out naming scheme. All function names should be verbs, and snake_case. Try something like monitor_sigwinch and mention that this is only used on Windows. I had more things to say, but I can't remember them nor find them anymore.

USLTD commented 2 years ago

Ok. So last patch of this PR will be naming. In 1 minute

USLTD commented 2 years ago

I renamed it to _alt_sigwinch (Alternative SIGWINCH) since it is not only for Windows but for all platforms which don't support SIGWINCH signal and I think I am done with this PR. Are there anything left?

bczsalba commented 2 years ago

The tput comments are still there in the file, but if I keep mentioning every single change I want you to make we are never getting anywhere. I'll merge this now, thank you for your work.

USLTD commented 2 years ago

The tput comments are still there in the file, but if I keep mentioning every single change I want you to make we are never getting anywhere. I'll merge this now, thank you for your work.

No problem but I removed that comments before commiting 🤔 Looks like Editor didn't save it. Forget it. I will fix it in next commit

bczsalba commented 2 years ago

So it turns out this broke installation on Windows. I removed it for the time being, and we may need to look towards the Windows APIs to get the functionality back.

USLTD commented 2 years ago

As far as I know there is one way to get events using Windows APIs. It is done using ReadConsoleInput to read input buffer and detect events in it. To detect resize event we need to capture WINDOW_BUFFER_SIZE_RECORD and then compare it by previous capture.

USLTD commented 2 years ago

I don't have currently access to Windows device but when I will have I will try to implement it.

bczsalba commented 2 years ago

As far as I know there is one way to get events using Windows APIs. It is done using ReadConsoleInput to read input buffer and detect events in it. To detect resize event we need to capture WINDOW_BUFFER_SIZE_RECORD and then compare it by previous capture.

Yeah, that's the way I know it as well. I also don't really have a Windows dev machine at the moment, so I can't really develop it myself.

USLTD commented 2 years ago

The thing is, since ReadConsoleInput captures every event, even key and mouse events, so wouldn't that require reimplementing input/capture system for Windows? Second option would be to instead of Win APIs, run resize detection function in separate thread, but that would consume more CPU.

USLTD commented 2 years ago

Which would be better? I personally think first is better

bczsalba commented 2 years ago

I think the first one is better, but as you said by involving Win APIs we are kind of forcing ourselves to redo a lot of other parts of the input system.

Doing a monitoring thread could work fine, but it caused issues when I tried it, same with the async loop. Probably just me doing the implementation badly, though.

USLTD commented 2 years ago

I think the first one is better, but as you said by involving Win APIs we are kind of forcing ourselves to redo a lot of other parts of the input system. That's true, although I think it will simplify Windows's implementation since ReadConsoleInput also handles MOUSE_EVENT_RECORDs too. Also Textual's win32 contains thread-based handler so why not implement it?

bczsalba commented 2 years ago

I don't want to depend on Textual. We probably will have to implement something similar at some point.

USLTD commented 2 years ago

I don't want to depend on Textual. We probably will have to implement something similar at some point.

I am not saying we should depend on Textual. I am saying that we can implement handler using idea of Textual's handler. That doesn't mean depending on it.

bczsalba commented 2 years ago

The problem is we have to update the project architecture to get the same functionality to work. We still would need to reimplement the handler though, as copying it in with minor modifications doesn't really align with the philosophy of the project.

USLTD commented 2 years ago

Is changing/not changing architecture of project important? Just curious

bczsalba commented 2 years ago

It's not very important, the only reason I generally oppose it is it's a lot of work and with university I'm not sure I have the time for it. If others can pick the project up and do it I'm fully fine with it though, as I'm very uncertain about the library on non UNIX platforms.

USLTD commented 2 years ago

It's not very important, the only reason I generally oppose it is it's a lot of work and with the university, I'm not sure I have the time for it

I don't think that's a lot of work. It won't ever take 1 day to restructure the entire project and it's fine if you don't have time.

If others can pick the project up and do it I'm fully fine with it though, as I'm very uncertain about the library on non UNIX platforms.

That's what contributors are for.