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

Add `localPosition` function to Graphics.Input module for finding mouse position relative to Element position. #307

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@mitchmindtree

mitchmindtree commented Jul 23, 2015

Note: This is the first time I've written any JS - any review of this would be greatly appreciated!

This is a follow up to the discussions here and here in regards to providing a function that exposes the mouse position relative to some given Element. I needed this functionality so that I could design a custom slider widget for my business' web page, so I thought I might as well be the one to have a go at implementing it!

This should be useful for any kind of GUI work that requires knowledge of the mouse/touch position relative to a specific Element (sliders, precision number dialers, knobs, envelope editors, games). I guess it should also make implementing drag-and-drop a bit easier.

I was able to test this locally by drawing a few elements and logging the messages produced by localPosition to the console - everything seems to be working fine. I can add this test to the tests directory if necessary, but I thought it might be out of place as all of the other tests are automated whereas this test requires the user to move the mouse around and check the logs.

The event is currently triggered on any movement of the mouse within the window. It may also be useful to trigger this event when the Element's position changes, however I was unsure how and where would be best to do this within the JS?

Also, apologies for the spaces vs tabs! I didn't realise tabs were used originally until I checked the diff of this PR. I can go through and change them to tabs if you feel its necessary 👍

Again, any feedback appreciated 😸

Edit: Just fixed the indentation to use tabs like the original.

@@ -39,7 +38,7 @@ Elm.Native.Graphics.Element.make = function(localRuntime) {
:
function(elementType)
{
var node = documentCreateElement(elementType);
var node = document.createElement(elementType);

This comment has been minimized.

@mitchmindtree

mitchmindtree Jul 23, 2015

For some reason this was throwing an error in my browser (Chrome) - something along the lines of documentCreateElement is not defined. Changing it to this seemed to fix it.

@mitchmindtree

mitchmindtree Jul 23, 2015

For some reason this was throwing an error in my browser (Chrome) - something along the lines of documentCreateElement is not defined. Changing it to this seemed to fix it.

@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka commented Jul 23, 2015

LGTM 👍

@rrichardson

This comment has been minimized.

Show comment
Hide comment

rrichardson commented Jul 24, 2015

+1

@jahrmarkt

This comment has been minimized.

Show comment
Hide comment
@jahrmarkt

jahrmarkt Jul 24, 2015

Nice,
here's my suggestion :
Add a function
localPositionCollage : ((Float, Float) -> SignalMessage) -> Element -> Element
when you want the position from a Collage-Element. The problem is that you have to keep track of the size of the Collage, if you want to get the position.

jahrmarkt commented Jul 24, 2015

Nice,
here's my suggestion :
Add a function
localPositionCollage : ((Float, Float) -> SignalMessage) -> Element -> Element
when you want the position from a Collage-Element. The problem is that you have to keep track of the size of the Collage, if you want to get the position.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 24, 2015

@evancz closed a couple PRs recently encouraging contributions to external packages as part of a long-term review process. Perhaps circuithub/elm-graphics-shorthand? Otherwise a new elm-graphics-extra sounds useful!

texastoland commented Jul 24, 2015

@evancz closed a couple PRs recently encouraging contributions to external packages as part of a long-term review process. Perhaps circuithub/elm-graphics-shorthand? Otherwise a new elm-graphics-extra sounds useful!

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jul 24, 2015

Contributor

@dnalot, it seems unlikely that (putting this into an ...-extra package) could work here. Note that @mitchmindtree had to touch properties and functions deep within Native/Graphics/Element.js. Really changing existing functions there, not just adding new functions that could as well live in some other module. So how should this work as an extra, independent package?

Contributor

jvoigtlaender commented Jul 24, 2015

@dnalot, it seems unlikely that (putting this into an ...-extra package) could work here. Note that @mitchmindtree had to touch properties and functions deep within Native/Graphics/Element.js. Really changing existing functions there, not just adding new functions that could as well live in some other module. So how should this work as an extra, independent package?

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 24, 2015

If not merged I think it could be tackled as follows (from hacking on ExtJS and iOS). Please fork my Gist in case I delete it later.

texastoland commented Jul 24, 2015

If not merged I think it could be tackled as follows (from hacking on ExtJS and iOS). Please fork my Gist in case I delete it later.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jul 26, 2015

Contributor

@dnalot And then what? Who will call this new newElement method to set up elements with potential for "local position queries"? Note that all element creation functions in Graphics.Element will still ultimately call the newElement method from core's Native/Graphics/Element.js, not from the hypothetical new package's implementation (in file Native/Graphics/Extra/Element.js or so).

Hence, something like

Graphics.Extra.Input.localPosition (message ...) (Graphics.Element.show "abc")

would not work, since the element created by Graphics.Element.show was created with the wrong/old newElement method.

(Of course, this does not just pertain to element creation, but also to other functionality in Native/Graphics/Element.js, which is not simply replaced by methods from Native/Graphics/Extra/Element.js just because that latter file now exists.)

Okay, so then let's recreate the whole API of core's Graphics.Element stuff in the new elm-graphics-extra package, and force the programmer to write the above as

Graphics.Extra.Input.localPosition (message ...) (Graphics.Extra.Element.show "abc")

instead? Hmm, not really workable either. It will fail as soon as the programmer wants to additionally use some other package that also creates Elements and that does so by using Element from the core package. So the poor programmer who wants to use Graphics.Extra.Input.localPosition would now also have to go and replicate that third-party package in order to swap out all its uses of Element stuff from core for ones that use the new element stuff from elm-graphics-extra, ...

So, I still think that such a change (like adding localPosition) is impossible in some extra package. It needs to be done in core or it won't exist.

Contributor

jvoigtlaender commented Jul 26, 2015

@dnalot And then what? Who will call this new newElement method to set up elements with potential for "local position queries"? Note that all element creation functions in Graphics.Element will still ultimately call the newElement method from core's Native/Graphics/Element.js, not from the hypothetical new package's implementation (in file Native/Graphics/Extra/Element.js or so).

Hence, something like

Graphics.Extra.Input.localPosition (message ...) (Graphics.Element.show "abc")

would not work, since the element created by Graphics.Element.show was created with the wrong/old newElement method.

(Of course, this does not just pertain to element creation, but also to other functionality in Native/Graphics/Element.js, which is not simply replaced by methods from Native/Graphics/Extra/Element.js just because that latter file now exists.)

Okay, so then let's recreate the whole API of core's Graphics.Element stuff in the new elm-graphics-extra package, and force the programmer to write the above as

Graphics.Extra.Input.localPosition (message ...) (Graphics.Extra.Element.show "abc")

instead? Hmm, not really workable either. It will fail as soon as the programmer wants to additionally use some other package that also creates Elements and that does so by using Element from the core package. So the poor programmer who wants to use Graphics.Extra.Input.localPosition would now also have to go and replicate that third-party package in order to swap out all its uses of Element stuff from core for ones that use the new element stuff from elm-graphics-extra, ...

So, I still think that such a change (like adding localPosition) is impossible in some extra package. It needs to be done in core or it won't exist.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 26, 2015

The criticism about not interacting with other Element APIs has to do with Elm's design. It's relevant to every -extra package. I proposed an API level change (in a package I'm working on) to fix that. We can easily marshall between though. You can also proxy every function you need to call as I did here. I'm doing something similar on my own. The thrust is Elm has been conscientiously conservative about not adding API without vetting it apart from the official Core library. Maybe it would be helpful to consider how to make the JavaScript parts more extensible? I saw a similar difficulty adding Date.now. Regardless I'm just trying to help by letting you know the resolution of similar issues and an alternative strategy. Code looked good except for minor white space changes contrary to Elm's convention separating functions with two blank lines.

texastoland commented Jul 26, 2015

The criticism about not interacting with other Element APIs has to do with Elm's design. It's relevant to every -extra package. I proposed an API level change (in a package I'm working on) to fix that. We can easily marshall between though. You can also proxy every function you need to call as I did here. I'm doing something similar on my own. The thrust is Elm has been conscientiously conservative about not adding API without vetting it apart from the official Core library. Maybe it would be helpful to consider how to make the JavaScript parts more extensible? I saw a similar difficulty adding Date.now. Regardless I'm just trying to help by letting you know the resolution of similar issues and an alternative strategy. Code looked good except for minor white space changes contrary to Elm's convention separating functions with two blank lines.

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 26, 2015

PS I failed to address your comment about the proxy API looking horrible. Naturally you would just do import Graphics.Element.Extra as Gx or something. I can help with marshalling if you guys are interested.

texastoland commented Jul 26, 2015

PS I failed to address your comment about the proxy API looking horrible. Naturally you would just do import Graphics.Element.Extra as Gx or something. I can help with marshalling if you guys are interested.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Jul 26, 2015

Contributor

Sorry, I'm afraid we are misunderstanding each other. My point here is that it is not enough to "proxy every function I [if I were the author of an -extra package that wants to change the behavior of Elements] need to call". The point is that other, completely unrelated packages call functions that create (normal) Elements, and I would have no way to change that, or make them call other functions or go through any proxies.

Say I want to implement localPosition. It turns out I need to change how the Element objects in JavaScript are created. I'm barred from changing how they are created in core, because I'm supposed to implement my stuff in an -extra package instead. So, sure, I can create a new package, with a new module, and give it a native implementation that creates a slightly different kind of element (which is prepared to support my localPosition function). I can even reuse much of the core functionality, by writing my own functions as proxies that just add a bit here and there, and otherwise call the corresponding functions from core.

But then, someone imports both my -extra package and some other package that deals in Elements. That other package has nothing to do with localPosition or with my package. But it contains some functions that give back Elements. So someone wants to apply localPosition on those Elements. But those Elements were created (by the other library) using the standard core functions for dealing in Elements. They never went through my proxies from the -extra package. So when my localPosition function meets them, it doesn't find what it expects, and crash.

I don't see how your proposal addresses this, but maybe I'm not seeing the full picture of it. Anyway, if your other library is indeed tackling a similar problem, and comes up with a working solution, maybe it can be ported here.

(Asides: I don't think this is "relevant to every -extra package". There are plenty that work fine without any such problems, because they are not trying/having to change a core type. Also, I didn't have a concern about the API looking horrible. Of course, qualified imports address this. The issue was not about writing Graphics.Extra.Input.localPosition (message ...) (Graphics.Extra.Element.show "abc") vs. Graphics.Extra.Input.localPosition (message ...) (Graphics.Element.show "abc"). The issue was about the fact that the show call in question might not be under my control. It might be an Element that comes from a third-party library, and I don't have the power to make that other library use functions from Extra instead of from core.)

Contributor

jvoigtlaender commented Jul 26, 2015

Sorry, I'm afraid we are misunderstanding each other. My point here is that it is not enough to "proxy every function I [if I were the author of an -extra package that wants to change the behavior of Elements] need to call". The point is that other, completely unrelated packages call functions that create (normal) Elements, and I would have no way to change that, or make them call other functions or go through any proxies.

Say I want to implement localPosition. It turns out I need to change how the Element objects in JavaScript are created. I'm barred from changing how they are created in core, because I'm supposed to implement my stuff in an -extra package instead. So, sure, I can create a new package, with a new module, and give it a native implementation that creates a slightly different kind of element (which is prepared to support my localPosition function). I can even reuse much of the core functionality, by writing my own functions as proxies that just add a bit here and there, and otherwise call the corresponding functions from core.

But then, someone imports both my -extra package and some other package that deals in Elements. That other package has nothing to do with localPosition or with my package. But it contains some functions that give back Elements. So someone wants to apply localPosition on those Elements. But those Elements were created (by the other library) using the standard core functions for dealing in Elements. They never went through my proxies from the -extra package. So when my localPosition function meets them, it doesn't find what it expects, and crash.

I don't see how your proposal addresses this, but maybe I'm not seeing the full picture of it. Anyway, if your other library is indeed tackling a similar problem, and comes up with a working solution, maybe it can be ported here.

(Asides: I don't think this is "relevant to every -extra package". There are plenty that work fine without any such problems, because they are not trying/having to change a core type. Also, I didn't have a concern about the API looking horrible. Of course, qualified imports address this. The issue was not about writing Graphics.Extra.Input.localPosition (message ...) (Graphics.Extra.Element.show "abc") vs. Graphics.Extra.Input.localPosition (message ...) (Graphics.Element.show "abc"). The issue was about the fact that the show call in question might not be under my control. It might be an Element that comes from a third-party library, and I don't have the power to make that other library use functions from Extra instead of from core.)

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 6, 2015

Contributor

Mentioning elm-lang/core#332, so this PR shows up there for potential further discussion.

Contributor

jvoigtlaender commented Aug 6, 2015

Mentioning elm-lang/core#332, so this PR shows up there for potential further discussion.

@changlinli

This comment has been minimized.

Show comment
Hide comment
@changlinli

changlinli Apr 20, 2016

Any note on the status of this? Is the unimplemented trigger on Element movement a blocker? This is a rather useful piece of functionality that I would be sad to see wither away on the vine.

changlinli commented Apr 20, 2016

Any note on the status of this? Is the unimplemented trigger on Element movement a blocker? This is a rather useful piece of functionality that I would be sad to see wither away on the vine.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Apr 20, 2016

Contributor

@changlinli, it's not even clear (to me, at least) whether Graphics.Element (with Graphics.Input) will still be a supported thing in the next release of Elm. The mailing lists contain some discussion of this, and one message I took from that is that Graphics.Collage may survive in some form, but Graphics.Element less likely so.

Contributor

jvoigtlaender commented Apr 20, 2016

@changlinli, it's not even clear (to me, at least) whether Graphics.Element (with Graphics.Input) will still be a supported thing in the next release of Elm. The mailing lists contain some discussion of this, and one message I took from that is that Graphics.Collage may survive in some form, but Graphics.Element less likely so.

@category

This comment has been minimized.

Show comment
Hide comment
@category

category Apr 20, 2016

@jvoigtlaender Re potentially dropping Graphics.Element, could you possibly provide a link to the said discussion? Depending on where this change is made, as noted here the absolute position of a Form could be something useful to have also.

category commented Apr 20, 2016

@jvoigtlaender Re potentially dropping Graphics.Element, could you possibly provide a link to the said discussion? Depending on where this change is made, as noted here the absolute position of a Form could be something useful to have also.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Apr 20, 2016

Contributor

See the exchange after this message.

Also, linked from that exchange was this thread. There, it is proposed to "deprecate Graphics.Elements" and Evan answers "I'm thinking of doing something like this [for 0.17]."

Contributor

jvoigtlaender commented Apr 20, 2016

See the exchange after this message.

Also, linked from that exchange was this thread. There, it is proposed to "deprecate Graphics.Elements" and Evan answers "I'm thinking of doing something like this [for 0.17]."

@category

This comment has been minimized.

Show comment
Hide comment
@category

category Apr 20, 2016

@jvoigtlaender Thanks for the above - since Graphics.Element is to be replaced by elm-html it would seem there would be no way explicitly use getBoundingClientRect(), or any other JS method for this change.

An interop could possibly do it - getBoundingClientRect() could be referenced in a file called Ports.js included in the package src/ - then the elm-html function would have to make sure

<script type="text/javascript" src="Ports.js"></script>

is included in the output, and use element ids referenced in Ports.js.

category commented Apr 20, 2016

@jvoigtlaender Thanks for the above - since Graphics.Element is to be replaced by elm-html it would seem there would be no way explicitly use getBoundingClientRect(), or any other JS method for this change.

An interop could possibly do it - getBoundingClientRect() could be referenced in a file called Ports.js included in the package src/ - then the elm-html function would have to make sure

<script type="text/javascript" src="Ports.js"></script>

is included in the output, and use element ids referenced in Ports.js.

@mitchmindtree

This comment has been minimized.

Show comment
Hide comment
@mitchmindtree

mitchmindtree Apr 20, 2016

I'd be more than happy to close this in favour of having elm-html provide something like this (perhaps along with other related Signals) if it is indeed becoming the preference over Element 👍 It's generally nicer and less restrictive for the compiler if we can move features like this into a lib.

The reason I originally took the approach of adding this directly to core was that I couldn't see any other way of achieving the same functionality at the time. If it turns out we can now achieve this using Ports, I'm all for it!

mitchmindtree commented Apr 20, 2016

I'd be more than happy to close this in favour of having elm-html provide something like this (perhaps along with other related Signals) if it is indeed becoming the preference over Element 👍 It's generally nicer and less restrictive for the compiler if we can move features like this into a lib.

The reason I originally took the approach of adding this directly to core was that I couldn't see any other way of achieving the same functionality at the time. If it turns out we can now achieve this using Ports, I'm all for it!

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz May 10, 2016

Member

Sorry for the slow reply. The Graphics.Input module did not make the jump to evancz/elm-graphics and the way to get this functionality will look different for HTML when it happens.

Member

evancz commented May 10, 2016

Sorry for the slow reply. The Graphics.Input module did not make the jump to evancz/elm-graphics and the way to get this functionality will look different for HTML when it happens.

@evancz evancz closed this May 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment