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

UIContext #94

Merged
merged 99 commits into from
Jun 26, 2020
Merged

UIContext #94

merged 99 commits into from
Jun 26, 2020

Conversation

AaronM04
Copy link
Member

@AaronM04 AaronM04 commented Dec 10, 2019

This adds a UIContext so that widget logic can happen within the widget definition rather than in client.rs.

  • allow setting handlers
  • Layer and Pane propagate Click events to the appropriate children
  • Layer and Pane propagate all events to the appropriate children
  • switch and activate buttons and checkboxes with keyboard
  • chatbox should use keyboard event system
  • get screen transition buttons to work again
  • no UI actions; widget IDs shouldn't really matter for our code
  • tests pass (except the two network-related ones which we'll fix ASAP)
  • no new warnings

@AaronM04 AaronM04 requested a review from Manghi December 10, 2019 21:25
@AaronM04
Copy link
Member Author

@Manghi I think I'm going to just make this a proof of concept for now because we have two branches making large changes to our UI stuff already. Once #83 and #88 are merged into master, I'll merge master into this branch and do this All The Way(tm).

conwayste/src/ui/context.rs Outdated Show resolved Hide resolved
conwayste/src/ui/context.rs Outdated Show resolved Hide resolved
conwayste/src/ui/context.rs Outdated Show resolved Hide resolved
conwayste/src/ui/pane.rs Outdated Show resolved Hide resolved
@AaronM04
Copy link
Member Author

AaronM04 commented Jan 14, 2020

At this point (unless you have any more review feedback @Manghi ), I'm going to pause this until #83 is merged, so I know how to distribute keyboard events based on focus.

conwayste/src/ui/context.rs Outdated Show resolved Hide resolved
@Manghi
Copy link
Contributor

Manghi commented Jan 15, 2020

At this point (unless you have any more review feedback @Manghi ), I'm going to pause this until #83 is merged, so I know how to distribute keyboard events based on focus.

Probably a good idea since 83 changes how the underlying data is structured. No additional comments on my part for this PR. Great job with the clean up so far. 👍

@Manghi Manghi mentioned this pull request Jan 15, 2020
11 tasks
@AaronM04 AaronM04 marked this pull request as ready for review June 24, 2020 05:18
@AaronM04
Copy link
Member Author

@Manghi ready for your final review! 🚀

@Manghi
Copy link
Contributor

Manghi commented Jun 24, 2020

I've been following your changes along the way and they looks awesome! I'm going to play around with it tomorrow and then do a final general review.

Copy link
Contributor

@Manghi Manghi left a comment

Choose a reason for hiding this comment

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

ABSOLUTELY WONDERFUL JOB. 🎊

I left a dozen comments/questions I had, but most of them are stylistically related. Bravo man. Fixing the UI backend design was not a trivial task, and you've shown time and time again you're up to the challenge.

conwayste/src/ui/context.rs Show resolved Hide resolved
conwayste/src/ui/button.rs Outdated Show resolved Hide resolved
conwayste/src/ui/context.rs Outdated Show resolved Hide resolved
conwayste/src/ui/context.rs Show resolved Hide resolved
Comment on lines +336 to +355
// handle forwarded events
loop {
let mut events = vec![];
std::mem::swap(&mut events, &mut self.$handler_data_field.forwarded_events);
if events.len() == 0 {
break;
}

for event in events {
if let Some(handler_vec) = handlers.get_mut(&event.what) {
// call each handler for this event type, until a Handled is returned
for hdlr in handler_vec {
let handled = hdlr(self, uictx, &event)?;
if handled == Handled {
break;
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Bump!

conwayste/src/ui/pane.rs Show resolved Hide resolved
conwayste/src/ui/pane.rs Show resolved Hide resolved
conwayste/src/ui/textfield.rs Outdated Show resolved Hide resolved
Comment on lines +295 to +296
#[test]
fn test_compile_fail_double_mut_borrow() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this test ever be needed?

Copy link
Member Author

@AaronM04 AaronM04 Jun 26, 2020

Choose a reason for hiding this comment

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

Uncommenting it causes a compile failure -- if it doesn't, something got screwed up with borrowing, and undefined behavior could result.

I don't know of a way to have a test that verifies it can't compile, so I'm just leaving it commented.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW if you find a way to have a test with code that only passes if it fails to compile, that would be amazing! 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks promising: https://crates.io/crates/compiletest_rs
but I think it's overkill for this one test. Fine with leaving it as is.

conwayste/src/ui/ui_errors.rs Outdated Show resolved Hide resolved
@AaronM04
Copy link
Member Author

Github lost what the Bump comment was about.

@AaronM04
Copy link
Member Author

Github lost what the Bump comment was about.

OK I think it's coming back to me -- you had concerns about the potential for infinite loops when handling forwarded events. I think we just have to be aware of that possibility when writing our handlers.

Copy link
Contributor

@Manghi Manghi left a comment

Choose a reason for hiding this comment

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

I believe you addressed all the feedback. Thank you!

Approved and merging it in!

@Manghi Manghi merged commit cd2c2af into master Jun 26, 2020
@Manghi Manghi deleted the aaron/uicontext branch June 26, 2020 06:38
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