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

Bump keyseq to 0.4.0 and add action::trigger* convenience systems, CHANGE NOTATION, and prep for bevy-input-sequence 0.6.0 release. #10

Merged
merged 17 commits into from
Dec 8, 2024

Conversation

shanecelis
Copy link
Collaborator

Hey Elm,

I added some convenience methods to trigger events instead of sending them. But that's not my whopper.

My whopper is that I changed keyseq and published 0.4.0. After a lot of deliberation, I decided to switch to capitalized modifier keys, so Ctrl-A instead of ctrl-A. I also added a feature flag to permit the even more standard Ctrl+A. This change breaks any keyseq users, which right now is only this crate, so I say break 'em while they're small.

My hope is to get bevy-input-sequence out the door and then push my first release of bevy_minibuffer for bevy 0.14 before bevy 0.15 gets published, which may be as soon as this weekend.

If you don't viscerally object to the notation change, let me know how I can help. If you're ok with this PR and permit it, I can publish to crates.io since we're both owners. That's purely an offer to help and expedite if you lack time or access.

Be well!

-Shane

@not-elm not-elm self-requested a review December 7, 2024 08:21
Copy link
Owner

@not-elm not-elm left a comment

Choose a reason for hiding this comment

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

@shanecelis
Thank you for the PR!
I apologize for the delay in responding as I didn't notice the GitHub notification.
There seems to be no problem with the issue.

Not related to this PR, but I thought that the keyseq! brackets would be better as [] instead of ().
The brackets [] will be automatically completed when the user calls the macro by embedding the following example code in the macro's doc comments. (I recently discovered this ( ー̀֊ー́ ))

/// ```
/// keyseq![Ctrl-W Ctrl-D Ctrl-S Ctrl-A]
/// ```
pub fn bevy_pkeyseq(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
}

The fix itself is fine, so if there aren't additional fixes, please merge.
Once the merge is confirmed, I'll handle the release on my end. (If it's urgent, you can go ahead with the release as well.)"

Thank you!

@shanecelis
Copy link
Collaborator Author

shanecelis commented Dec 7, 2024

I definitely don't like the parentheses (). Cargo's formatter takes weird liberties with what's inside, which is why I had switched to curly braces {}. Apparently one of the few differences between those macro notations are that curly braces permit statements while () and [] only permit expressions. key*! are all expressions, so that's another reason to prefer [].

Do you have a link to any docs about the macro [] auto completion? I can put the comment for the code in as you have it, but it doesn't make sense to me. Would the names have to line up? What editor are you using for macro completion?

/// ```
/// bevy_pkeyseq![Ctrl-W Ctrl-D Ctrl-S Ctrl-A]
/// ```
pub fn bevy_pkeyseq(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
}

@shanecelis
Copy link
Collaborator Author

shanecelis commented Dec 7, 2024

Bummer. Cargo format does the same to [] as it does to (). Look at this:

#[rustfmt::skip]
#[test]
fn before_cargo_format() {
    assert_eq!(
        [key![Ctrl-A],
         key! [Ctrl-A],
         key! [ Ctrl-A ],
         key!{Ctrl-A},
         key! {Ctrl-A},
         key! { Ctrl-A },
         key!(Ctrl-A),
         key! (Ctrl-A),
         key! ( Ctrl-A ),
        ],
        [
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
        ]
    );
}

#[test]
fn after_cargo_format() {
    assert_eq!(
        [
            key![Ctrl - A],
            key![Ctrl - A],
            key![Ctrl - A],
            key! {Ctrl-A},
            key! {Ctrl-A},
            key! { Ctrl-A },
            key!(Ctrl - A),
            key!(Ctrl - A),
            key!(Ctrl - A),
        ],
        [
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
            (Modifiers::CONTROL, KeyCode::KeyA),
        ]
    );
}

I really don't like how it adds the space between the dash. I want it to look like how it would look in the manual. How do you feel about curly braces?

@not-elm
Copy link
Owner

not-elm commented Dec 7, 2024

@shanecelis
I use Rust Rover and Vscode and both work.
I found the following page that mentions this behavior.
https://users.rust-lang.org/t/default-delimiters-for-macro-invocation-vs-code-rust-analyzer/96814/3

@not-elm
Copy link
Owner

not-elm commented Dec 7, 2024

I checked it on my end, and indeed, the space is being added.
I feel this problem is surprisingly easy to face, so there might be a workaround, but I think curly braces would be fine.

shanecelis added a commit to shanecelis/keyseq that referenced this pull request Dec 8, 2024
Elm notified me to this
[feature](not-elm/bevy-input-sequence#10 (comment)).
I hope it will still work despite the code block being ignored. It's
ignored because my procedural macros use the `Modifiers` struct which is
located in the `keyseq` crate, so I can't actually have a compilable,
runnable test for the 'bevy' or 'winit' features of keyseq-macros.
@shanecelis shanecelis merged commit f1a3505 into not-elm:master Dec 8, 2024
3 checks passed
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