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

Initial implementation of Custom Event support #179

Merged
merged 72 commits into from
Dec 28, 2016

Conversation

vipentti
Copy link
Collaborator

This should resolve #173 and enable support for #164.

I have some performance concerns regarding the usage of Dict for keeping track of the listeners and then calling Dict.merge to merge internal and user provided event listeners.

Using Dict was the simplest way I could come up with to allow this. However, this does limit the maximum number of Events to dispatch to 2 currently but this could be changed.

The Dispatch module could be separated into a separate package.

Looking for feedback and suggestions.

@OvermindDL1
Copy link
Contributor

As for dict efficiency, if it matters (I am not convinced it does without benchmarks) but you could always just use a list of tuples. The only access on them would be scanning, which is plenty fast, merging is trivial, and most people are not going to have hundreds of events, and at less than a count of 10 linked lists often outperform dicts in lookup speed as well in other languages I've used. I do like the style from a cursory look though, need to try it out to see how it actually works.

@vipentti
Copy link
Collaborator Author

Changed from Dict to List

This change allows us to have more than 2 dispatched events
per HTML event. This is useful in cases like Button where
you might have a ripple and then in addition you might want to have
a tooltip

@debois
Copy link
Owner

debois commented Aug 19, 2016

I have a thing for benchmarking things, so I went to town on the group implementation. This is the current implementation:

Benchmarking original implementation
empty x 4,320,238 ops/sec ±1.38% (60 runs sampled)
1 x 918,530 ops/sec ±1.61% (60 runs sampled)
2 x 650,735 ops/sec ±0.98% (59 runs sampled)
2-overlap x 517,829 ops/sec ±1.02% (61 runs sampled)
5 x 102,966 ops/sec ±1.24% (60 runs sampled)
5-overlap-1 x 92,734 ops/sec ±0.93% (62 runs sampled)
5-overlap-2 x 81,728 ops/sec ±1.07% (61 runs sampled)
7 x 59,567 ops/sec ±1.63% (59 runs sampled)
10 x 48,067 ops/sec ±1.84% (56 runs sampled)
15 x 49,495 ops/sec ±1.85% (53 runs sampled)

I then fiddled (quite) a bit before arriving at the implementation below, which has a respectable 1-2 orders of magnitude improvement:

Benchmarking specialised partition / unroll-2
empty x 51,357,133 ops/sec ±1.96% (57 runs sampled)
1 x 18,261,686 ops/sec ±1.71% (59 runs sampled)
2 x 9,330,503 ops/sec ±1.69% (59 runs sampled)
2-overlap x 10,529,456 ops/sec ±0.84% (63 runs sampled)
5 x 1,740,235 ops/sec ±1.10% (60 runs sampled)
5-overlap-1 x 1,626,696 ops/sec ±0.62% (62 runs sampled)
5-overlap-2 x 1,417,282 ops/sec ±1.55% (60 runs sampled)
7 x 963,474 ops/sec ±0.94% (62 runs sampled)
10 x 503,858 ops/sec ±0.86% (61 runs sampled)
15 x 503,694 ops/sec ±0.74% (63 runs sampled)

@OvermindDL1 might be right that this is totally irrelevant. I think its important nonetheless: This (and Options.collect) is the "inner loop" of elm-mdl; if people should say "but its too slow", I want to be able to say both "yeah, but virtual-dom is using 80% of the time" and "we shaved off every bit of overhead we could find".

split
    : a
    -> List b
    -> List ( a, b )
    -> List ( a, b )
    -> ( List b, List ( a, b ) )
split k0 same differ xs = 
  case xs of 
    [] -> 
      (same, differ)

    ((k, v) as x) :: xs -> 
      if k == k0 then 
        split k0 (v :: same) differ xs
      else
        split k0 same (x :: differ) xs


group' : List ( a, List b ) -> List ( a, b ) -> List ( a, List b )
group' acc items = 
  case items of 
    [] -> 
      acc 

    [(k,v)] -> 
      (k, [v]) :: acc

    [(k1,v1), (k2,v2)] -> 
      if k1 == k2 then 
        (k1, [v2, v1]) :: acc
      else
        (k2, [v2]) :: (k1, [v1]) :: acc

    (k,v) :: xs -> 
      let (same, different) = 
          split k [v] [] xs
      in 
        group3' ((k, same) :: acc) different


group : List ( a, b ) -> List ( a, List b )
group = group' [] 

@vipentti
Copy link
Collaborator Author

Awesome! I'll update the PR

@OvermindDL1
Copy link
Contributor

@OvermindDL1 might be right that this is totally irrelevant.

Optimizing is fun though! Especially if benchmarked. ^.^

@@ -12,6 +13,7 @@ type Property c m
| Attribute (Html.Attribute m)
| Many (List (Property c m))
| Set (c -> c)
| Listener String (Json.Decode.Decoder m)
Copy link
Owner

