Skip to content

Lib: fix #333 #345

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 23, 2015
Merged

Lib: fix #333 #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c26d400
Lib: fix #333
hhugo Jul 22, 2015
c027ecd
Update tyxml_js.ml
hhugo Jul 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions lib/tyxml/tyxml_js.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,10 @@ module Xml = struct
type keyboard_event_handler = Dom_html.keyboardEvent Js.t -> bool
type attrib_k =
| Event of biggest_event_handler
| Attr of Dom.attr Js.t
| Attr of Js.js_string Js.t option React.S.t
Copy link
Member

@Drup Drup Jul 23, 2015

Choose a reason for hiding this comment

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

Would it make sense (from an efficiency perspective) to keep a constant attribute constructor, with the new signal attribute ?

Copy link
Member Author

@hhugo hhugo Jul 23, 2015

Choose a reason for hiding this comment

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

React already has fast path for constant signals. I don't think you'll gain any efficiency adding an extra constructor.

Copy link
Member

@Drup Drup Jul 23, 2015

Choose a reason for hiding this comment

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

Ok.

type attrib = aname * attrib_k

let attr name v =
let a = Dom_html.document##createAttribute(Js.string name) in
a##value <- v;
name,Attr a
let attr name v = name,Attr (React.S.const (Some v))

let float_attrib name value : attrib = attr name (js_string_of_float value)
let int_attrib name value = attr name (js_string_of_int value)
Expand Down Expand Up @@ -78,9 +75,15 @@ module Xml = struct

let attach_attribs e l =
List.iter (fun (n,att) ->
let n = Js.string n in
match att with
| Attr a -> ignore(e##setAttributeNode(a))
| Event h -> Js.Unsafe.set e (Js.string n) (fun ev -> Js.bool (h ev))
| Attr a ->
(* Note that once we have weak pointers working, we'll need to React.S.retain *)
let _ : unit React.S.t = React.S.map (function
Copy link
Member

@Drup Drup Jul 23, 2015

Choose a reason for hiding this comment

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

Assuming that GC of signals on javascript might be one day fixed, shouldn't this use Lwt_react.S.keep ?

Copy link
Member Author

@hhugo hhugo Jul 23, 2015

Choose a reason for hiding this comment

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

I'll just add a comment about it.

| Some v -> ignore(e##setAttribute(n, v))
| None -> ignore(e##removeAttribute(n))) a
in ()
| Event h -> Js.Unsafe.set e n (fun ev -> Js.bool (h ev))
) l

let leaf ?(a=[]) name =
Expand Down Expand Up @@ -190,6 +193,7 @@ module Util = struct

let update_children (dom : Dom.node Js.t) (nodes : Dom.node Js.t t) =
removeChildren dom;
(* Note that once we have weak pointers working, we'll need to React.S.retain *)
let _s : unit React.S.t = fold (fun () msg -> merge_msg dom msg) nodes ()
in ()
end
Expand All @@ -209,10 +213,7 @@ module R = struct
type attrib = Xml.attrib

let attr name f s =
let a = Dom_html.document##createAttribute(Js.string name) in
let _ = Xml_wrap.fmap (fun s -> match f s with
| None -> ()
| Some v -> a##value <- v) s in
let a = Xml_wrap.fmap f s in
name,Xml.Attr a

let float_attrib name s = attr name (fun f -> Some (js_string_of_float f)) s
Expand Down Expand Up @@ -263,6 +264,15 @@ module R = struct

module Svg = Svg_f.MakeWrapped(Xml_wrap)(Xml_wed_svg)
module Html5 = Html5_f.MakeWrapped(Xml_wrap)(Xml_wed)(Svg)
let filter_attrib (name,a) on =
match a with
| Xml.Event _ ->
raise (Invalid_argument "filter_attrib not implemented for event handler")
| Xml.Attr a ->
name,
Xml.Attr
(React.S.l2
(fun on a -> if on then a else None) on a)
end

module To_dom = Tyxml_cast.MakeTo(struct
Expand Down
1 change: 1 addition & 0 deletions lib/tyxml/tyxml_js.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ module R : sig
module Html5 : Html5_sigs.MakeWrapped(Xml_wrap)(Xml)(Svg).T
with type +'a elt = 'a Html5.elt
and type +'a attrib = 'a Html5.attrib
val filter_attrib : 'a Html5.attrib -> bool React.signal -> 'a Html5.attrib
end

module To_dom : Tyxml_cast_sigs.TO with type 'a elt = 'a Html5.elt
Expand Down