blunty666 / Tiles

MIT License
2 stars 3 forks source link

Code Mostly Undocumented #1

Open Fizzixnerd opened 8 years ago

Fizzixnerd commented 8 years ago

Hey there!

Love the demos you gave. Tonnes of great ideas. One problem: I have no idea how to use your code.

I'd like to help document it using luadoc (http://keplerproject.github.io/luadoc/index.html). This would help me understand the code as well, and I think it's generally a good practice. You've obviously spent a lot of effort making this code high quality (with type checks and so forth), so docs are really the only thing you're missing.

Let me know if you wouldn't mind me helping in this small way.

Thanks.

blunty666 commented 8 years ago

Sure sounds like a good idea, I keep planning on writing up some documentation but never get round to doing it.

Using luadoc should speed things up, though I need to see how it works as the tiles API defines a lot of it's functions not in the normal way (it self-generates them when it is first loaded).

Feel free to have a look and let me know what you need to know to get started though!

Fizzixnerd commented 8 years ago

Alrighty cool, sounds good.

It turns out LuaDoc is dead, and one should use LDoc instead. Not a big deal, it's mostly compatible. Either way, it allows for sections, so I can just make a section for each of the generated objects. It shouldn't be too hard.

One thing I'm confused about is the interface presented in guiTiles. Is this written against an older version of tiles? I'm just not sure why you don't give them a surrogate primitive object (say, a non-visible point) to present their interface through.

For example, I can fetch a regular Text by doing (say)

surfaceHandler:GetTileByName("text"):GetObjectByName("myTextObject")

But I can't do that for a FancyText, or it seems anything in guiTiles.

Basically, what I think you should do is something like:

function AddFancyText(tile, x, y, text, colour, name) -- same constructor
args as regular Text
  local ft = oldAddFancyText(tile, x, y, 1, text, colour, 255)
  local ftProxy = tile:AddPoint({x, y}, 0x000000, 0, name)
  ftProxy.SetClientObject = function (self, obj) self.ClientObject = obj end
  -- similarly for GetClientObject
  ftProxy:SetClientObject(ft)
  -- forward all method calls to ftProxy into ft via getClientObject...

Probably a better way would be to add first-class proxies to tiles.lua. But you get the basic idea. Using a non-visible object was just a hack I had that let's you fetch it like normal. I guess you could just use a Tile as a proxy, since then most of the work is done. The point is that that Tile should be in the objects list though, so I can fetch it just like a primitive drawable.

I'm also a little scared, because you use SetUserdata to store data about the objects! I think that's probably a bad idea, mostly because you expose it to the "user", and I wouldn't have realized without reading the source that I'm not allowed to use it myself. Maybe it's not as exposed as I think, I just saw the call and was like "oh dear, I use user data!"

I can write up two pull request for you for these issues if you'd like, but I just wanted to make sure you think a change SHOULD be made first.

Loving the base API by the way. It's super awesome -- very clean. Having an explicit MultiSurfaceHandler is a great idea; I wouldn't have thought of that.

Sure sounds like a good idea, I keep planning on writing up some documentation but never get round to doing it.

Using luadoc should speed things up, though I need to see how it works as the tiles API defines a lot of it's functions not in the normal way (it self-generates them when it is first loaded).

Feel free to have a look and let me know what you need to know to get started though!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/blunty666/Tiles/issues/1#issuecomment-224690474, or mute the thread https://github.com/notifications/unsubscribe/AFnqK_FnYbMyIQCtWrSMUtQ1GZeL_MrIks5qJw6SgaJpZM4IwhxB .

blunty666 commented 8 years ago

I think I see the problem you are talking about with the guiTiles. Are you referring to the fact that you can't access the methods created as part of the guiTiles when you get the tile directly from the surfaceHandler. There are ways to solve this I am looking at now, though the one you suggest where I add it as a primitive drawable would go against the a key design principle where the objects in tiles only refer to individual primitive drawables. Currently looking at how I can fiddle with the metatables in the guiTiles API to solve this, or maybe provide some kind of restricted access to the original metatables from the tiles API.

For the userdata issue you refer to, I don't think there is an issue. When an end user wants to use userdata, they should use the additional methods provided through the tile object, not the original getter / setter on the primitive drawable. This is the same as for all methods the primitive drawable provides, they should not be used, only the ones provided by the tiles API (the ones that start with a capital letter) shoud be used so as not to interfere with the internal workings.