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

WindowsDriver and CursesDriver's MainLoops use an Action incorrectly #2921

Closed
tig opened this issue Oct 18, 2023 · 8 comments
Closed

WindowsDriver and CursesDriver's MainLoops use an Action incorrectly #2921

tig opened this issue Oct 18, 2023 · 8 comments
Labels
bug v2 For discussions, issues, etc... relavant for v2

Comments

@tig
Copy link
Collaborator

tig commented Oct 18, 2023

The right fix is to change both Windows and Curses' MainLoop implementations to do this:

image

IOW, get rid of the ProcessInput Action alltogether and just have them call ProcessInput on the driver directly.

I tested it and it appears to work fine. Am I missing something (I probably am).

Originally posted by @tig in #2911 (comment)

@tig tig added bug v2 For discussions, issues, etc... relavant for v2 labels Oct 18, 2023
@BDisp
Copy link
Collaborator

BDisp commented Oct 18, 2023

I tested it and it appears to work fine. Am I missing something (I probably am).

Yes you are missing. You can also do it for the NetDriver.
CursesDriver handles differently, because it use file descriptor that will check for revents. If exist a revent then it will call the event attached by the Watch instance.

@tig
Copy link
Collaborator Author

tig commented Oct 18, 2023

I tested it and it appears to work fine. Am I missing something (I probably am).

Yes you are missing. You can also do it for the NetDriver. CursesDriver handles differently, because it use file descriptor that will check for revents. If exist a revent then it will call the event attached by the Watch instance.

I applied the same change to CursesDriver and it seems fine.

  • I made ProcessInput internal on
  • I removed the WinChange Action
  • I added a private reference to CursesDriver to UnixMainLoop
  • I now call _cursesDrivder.ProcessInput (); instead of invoking WinChange?.Invoke()

See #2922

@tig
Copy link
Collaborator Author

tig commented Oct 18, 2023

Related, the file descriptor watching in the CursesDriver is unnecessary functionality. I don't think any apps use it, and even if they do, they can just use native unix APIs or the .NET file system watcher classes instead.

I think we should remove it.

@BDisp
Copy link
Collaborator

BDisp commented Oct 18, 2023

Related, the file descriptor watching in the CursesDriver is unnecessary functionality. I don't think any apps use it, and even if they do, they can just use native unix APIs or the .NET file system watcher classes instead.

I'm curious to see what you'll do. Only to remember that ncurses isn't compatible with .NET. Don't confuse the NetDriver with CursesDriver. Descriptors are native unix APIs.

I think we should remove it.

If you intend to use Console.ReadKey and remove ncurses at all, then is better to remove CursesDriver too.

@tig
Copy link
Collaborator Author

tig commented Oct 18, 2023

Related, the file descriptor watching in the CursesDriver is unnecessary functionality. I don't think any apps use it, and even if they do, they can just use native unix APIs or the .NET file system watcher classes instead.

I'm curious to see what you'll do. Only to remember that ncurses isn't compatible with .NET. Don't confuse the NetDriver with CursesDriver. Descriptors are native unix APIs.

I really mean just making AddWatch/RemoveWatch internal.

I think we should remove it.

If you intend to use Console.ReadKey and remove ncurses at all, then is better to remove CursesDriver too.

That is not my intent at this point.

Can you help me understand what the two AddWatch calls each do?

This one's obvious: It's watching for input:
image

But what is this one doing?
image

@BDisp
Copy link
Collaborator

BDisp commented Oct 18, 2023

Yes the prior is for input and the second is reading for any writing done on the _wakeUpPipes. The later is useful for cancelation or from a post of the threading synchronization, normally by calling the Wakeup method which will interrupt the poll function and thus ensuring a new run loop.

Edit:
Pay attention that the Wakeup method isn't used to cancel a cancellation token but only to wake while awaiting a keystroke. That why I was using in the WindowsDriver and NetDriver the keyReady.Set to wake and not cancellationToken.Cancel.

@tig
Copy link
Collaborator Author

tig commented Oct 19, 2023

Please review changes now in this PR. More to do, but this is a massive simplifier... I think.

@BDisp
Copy link
Collaborator

BDisp commented Oct 19, 2023

Please review changes now in this PR. More to do, but this is a massive simplifier... I think.

I already reviewed. Thanks.

tig added a commit that referenced this issue Oct 21, 2023
* Adds basic MainLoop unit tests

* Remove WinChange action from Curses

* Remove WinChange action from Curses

* Remove ProcessInput action from Windows MainLoop

* Simplified MainLoop/ConsoleDriver by making MainLoop internal and moving impt fns to Application

* Modernized Terminal resize events

* Modernized Terminal resize events

* Removed un used property

* for _isWindowsTerminal devenv->wininit; not sure what changed

* Modernized mouse/keyboard events (Action->EventHandler)

* Updated OnMouseEvent API docs

* Using WT_SESSION to detect WT

* removes hacky GetParentProcess

* Updates to fix #2634 (clear last line)

* removes hacky GetParentProcess2

* Addressed mac resize issue

* Addressed mac resize issue

* Removes ConsoleDriver.PrepareToRun, has Init return MainLoop

* Removes unneeded Attribute methods

* Removed GetProcesssName

* Removed GetProcesssName

* Refactored KeyEvent and KeyEventEventArgs into a single class

* Revert "Refactored KeyEvent and KeyEventEventArgs into a single class"

This reverts commit 88a0065.

* Fixed key repeat issue; reverted stupidity on 1049/1047 confusion

* Updated CSI API Docs

* merge
@tig tig closed this as completed Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug v2 For discussions, issues, etc... relavant for v2
Projects
None yet
Development

No branches or pull requests

2 participants