Choose a reason for hiding this comment

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

See comments below.

@debois
Copy link
Owner

debois commented Aug 21, 2016

I made some line comments above. The big questions now are: (A) Do we apply this to all components? (B) How would we do that?

Before looking at these, a note on how to proceed: I think this will be a multi-pr project, so I propose we open a new branch dispatch, and merge this PR there?

I think it'd be very nice for users if there were a general mechanism for attaching event-handlers through options, so I'm in favour of applying this to all components, so I'm in favour of (A), but I'd be interested in opinions to the contrary.

For (B), there are currently two kinds of components in elm-mdl:

  1. Html m constructors (e.g., List, Footers, Cards, ...)
  2. TEA components (e.g., Button)

For (2), we can just add the Dispatch trick to whatever messages are already there. For (1), I'm not sure what to do. In plain TEA, I can see two options:

  1. Require the user to have a Dispatch message and handle that in his own update.
  2. Prohibit multiple subscriptions for the same event. (This requires messing with the Dispatch API; I'm not sure if it can be done transparently.)

For parts, I can only think of this:

  1. Wrap the global MDL message type changing this to
type Msg obs 
  = Actual (Parts.Msg Model Obs)
  | Dispatch (Parts.Msg Model Obs) 

and adapt update accordingly.

@debois
Copy link
Owner

debois commented Aug 21, 2016

This just in:

https://groups.google.com/forum/#!topic/elm-dev/7iUYV0KMvJc

So, we might get support for duplicate event handlers directly from elm. I propose we close this for now, then resurrect it if upstream elm doesn't come through?

@vipentti
Copy link
Collaborator Author

Closing this until the fate of duplicate event handlers has been decided officially

https://groups.google.com/forum/#!topic/elm-dev/7iUYV0KMvJc

@vipentti
Copy link
Collaborator Author

Reopening this for #208 as well as due to https://groups.google.com/forum/#!topic/elm-dev/7iUYV0KMvJc not reaching any final conclusions regarding multi event handling

@vipentti vipentti reopened this Aug 27, 2016
This change allows us to have more than 2 dispatched events
per HTML event. This is useful in cases like `Button` where
you might have a ripple and then in addition you might want to have
a tooltip
debois and others added 16 commits September 2, 2016 10:13
Also remove closure from `Material.update`.
To support rewriting `Json.map (g >> f)` --> `Json.map g >> Json.map f`
for the benefit of vdom, primarily in elm-mdl.

For fixed top-level values `f` and `g`, vdom will recognize an `on*`
handler based on the latter as unchanged across two different views, but
not the former.
Changes roughly fall in three categories:

1. Changes necessary for the new container/input option distribution
2. Improvements to documentation
3. Removal of confusing functions from the API.

- [1] Change (internal) .inner -> .input
- [3] Remove Options.dispatch'

  Options.dispatch' connected, say, Options.div to multi-event
  dispatch. However, we currently have no clear use-case for that
  (end-users can do multiple-dispatch in their update; internally we
  provide dispatch through Internal.apply when necessary). It is
  _really_ hard to write appropriate documentation for `dispatch'`
  without risking making users think they need to add in various
  situations where they don't.

  I propose leaving it out (like I did); its trivial to bring back as a
  minor version feature if somebody turns out to need it.

- [1,2] Document Option distribution for input/container elements
- [2] Extensive rewrite of Options documentation
- [3] Moved internal use only values from Options -> Options.Internal.
  (Summary, apply, applyInput, applyContainer, ...)
- [2] Add MIGRATION.md skeleton
- [1] Tooltip.container -> Tooltip.element
  (To avoid clashes with the generic `Option.container` property)
- [1] Components (Button, Textfield, ...) have been been updated to
  support input/container distinction where applicable.

Besides these changes, I've tried to replace use of Internal.Set with
Options.attribute where applicable. I've also tried to introduce
`Json.map g >> Json.map f` over `Json.map (g >> f)` everywhere in new
code related to Dispatch.
Update custom events API and Options
vipentti added a commit to vipentti/elm-dispatch that referenced this pull request Sep 8, 2016
**NB!** Most of the development of this module and its
related usage has occurred in elm-mdl. Particularly
debois/elm-mdl#179

Special thanks to @debois for contributing and helping with the
development of this package
@@ -126,7 +126,7 @@ The 7.0.0 release changes the required boilerplate in two aspects.
case message of
...
Mdl message' ->
Material.update message' model
Material.update update message' model
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be right can it?

Remove outdated Material.update call
Change Material.update
@vipentti vipentti added this to the v8 milestone Oct 7, 2016
@debois debois merged commit 03f5d01 into debois:master Dec 28, 2016
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.

4 participants