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

refactor(core): simplify usage of app event and window label types #1623

Merged
merged 7 commits into from Apr 27, 2021

Conversation

lucasfernog
Copy link
Member

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • New Binding Issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@lucasfernog lucasfernog requested a review from a team as a code owner April 26, 2021 02:54
@lucasfernog lucasfernog requested review from a team April 26, 2021 02:54
@chippers
Copy link
Member

0bfd525 changes all the interfaces that might not need an owned tag into a new trait, TagRef. Some additional bounds are also required when wanting to use it as a key into an internal hashmap e.g. P::Event: Borrow<E>.

If an item is going to be stored (like an Event or Label tag in an event Listener), we accept a Into<Event>.

I think that commit covers all the event stuff, I'll double check to make sure there's no window labels lying around in the API that need the same treatment.

@chippers
Copy link
Member

chippers commented Apr 26, 2021

Looks like Labels are already mostly handled internally and I already updated the once place where it's passed into a Manager.

This PR is probably good once the tests pass. I don't think this is actually a breaking change, because it previously accepted &String because P::Event was String and it required &P::Event. The new bounds still accept a &String.

edit: nope, .emit(&"event".into(), None) is breaking, solution: .emit("event", None)

@lucasfernog
Copy link
Member Author

lucasfernog commented Apr 27, 2021

@chippers any chance we can also accept owned values where &Event or &Label is being used (e.g. get_window or emit)? looks just a little weird writing .emit(&MyEvent::Kind) :(

I tried and it seems like it's not easy or even possible :/

@chippers
Copy link
Member

@chippers any chance we can also accept owned values where &Event or &Label is being used (e.g. get_window or emit)? looks just a little weird writing .emit(&MyEvent::Kind) :(

I tried and it seems like it's not easy or even possible :/

I'll look into it tonight or tomorrow. It would look less weird if the event or label was already stored in a variable. I chose to take the reference because those items don't necessarily clone, so this is the only way to absolutely prevent an allocation by the end user.

imo immediately referencing a newly created value isn't too weird anyways. I'll still take a look at possibilities tonight/tomorrow

@lucasfernog
Copy link
Member Author

@chippers any chance we can also accept owned values where &Event or &Label is being used (e.g. get_window or emit)? looks just a little weird writing .emit(&MyEvent::Kind) :(
I tried and it seems like it's not easy or even possible :/

I'll look into it tonight or tomorrow. It would look less weird if the event or label was already stored in a variable. I chose to take the reference because those items don't necessarily clone, so this is the only way to absolutely prevent an allocation by the end user.

imo immediately referencing a newly created value isn't too weird anyways. I'll still take a look at possibilities tonight/tomorrow

alright, don't waste too much time on it :P

@chippers
Copy link
Member

In short, we can allow MyEvent::Foo and &MyEvent::Foo with the downsides of:

  1. A Cow is created for every call
  2. String, &String can no longer be used without turbofishing

Since allowing String, &String, and &str to be used directly is more important, no additional changes will be made. See HashMap::get for a similar api mechanics to the one we are exposing now.

let map: HashMap<MyEvent, String> = HashMap::new();
map.get(&MyEvent::Foo);

is similar to

window.emit(&MyEvent::Foo, None);

This PR is ready if there are no more reviews

@lucasfernog lucasfernog merged commit 181e132 into dev Apr 27, 2021
@lucasfernog lucasfernog deleted the refactor/simplify-label-event branch April 27, 2021 14:52
chippers added a commit that referenced this pull request Apr 27, 2021
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.

None yet

2 participants