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

Hide internal variants of LifeCycle from docs #826

Closed
luleyleo opened this issue Apr 10, 2020 · 7 comments · Fixed by #833
Closed

Hide internal variants of LifeCycle from docs #826

luleyleo opened this issue Apr 10, 2020 · 7 comments · Fixed by #833
Labels
D-Easy just needs to be implemented help wanted has no one working on it yet maintenance cleans up code or docs

Comments

@luleyleo
Copy link
Collaborator

I've been thinking about adding doc(hidden) to RouteFocusChanged and RouteWidgetAdded.
If I understand correctly, those are never really of interest to the user.

One disadvantage could be that they will still be completed by IDEs like Rust Analyzer, same applies however to the Debug... ones so this problem exists already.

Maybe hiding them and renaming to InternalRouteFocusChanged would be an option?

By @cmyr:

We could also do something like prefix them with _.

Also adding a LifeCycle::Internal(InternalLifeCycle) came to my mind.

@cmyr
Copy link
Member

cmyr commented Apr 10, 2020

I think hiding the docs is reasonable; my initial inclination was that it might be helpful for people who are curious about how things work.

I think that prepending things with Internal won't really help the problem of cluttering IDEs. Moving everything to a single Internal namespace would help a bit; so might prepending with _ (if it moves them to the end of the list, and so offscreen, and then means they are unlikely to be presented as suggestions once the user has typed any other character).

So: no strong opinions. I will not stand in the way of progress.

@luleyleo
Copy link
Collaborator Author

I just tried the _ thing and it has the opposite effect, the items are listed first 😓

@luleyleo
Copy link
Collaborator Author

I think LifeCycle::Internal(InternalLifeCycle) with an advice to ignore them in user code is the best choice.
InternalLifeCycle would then be an enum of the LifeCycle::Debug... and LifeCycle::Route... variants.
The documentation for curious folks could then be moved to InternalLifeCycle as well.

@luleyleo luleyleo added breaking change pull request breaks user code D-Easy just needs to be implemented maintenance cleans up code or docs labels Apr 10, 2020
@luleyleo
Copy link
Collaborator Author

Also InternalLifeCycle should probably be marked as #[non_exhaustive]

@luleyleo luleyleo removed the breaking change pull request breaks user code label Apr 10, 2020
@xStrom
Copy link
Member

xStrom commented Apr 10, 2020

I've been thinking about this for the past few days due to my work on focus. I've really started favoring the LifeCycle::Internal(InternalLifeCycle) approach. Same method of internalizing for Event::Internal(InternalEvent).

This approach would keep the public enum free from clutter, especially if we wanted more of precisely targeted events like FocusChanged.

  • Focus means that the focused widget receives keyboard events. Should ancestors even be receiving these? We could route keyboard events internally if there's a focused widget and then send the actual keyboard event only to the focused widget.
  • Active widgets want mouse events, but do all their ancestors need to receive these? Same story, we could tunnel the mouse events via the internal event.

I'm not so sure about #[non_exhaustive] on the enum itself. Getting compile errors because our internal code isn't specifically handling its own events is beneficial. Non-internal people shouldn't even be using it so we shouldn't be afraid of breaking their code. Having #[non_exhaustive] on the enum variants might be worth it though?

@luleyleo
Copy link
Collaborator Author

Same method of internalizing for Event::Internal(InternalEvent)

Seems like a good idea.

route keyboard events internally

Would make sens as well, I haven't thought to much about shortcuts tho which would require keyboard events for widgets without being focused.

Active widgets want mouse events, but do all their ancestors need to receive these?

At least I can't think of a use case where they would need them.

Getting compile errors because our internal code isn't specifically handling its own events is beneficial.

Oh, right. It's probably enough to add a warning that those enums are 'unstable' for ever.

Having #[non_exhaustive] on the enum variants might be worth it though?

Considering that they are never really used in an exhaustive fashion anyways adding #[non_exhaustive] should be fine

@luleyleo luleyleo added the help wanted has no one working on it yet label Apr 11, 2020
@cmyr
Copy link
Member

cmyr commented Apr 13, 2020

I'm happy with this design.

We could also hide the internal type of the enum, by doing something like,

enum Event {
    // ...
    Internal(InternalEvent),
}

pub struct InternalEvent(InternalEventT);

enum InternalEventT {
...
}

I think this is too much though; I think it's generally useful that people have access to internals if the need/want them. Escape hatches are good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D-Easy just needs to be implemented help wanted has no one working on it yet maintenance cleans up code or docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants