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

windows: add console input api #196

Closed
wants to merge 1 commit into from

Conversation

aymanbagabas
Copy link
Contributor

@aymanbagabas aymanbagabas commented May 14, 2024

Add Console input API types and syscalls. Since InputRecord contains a
union type, we need some way to access the union members. This is done
here by extending InputRecord to return the respective type based on the
function called.

Fixes #98

@gopherbot
Copy link

This PR (HEAD: 44194b0) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/sys/+/585495.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@aymanbagabas aymanbagabas changed the title windows: add console input handling api windows: add console input api Jun 6, 2024
@gopherbot
Copy link

This PR (HEAD: a9a4b62) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/sys/+/585495.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@aymanbagabas
Copy link
Contributor Author

Here's example code of how to use this. This is based on https://github.com/erikgeiser/coninput/blob/main/example/main.go

package main

import (
	"context"
	"fmt"
	"log"
	"os"
	"os/signal"

	"golang.org/x/sys/windows"
)

func main() {
	con, err := windows.GetStdHandle(windows.STD_INPUT_HANDLE)
	if err != nil {
		log.Fatalf("get stdin handle: %s", err)
	}

	var originalConsoleMode uint32

	err = windows.GetConsoleMode(con, &originalConsoleMode)
	if err != nil {
		log.Fatalf("get console mode: %s", err)
	}

	newConsoleMode := uint32(windows.ENABLE_MOUSE_INPUT) |
		windows.ENABLE_WINDOW_INPUT |
		windows.ENABLE_PROCESSED_INPUT |
		windows.ENABLE_EXTENDED_FLAGS

	err = windows.SetConsoleMode(con, newConsoleMode)
	if err != nil {
		log.Fatalf("set console mode: %s", err)
	}

	defer func() {
		resetErr := windows.SetConsoleMode(con, originalConsoleMode)
		if err == nil && resetErr != nil {
			log.Fatalf("reset console mode: %s", resetErr)
		}
	}()

	ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt)
	defer cancel()

	var read uint32
	events := [16]windows.InputRecord{}
	for {
		if ctx.Err() != nil {
			break
		}

		if err := windows.ReadConsoleInput(con, &events[0], uint32(len(events)), &read); err != nil {
			log.Fatalf("read input events: %s", err)
		}

		fmt.Printf("Read %d events:\n", len(events))
		for _, event := range events[:read] {
			var e any
			switch event.EventType {
			case windows.KEY_EVENT:
				e = event.KeyEvent()
			case windows.MOUSE_EVENT:
				e = event.MouseEvent()
			case windows.WINDOW_BUFFER_SIZE_EVENT:
				e = event.WindowBufferSizeEvent()
			case windows.FOCUS_EVENT:
				e = event.FocusEvent()
			case windows.MENU_EVENT:
				e = event.MenuEvent()
			}
			fmt.Printf("%#v\n", e)
		}
	}
}

@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 2: Code-Review+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 2: -Code-Review


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 2: Code-Review+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

This PR (HEAD: e49ad90) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/sys/+/585495.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 3: Code-Review+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

This PR (HEAD: b1e55df) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/sys/+/585495.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 4: Code-Review+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

Add Console input API types and syscalls. Since InputRecord contains a
`union` type, we need some way to access the union members. This is done
here by extending InputRecord to return the respective type based on the
function called.

Related golang#98
@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 5: Code-Review+1

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@aymanbagabas
Copy link
Contributor Author

Hey @alexbrainman, could you take a look at this when you get a chance? Right now, we're maintaining this in a windows package and would be nice to upstream it.

@alexbrainman
Copy link
Member

Hey @alexbrainman, could you take a look at this when you get a chance? Right now, we're maintaining this in a windows package and would be nice to upstream it.

Hello Ayman.

This is a large CL. So it might take me a long time to review it. It will help if you can break it. Regardless I will review it eventually.

Feel free to ping me on https://go-review.googlesource.com/c/sys/+/585495 to remind me.

Do not ping on #196 , because no one monitors PRs. Your ping will get lost.

