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

masonry: More efficient set of PointerButtons. #334

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

waywardmonkeys
Copy link
Contributor

This brings code over from glazier for a set of PointerButtons that just uses a single value and bit flags rather than a HashSet.

@waywardmonkeys
Copy link
Contributor Author

This builds upon #333. The comment that was removed in #333 from the code in Glazier about PointerButton::None needing to be zero was not brought back as it isn't accurately with the usage of a function to get the bit flag for each variant.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Some suggestions for added utilities, and a docs improvement.


impl std::fmt::Debug for PointerButtons {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "PointerButtons({:05b})", self.0)
Copy link
Member

Choose a reason for hiding this comment

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

Can we display this in full? I.e. Primary | Secondary | ...

We can maybe implement std::fmt::Binary to use this display?

Copy link
Member

Choose a reason for hiding this comment

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

See also linebender/vello#416. I should implement Binary for DebugLayers, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done now as well.

@@ -50,6 +50,84 @@ pub enum PointerButton {
Other,
}

/// A set of [`PointerButton`]s.
#[derive(PartialEq, Eq, Clone, Copy, Default)]
pub struct PointerButtons(u8);
Copy link
Member

Choose a reason for hiding this comment

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

Can we have the same utils as from bitflags (such as |, etc.)

I know we don't want to have different public names for these versions of the values (which is right), so we can't really use bitflags directly.

In theory, I guess we could make PointerButton | PointerButton yield PointerButtons, but I think that's too cute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We barely use PointerButtons right now (only on the PointerState).

If I did this, I'd want to move it to a separate file as it would start to grow too large to just have it in event.rs ... logically, we could do masonry/src/event/pointer.rs and then pub use the stuff from it into masonry::event. It would be easiest then if we left src/event.rs rather than renaming it to src/event/mod.rs ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also starts to get closer to where I'd rather have a new crate called input-types (name not taken yet) or something and go all out there since this isn't specific to Masonry (and I have another version of code like this in my own codebases...)

masonry/src/event.rs Show resolved Hide resolved
@@ -50,6 +50,84 @@ pub enum PointerButton {
Other,
}

/// A set of [`PointerButton`]s.
Copy link
Member

Choose a reason for hiding this comment

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

We should point to the web spec for their equivalent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe! There's some interesting differences between what we do for button / buttons and what the mouse event and pointer events specs say.

https://www.w3.org/TR/pointerevents3/#button-states

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I think we should aim to move more towards pointer events and a cleaner model over time.)

PointerButton::Auxiliary => 0b100,
PointerButton::X1 => 0b1000,
PointerButton::X2 => 0b10000,
// TODO: When we properly do `Other`, this changes
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any ideas about how this might change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't.


/// Returns `true` if all the `buttons` are in the set.
#[inline]
pub fn is_superset(self, buttons: PointerButtons) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

bitflags uses contains for this method. In linebender/vello#416, I renamed the same method to contains

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already a contains for a single button ... I'll rename this to contains_all for now unless you have another suggestion to deal with both.


impl std::fmt::Debug for PointerButtons {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "PointerButtons({:05b})", self.0)
Copy link
Member

Choose a reason for hiding this comment

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

See also linebender/vello#416. I should implement Binary for DebugLayers, I guess.

@flosse flosse added the masonry Issues relating to the Masonry widget layer label Aug 13, 2024
This brings code over from glazier for a set of PointerButtons
that just uses a single value and bit flags rather than a
`HashSet`.
@waywardmonkeys
Copy link
Contributor Author

This is done now, I think ... I didn't add tests, but probably will in the future. But right now, this code is barely even used! (But definitely should be used more.)

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Aug 16, 2024
Merged via the queue into linebender:main with commit c6c1b71 Aug 16, 2024
17 checks passed
@waywardmonkeys waywardmonkeys deleted the pointerbuttons branch August 16, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
masonry Issues relating to the Masonry widget layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants