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

improve argument names in Widget trait #2679

Open
wants to merge 1 commit into
base: master
Choose a base branch
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
Copy link
Contributor Author

edwloef commented Dec 14, 2024

FWIW I've reverted the change with the underscores, only leaving the argument name changes, since that's the one that's caused me more confusion anyways.

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