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

[Breaking Change] Refactor Event system and update Widgets accordingly. Add new multi-line TextEdit widget. Remove multiple crate deps, and more. #703

Merged
merged 100 commits into from
Jun 7, 2016

Conversation

mitchmindtree
Copy link
Contributor

@mitchmindtree mitchmindtree commented Mar 2, 2016

WIP - do not merge yet.

These changes are a follow up on the implementation of conrod's new event-based user input system in #684 as discussed in #670.

Related todo:

  • Have GlobalInput take drag_threshold as a push_event argument rather than storing it to ensure the value is always up-to-date.
  • Track widget_under_mouse in InputState on mouse_move, resize and scroll so that we can use it to track widget-specific MouseClick etc events properly.
  • Consider inlining InputState::update and event interpretation within GlobalInput::push_event as they're only used once in a very specific area (Ui::handle_event).
  • Provide a backend::ToEvent trait which allows users to pass custom events to Ui::handle_event, as long as they implement the ToEven trait.
  • Keep track of the widget that was under the mouse when each button was pressed by storing a Option<widget::Index> for ButtonPosition::Down.
  • WidgetInput::all_events currently takes global_input.start - shouldn't it take global_input.current? Ahh, I see that InputState::update was being called as each event was emitted. As long as we track widget_under_mouse during handle_event and within each UiEvent where necessary, I think we can remove the incremental updating of the InputState in the WidgetInput event iterator.
  • MouseClick should track the widget that was directly under it at the time so that it can be delivered to the correct widget (rather than whatever happens to be under the mouse at current when Ui::set_widgets is called).
  • Ensure that all Input types are exposed from backend module (so that users don't need to import piston-input crate).
  • Fix bug where widgets still receive Clicked events when covered by another widget (should be using Graph::pick_widget method).
  • Change module structure from:
events/
    global_input
    input_provider
    input_state
    widget_input
    ui_event

to

event
input/
    global
    provider
    state
    widget
  • Review behaviour of widget_input::should_provide_event. Edit: Removing this along with an overhaul of the input::widget module.
  • Review the Widget::drag_area method and related types and see if these can be removed in favour of using the new Drag events.
  • Record the precise time each event was received and store it with each event (see Track the Instant at which each UiEvent is created once Rust 1.8 lands #708). Just tracking the time of the last Click event now in order to generate DoubleClicks, will hold off on Track the Instant at which each UiEvent is created once Rust 1.8 lands #708 until it is necessary.
  • Remove Ui::Raw and Widget::Raw in favour of adding more event variants that correspond with each of the raw event variants. This should simplify the event API for new conrod users.
  • Remove all manual capturing and un-capturing of the mouse - this should now be handled entirely within handle_event.
  • Remove old UserInput type and related data in Ui.
  • Move text-layout logic from glyph_cache module into a new text module at the top layer. Add a way to produce iterator's yielding Rects for both line and character position:
    • pub mod char:
      • widths -> Widths
      • xs -> Xs
    • pub mod line:
      • struct Info { start, break, width }
      • struct IndexPair { start, end }
      • enum Break { Newline(end, next_start), Wrap(end, next_start) }
      • struct Infos { .. } // Iterator yielding Infos
      • fn rects(infos, bounding_rect, x_align, y_align, max_width, line_spacing) -> Rects
      • fn char_rects(Rects, text, cache, font_size) -> CharRects
        pub mod cursor:
      • fn selection_rects(lines_with_rects: I, text, cache, font_size, start, end) -> SelectionRects
  • Create Scroll events for scrollable widgets on MouseScroll. If not over any scrollable widgets, emit Scroll events to the widget capturing the mouse if there is one.
  • Add Ui::scroll_widget and UiCell::scroll_widget methods that allow for the user (or other widgets) to emit Scroll events to other widgets.
  • Apply Scroll events to scrollable widgets during the widget::set_widget function.
  • Remove all built-in Scrollbar logic into a new Scrollbar widget that emits Scroll events to its associated widget via the UiCell::scroll_widget method.

Decided to leave the following three points for now in favour of doing #711 instead.

Events (inspired by DOM events):

  • Click
  • Drag
  • Scroll
  • DoubleClick

Decided to leave the following events for a future PR or until they're needed.

  • MouseOver (widget only) // Mouse moves over the widget _or_ one of its children
  • MouseOut (widget only)
  • MouseEnter // Mouse moves over a widget
  • MouseLeave
  • DragStart
  • DragEnd
  • DragEnter
  • DragLeave
  • DragCancel // Cancelled via pressing Esc or some other means.

Widgets that still need to be changed over to the new system:

  • Button
  • DropDownList
  • EnvelopeEditor
  • NumberDialer
  • Scrollbar (new)
  • Slider
  • Tabs (maybe?)
  • TextBox
  • TextEdit (new)
  • TitleBar
  • Toggle
  • XYPad
  • CircularButton (from custom_widget.rs example)

Closes #708.
Closes #724.
Closes #711. Scroll logic is still built in, however the Scrollbar was moved into its own Widget.
Closes #642.
Closes #639.
Closes #602.
Addresses some parts of #342.

@psFried
Copy link
Contributor

psFried commented Mar 2, 2016

@mitchmindtree Just got back and saw your comment on #700. Yeah, the purpose of creating drag events on mouse release was so widgets could differentiate between in-progress and completed drags. I certainly don't mind the idea of separate event for Drag and DragEnd or something like that.

On the names, MouseClick to Click, etc.: I went back and forth on that. The context that I'm thinking about is how would we want this to work if a user uses a controller or joystick to click. Or, if there's programmatically generated click events, or touch events on a touchscreen. I don't know how those things might be provided as raw input events. I originally decided to go with MouseClick over Click because the raw input events are also mouse specific. Thinking about it now, though, I'm thinking that Click is really better because it could be caused by things other than a mouse.

@mitchmindtree
Copy link
Contributor Author

Going to be away for the weekend, but will get back to this on Monday 👍

@mitchmindtree
Copy link
Contributor Author

I've been working on improving a DSP API over the past week, but will hopefully get back to this within the next couple days.

@bvssvni
Copy link
Member

bvssvni commented Mar 27, 2016

Updated to latest Gfx. Here is a dump from Eco:

    "dependencies": {
      "pistoncore-input": {
        "bump": {
          "old": "0.8.0",
          "new": "0.9.0"
        }
      },
      "piston2d-graphics": {
        "bump": {
          "old": "0.15.0",
          "new": "0.16.0"
        }
      }
    },
    "dev-dependencies": {
      "piston_window": {
        "bump": {
          "old": "0.37.0",
          "new": "0.39.0"
        }
      },
      "piston": {
        "bump": {
          "old": "0.17.0",
          "new": "0.19.0"
        }
      }
    }

@mitchmindtree
Copy link
Contributor Author

Thanks!

…o ensure it is up-to-date. Inline one-time use methods to clarify their purpose (use functions in tests for internal re-use).
…_xy. Track widget_under_mouse in InputState and update on resize, mouse_move and scroll
…nder event. Update to input_state mouse api changes. Identify Ui fields that should be removed.
…ous name changes. Changed pressed_button to pressed which now returns an iterator yielding all pressed buttons. Added notes on logic that needs to be reviewed in terms of expected behaviour.
InputState::update and GlobalInput::push_event into Ui::handle_event in
order to make GlobalInput::push_event a little less ambiguous.

Change handle_event to take `E: Into<Event<Input>>` instead of `E:
GenericEvent` in order to make event interpretation logic a little more
ergonomic. Not sure if this is a good idea or not, will think over this
as work continues.
… events to the Ui::handle_event method. A blanket implementation is provided for all E: GenericEvent, to provide default compatibility with all piston events.
…pls and functions at beginning of file rather than end.
…nput here, so that this is the only module pistoncore-input is used explicitly (should make it easier to change out in the future if necessary)
Change the pistoncore-input crate name from input to piston_input and
change the piston-2dgraphics crate name from graphics to
piston_graphics in order to highlight the use of the crates so that we
can easily keep them restricted to the backend modules.
…t::Drag to cover a wider range of use cases.
- Added new `event::Widget` type for events that are targeted towards a specific widget.
- Removed `input::Provider` trait in favour of direct `impl`s.
- Refactored `Drag` event to provide `origin`, `from`, `to`, `delta_xy`, `total_delta_xy`.
… it is not

being interacted with.

Removed old unused mouse module.

Removed consideration of input capturing in determination of `depth_order`.

Added a `Scrolls` event iterator to the Widget input for only yielded `Scroll`
events.

Change scrolling so that all scrolling is applied to the widget during the
`widget::set_widget` stage via `Scroll` events, rather than as soon as they are
received within the `Ui::handle_event` method.

Fix scrolling logic in `Ui::handle_event` so that scroll events aren't emitted
multiple times to the same widget.

Added a `pending_scroll_events` `Vec` to the `Ui` for storing `Scroll` events
emitted by widgets during a call to `Ui::set_widgets`.

Add `ScrollBar`s to all_widgets.rs and canvas.rs examples.
longer necessary now that a `Scrollbar` is a `Widget`, rather than built
into the `Ui` graph itself.

Add a Widget::crop_kids method and use this for cropping within
backend/graphics.rs instead of scrolling. Widget::scroll_kids now calls
the `crop_kids` method internally.

Use G2dTexture type directly in all_widgets example rather than
GfxGraphics' associated Texture type.
…mpiling all_widgets example. Goes away when the call to Widget::set is removed on the TextBox widget. Have not yet determined why.
@mitchmindtree
Copy link
Contributor Author

mitchmindtree commented Jun 2, 2016

Alright, I think this PR is fiiiiinally good to go!

I believe the only thing blocking it now is the ICE occurring when rustc attempts to compile the all_widgets.rs example. I've posted an issue for this here.

As this PR involved refactoring most of the foundational logic in conrod and in turn updating all of the widgets, it made sense to address some other issues at the same time.

One of the biggest changes is the addition of the TextEdit widget. This widget allows for multi-line text editing including selection, deletion, cursor movement (Up and Down are still not implemented), horizontal alignment, line spacing etc. It is largely the same as the Text widget (it actually uses it internally to display the text) however also offers editing capabilities. The TextBox widget is now vastly simplified and uses a TextEdit widget internally. The TextEdit is still missing some features including:

  • Add newline on Return (not sure whether we should enter insert a \n, \r or \r\n here? Maybe there's a std feature that does this that I'm not aware of).
  • Up and Down cursor navigation.
  • The react function is never called. We should make a text_box::Event enum which enumerates the different kind of events that might be emitted and have this passed to the react function.

It should also be noted that all text handling logic has been moved from the glyph_cache.rs module into a unique text.rs module at the top level. This module has char, cursor and line modules internally for handling related text logic.

Widgets no longer get Scrollbars automatically when calling scroll_kids, scroll_kids_vertically or scroll_kids_horizontally. Instead, Scrollbar has been added as a new widget. See an example of how they may be used at the Canvas widget in the all_widgets.rs example. This change is beneficial for a few reasons:

  • Allows us to remove scrollbar logic from the core of conrod.
  • Allows users to opt-in to their Scrollbar use (many widgets for specific platforms might not want to expose a scrollbar at all).
  • Allows users to create their own custom scrollbar widgets with unique skins, etc.

Cropping of children widgets is no longer limited to scrollable widgets - any widget may now crop its children widgets by calling the .crop_kids method.

Removes rand, time and vecmath crates, leaving us with only 4 dependencies: daggy, num, pistoncore-input and piston2d-graphics. We should probably also move towards removing our dependency on the deprecated num crate.

All-in-all the new event system has allowed us to remove a significant amount of complexity from most widgets! This PR adds two new widgets (TextEdit and Scrollbar), an entire module for text handling (text.rs) and a new text_edit.rs example yet still removes more code than it adds. One of the key benefits is that widgets no longer have to track their own Interactions and no longer have to manually capture or uncapture input devices, as this is all handled automatically within the conrod Ui as events are received. This helps to cut down on a lot of boilerplate (for an example, check out the diff for the button.rs widget module).

@psFried @bvssvni

@mitchmindtree mitchmindtree changed the title [WIP] Refactoring and tweaking the new event-based user input handling system [Breaking Change] Refactor Event system and update Widgets accordingly. Add new multi-line TextEdit widget. Remove multiple crate deps, and more. Jun 2, 2016
…t it wraps. This removes the need for the State::view method and allows the user to treat the widget::State as if it were the widget\'s unique state. This is especially useful as the primary purpose of the widget::State type is to restrict mutable access to widgets\' unique state, whereas immutable access is fine.
@bvssvni
Copy link
Member

bvssvni commented Jun 2, 2016

I could not resist the temptation and tried modify the "text" example to test the new TextEdit widget. However, I can't figure out the .react closure signature. Could you help me, I just NEED TO SEE IT NOW!! I'm so excited 😄

@bvssvni
Copy link
Member

bvssvni commented Jun 2, 2016

I figured it out, but got the compiler bug on the modified "text" example. 😢

@mitchmindtree
Copy link
Contributor Author

@bvssvni hmmm strange that you got the bug on the modified version, but not the regular version! Looking into it now to see if I can recreate this and work out what's going on.

Not sure how to side-step this issue just yet. It doesn't look like anyone's looking into the ICE yet, so I'll probably spend the day looking into a workaround to use in the meantime.

@mitchmindtree
Copy link
Contributor Author

Hmmm I got the same thing. Simply changing the widgets in the text.rs example from Text to TextEdit, changing the DEMO_TEXT to a mutable String and adding the empty react methods to each widget causes it to ICE. Curiously, removing the call to the .set(*, ui); method stops the ICE from occurring (just like in the all_widgets.rs example), despite it working without issue for widgets in other examples. There seems to be something rustc doesn't like about calling Widget::set on the Text or TextEdit widgets.

@bvssvni
Copy link
Member

bvssvni commented Jun 6, 2016

Nice to make some progress on that compiler bug!

@mitchmindtree mitchmindtree mentioned this pull request Jun 7, 2016
6 tasks
@mitchmindtree
Copy link
Contributor Author

mitchmindtree commented Jun 7, 2016

Alright, ICE is fixed sidestepped! Never been so happy to see that green tick heh!

@bvssvni don't get too excited about TextEdit just yet! Still needs a bit of work I've opened an issue here which has the remaining TODO that came to mind (there's probably more I'm forgetting or haven't thought of). Would love for you to have a play with text_edit.rs and add some points.

In the meantime I'll bump the crate version, publish and merge this!

@mitchmindtree
Copy link
Contributor Author

Published as 0.36.0 🎉

@mitchmindtree mitchmindtree merged commit 1b2cc31 into PistonDevelopers:master Jun 7, 2016
@psFried
Copy link
Contributor

psFried commented Jun 23, 2016

I've been away for a while, but just wanted to say that these changes look really good. @mitchmindtree

@mitchmindtree mitchmindtree deleted the events branch July 31, 2016 06:57
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