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

add pointer.Buttons to MouseClick events #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mixmasala
Copy link

this commit extends ClickEvent to include the corresponding button.

this commit extends ClickEvent to include the corresponding button.
@mixmasala
Copy link
Author

I'm not sure why go fmt is failing

break
}
c.pressed = false
b := c.Buttons ^ e.Buttons // the released button
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean "buttons" in plural?

Use full comment sentences: // The released buttons.

break
}
c.pressed = false
b := c.Buttons ^ e.Buttons // the released button
c.pressed = e.Buttons&pointer.ButtonPrimary > 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "!=" for bitmask operations. A ">" looks like an ordered operation.

@@ -181,17 +185,17 @@ func (c *Click) Events(q event.Queue) []ClickEvent {
if c.pid != e.PointerID {
break
}
c.pressed = true
events = append(events, ClickEvent{Type: TypePress, Position: e.Position, Source: e.Source, Modifiers: e.Modifiers})
c.pressed = e.Buttons&pointer.ButtonPrimary > 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!=

@@ -37,6 +37,8 @@ type Click struct {
clicks int
// pressed tracks whether the pointer is pressed.
pressed bool
// Buttons tracks which buttons are pressed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem quite right. Did you mean

// Buttons specify the set of buttons to track. The zero value is equivalent to the
// primary button, not the empty set.

?

Ah, I see from the rest of your change that I wasn't clear enough in my description. What I meant by the Buttons field is that it should be set by the user from Click, not Click itself. The purpose of (the zero value of) Buttons is maintain the old behaviour where only the primary mouse button would be tracked. Users such as yourself that need to track other buttons can include them in Buttons.

@@ -37,6 +37,8 @@ type Click struct {
clicks int
// pressed tracks whether the pointer is pressed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this change to something like

// pressed tracks whether one or more buttons from the Buttons set is pressed.

?

@@ -131,7 +134,7 @@ func (c *Click) Hovered() bool {
return c.entered
}

// Pressed returns whether a pointer is pressing.
// Pressed returns whether a pointer is pressing the left mouse button.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"primary mouse button". Sorry for renaming the buttons while you're working on this change :)

c.pressed = false
b := c.Buttons ^ e.Buttons // the released button
c.pressed = e.Buttons&pointer.ButtonPrimary > 0
c.Buttons = e.Buttons
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Click shouldn't update its Buttons field. See my comment on the field above.

@whereswaldon whereswaldon force-pushed the main branch 4 times, most recently from 3d36537 to 74ccc9c Compare June 27, 2024 14:39
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