Docs clarification and solve #2094. Unwrap TiledImage closure.

[Aiosa]

Docs clarification and fix bug & implement propositions ~ issues #2133, #2094 . Move closure implementation of TiledImage to private members. Fix some tests.

[Aiosa]

I realized that the POST data feature does not necessarily require the format described in the docs, so I updated it to be more accurate.

[Aiosa]

Ah, the fix broke one test that needs fix too, stupid me should've checked earlier.

[Aiosa]

Ok, it is ready. I tried to even fix unrelated failing tests - mostly it was bad test design.
There are still two failing test suites, one looks like the test is wrong, not the implementation. Other I am not sure. In case I should do something about it let me know.

One test expects OSD to fire drag-end when the mouse did not move but a click occurred.
Other expects black pixel on tiled multiimage where a black letter was added. Did not really check whether it reads correct position, hard to say...

[Aiosa]

This reminds me: I decided to make coverage... methods 'static', since they do not touch TiledImage instance... but if you want them as class methods instead I can change it.

[iangilman]

This is fantastic! Thank you for the thorough and detailed job here, and thank you for fixing the related things you found along the way!

Thank you for fixing the unrelated tests as well. Are you up for fixing the last test as discussed in Discord? You're right that it's testing the wrong thing: We don't want drag-end to fire if there has been no movement.

As for making the coverage functions class functions instead of instance, seems reasonable... I don't feel strongly one way or the other.

One minor quibble: Fixing #2133 is such a huge thing, it would have been nice to have #2094 in a different pull request so they don't get mixed up together. We can run with it as is, but something to keep in mind for next time.

Anyway, this is a fantastic step forward! Seems to be functioning smoothly. I think it's all good (except for one minor formatting comment); let me know about that test.

[Aiosa]

I was also thinking about replacing $.TiledImage... with this.constructor... ..?

[iangilman]

I was also thinking about replacing $.TiledImage... with this.constructor... ..?

I think naming the class like we're doing is more clear from a readability standpoint. That said, if you think it's ugly, maybe that's an argument for making them instance methods instead of class methods. I don't know that we have any need for them to be class methods.

[iangilman]

Thank you for the fixes. Everything looks great! I think this is ready to merge if you're happy with it, but I'm also happy with you making those class methods become instance methods if you like. Just let me know :)

[Aiosa]

Ok, I will change them to instance methods :)

[Aiosa]

Ready.

[iangilman]

Looks great! Thank you again for doing this :)

[msalsbery]

You're right that it's testing the wrong thing: We don't want drag-end to fire if there has been no movement.

I noticed this comment in my notifications, just getting back on it now...

drag-end always fires if there was a drag-start, regardless of movement. The reasoning is drag-start can be used to set up many things for a drag operation, and without a corresponding drag-end, detecting the proper place to tear down anything set up in drag-start is pretty difficult - way more difficult then checking if there was any movement, which isn't included in the drag-end event anyway.

Anyway, that's why the test(s) were written that way

[iangilman]

@msalsbery Good to see you! You might be interested in #2064 where I made it so we don't fire drag-end if there haven't been any drag events. There's no drag-start that I'm aware of; are the drag events what you were referring to?