Alex

@aymanbagabas
Copy link
Contributor Author

Hello Ayman.

This is a large CL. So it might take me a long time to review it. It will help if you can break it. Regardless I will review it eventually.

Fwiw, most of the additions are just constants from the Windows API. It adds 3 new function calls, a union type (arguably what we need to discuss), and a bunch of constants.

Feel free to ping me on https://go-review.googlesource.com/c/sys/+/585495 to remind me.

Do not ping on #196 , because no one monitors PRs. Your ping will get lost.

Will do

@gopherbot
Copy link

Message from Ayman Bagabas:

Patch Set 5:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/585495.
After addressing review feedback, remember to publish your drafts!

@alexbrainman
Copy link
Member

most of the additions are just constants from the Windows API.

Someone still needs to verify the const values and names against Windows refs. This takes times.

It adds 3 new function calls, a union type (arguably what we need to discuss), ...

This also makes this CL complicated. Because if we are adding something more complicated than Windows API functions, then we need a proposal for these.

Alex

@aymanbagabas
Copy link
Contributor Author

This also makes this CL complicated. Because if we are adding something more complicated than Windows API functions, then we need a proposal for these.

The types are defined in the Windows API. The issue is translating the INPUT_RECORD union event field into Go. The way it's done in this PR is to use the union field names as functions. Otherwise, I could take out the member functions and implement them in a separate library so that we don't need to start a proposal for this.

Windows API definition:

typedef struct _INPUT_RECORD {
  WORD  EventType;
  union {
    KEY_EVENT_RECORD          KeyEvent;
    MOUSE_EVENT_RECORD        MouseEvent;
    WINDOW_BUFFER_SIZE_RECORD WindowBufferSizeEvent;
    MENU_EVENT_RECORD         MenuEvent;
    FOCUS_EVENT_RECORD        FocusEvent;
  } Event;
} INPUT_RECORD;

This PR:

type InputRecord struct {
	EventType uint16
	// Padding of the 16-bit EventType to a whole 32-bit dword.
	_ [2]byte
	Event [16]byte
}

func (ir InputRecord) FocusEvent() FocusEventRecord { ... }
func (ir InputRecord) KeyEvent() KeyEventRecord { ... }
func (ir InputRecord) MouseEvent() MouseEventRecord { ... }
func (ir InputRecord) WindowBufferSizeEvent() WindowBufferSizeRecord { ... }
func (ir InputRecord) MenuEvent() MenuEventRecord { ... }

@alexbrainman
Copy link
Member

The issue is translating the INPUT_RECORD union event field into Go. The way it's done in this PR is to use the union field names as functions.

I briefly looked at your CL. All existing Windows APIs are just functions in the package, not member functions of some struct.

This package intention is to provide exact copy of Windows APIs. So users can just use Microsoft documentation when using this package. Any extra functionality needs to be approved by the Go Team, because they will need to support it in the future - not you.

It is probably OK to add both functions and member functions to access new APIs. But while functions is OK to add without approval (if their name matches API), member functions need approval.

Feel free to seek another opinion.

Alex

@aymanbagabas
Copy link
Contributor Author

It is probably OK to add both functions and member functions to access new APIs. But while functions is OK to add without approval (if their name matches API), member functions need approval.

Just to be clear, we still need proposal approval for the INPUT_RECORD.Event union type member functions even though it matches the Windows API, right?

@alexbrainman
Copy link
Member

Just to be clear, we still need proposal approval for the INPUT_RECORD.Event union type member functions even though it matches the Windows API, right?

I do not know. I need to look at your CL properly to make the decision for myself.

And I am not the one who decides. So you can just create a proposal and not wait for me, if you want your change as is.

Alex

@aymanbagabas
Copy link
Contributor Author

aymanbagabas commented Oct 21, 2024

@alexbrainman Thank you for your feedback and quick replies. I've split this CL into 3 smaller ones 621495, 621515, and 621496 (draft). I'll close this one 🙂

#226
#227
#228

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