Skip to content

Fix inconsistent naming of Tree parameter in Widget trait #2679

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

Closed
wants to merge 1 commit into from

Conversation

edwloef
Copy link
Contributor

@edwloef edwloef commented Nov 28, 2024

This removes the underscores in front of the argument names in provided Widget methods, in order to make an IDE-autofilled method signature more useful. This was done in a similar manner to https://github.com/Smithay/smithay/blob/bc1d7320f95cdf17f9e7aa6867cccc5903548032/src/xwayland/xwm/mod.rs#L330-L356

It also renames all instances where the Tree argument was named state, as Tree has a state field. This has led to confusion on my part a few times, with the IDE-autofilled method signature clashing with my own naming, in which case I assumed state was the widget's (downcasted) state, not the widget's tree. I can't think of any reasoning as to why it was named tree vs state in different methods, so it makes sense to me to make it consistent everywhere.

@airstrike
Copy link
Contributor

Assuming this change is desirable, would it not be better to favor #[allow(unused_variables)] over let _ = ...?

@edwloef
Copy link
Contributor Author

edwloef commented Nov 28, 2024

I don't really mind either way, I just copied how Smithay dealt with this issue. If #allow's are preferred I can take care of it as well.

@hecrj
Copy link
Member

hecrj commented Dec 3, 2024

You will rarely need to use all the arguments, so the usefulness is quite subjective.

I prefer to have to opt-in into arguments rather than opt-out.

In any case, this seems like we are adding complexity to the code for better IDE support. Shouldn't the issue be directly addressed in the LSP layer? Maybe a setting in rust-analyzer?

@edwloef
Copy link
Contributor Author

edwloef commented Dec 3, 2024

You will rarely need to use all the arguments, so the usefulness is quite subjective.

You do always need all of them if your widget has child widgets, which to be fair I'm not sure how common that is.

If that's not such a common scenario it would make sense to not remove the underscores, since this is just a small annoyance I noticed while working on a few widgets that had each other as children.

@edwloef edwloef force-pushed the widget-trait-consistency branch from 6883dd7 to 379ad1e Compare December 14, 2024 11:21
@edwloef edwloef closed this Feb 23, 2025
@edwloef edwloef deleted the widget-trait-consistency branch April 1, 2025 11:00
@edwloef edwloef restored the widget-trait-consistency branch May 18, 2025 08:33
@edwloef edwloef changed the title improve argument names in Widget trait Fix inconsistent naming of Tree parameter in Widget trait May 18, 2025
@edwloef
Copy link
Contributor Author

edwloef commented May 18, 2025

I've reopened this to only include the fix for #2949 (EDIT: it didn't reopen for some reason? I'll open a new PR)

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