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

Design of new Lua API #4362

Open
15 tasks
smurfix opened this issue Nov 16, 2020 · 21 comments
Open
15 tasks

Design of new Lua API #4362

smurfix opened this issue Nov 16, 2020 · 21 comments

Comments

@smurfix
Copy link
Contributor

smurfix commented Nov 16, 2020

Brief summary of issue / Description of requested feature:

The current heap of semi-related functions does not scale any more. We need an object-oriented design that can be used with a mapper like sol3.

Steps to reproduce the issue / Reasons for adding feature:

  1. The list of available Lua functions is large, unstructured, not object oriented, and no longer discoverable.
  2. Many of these functions have some legacy quirks which we cannot fix or circumvent; adding a digit to create a "new and improved" function doesn't scale, and who can remember that?
  3. Many functions accept an optional initial argument. This doesn't work at all with sol3. Also it either prevents us from adding new optional trailing arguments, or makes that process rather fragile.

Expected result of feature

We have a new object-oriented interface with a handful (or two) of classes, each of which has a handful (or three) of methods / attributes.

Issues to be discussed / resolved

  • Keeping two APIs uptodate is a lot of busy-work.

    • true but the old code would be legacy, bugs warts and all, and won't change much
    • as we implement the new API, we can remove the old code from LuaInterface and rewrite the old functions in Lua
    • should be possible to do this incrementally
  • Lua 5.4?

    • lots of scripts will break, long-term transition required
    • only doing OO with Lua 5.4 is not feasible because Lua 5.1 then couldn't use our new features
    • worse, we couldn't use them ourselves, thus would need to keep the old bindings alive much longer
    • Idea: create a Lua 5.4 version of Mudlet, build it, encourage people to use it for checking whether their scripts still work
    • thus, try to keep Lua 5.4 transition independent of OO transition if at all possible
    • sol3 docs say this should be possible, but who knows
  • Performance

    • OO shouldn't be noticeably slower than the "old" way
    • fewer object lookups might actually make it faster, but who knows
    • the difference should not be noticeable either way but we should probably add some benchmarks just to make sure
  • Versioning

    • Going forward we might need to implement more incompatible changes
    • we might add version numbers to our objects, at first frozen at 1 (or maybe zero until we're "done" converting)
    • in theory, unlikely to be a problem in the next couple of years or so, but in practive we're bound to make misteaks
  • How object-oriented do we want this to be?

    • hierarchical naming = better discoverability, lua display(Room) immediately shows everything you can do with rooms
  • Editing, autocomplete

    • we might need to adapt the autocompleter
    • idea: expanding "myroom." would show all attributes+methods of Room, so if people name their vars intelligently they get a sufficiently-short list
    • shorter accessors can be remembered more easily
  • OOP vs. procedural coding

    • Lua has some OO features but isn't really an OO language
    • Our users typically are not programmers thus might have problems with OO concepts
    • setRoomFoo(roomID, bar) is longer == less memorable than room.foo = bar esp when you only need to get the room once but then set five attributes on it. More expressive code is good.
  • Object Creators

    • positional parameters vs. a table with named params
    • Our old interface mostly does the former, Geyser uses the latter
    • A table requires more typing (names, those braces)
    • But: a table is more expressive: obvious what each parameter does without reading the documentation or counting commas
    • easier to add new / deprecate old parameters without breaking any code

Details

  • Shortlist of classes.

    • Mapping: Room, Area, Map (that'd be a singleton)
    • Communication: IRC, Discord
    • GUI: Window, Label, Console, i.e. a bunch of objects which Geyser currently implements in terms of the old interface

    We need to do a review of

    • what we currently have and how it'll fit the new paradigm
    • whether some restructuring might be appropriate while we're at it

TODO

  • add sol3 to our build infrastructure.
  • decide on toplevel names. Do we add each class to the Lua toplevel directly, or do we create a single toplevel table with all of them?
  • decide on a "simple" test class to start the process with
  • design, refine, implement the class+object APIs
  • implement some old methods with the new objects.
@smurfix
Copy link
Contributor Author

smurfix commented Nov 16, 2020

What I'd like to see: instead of getLineNumber([windowName]) we'd have a Window:at("windowName") function which returns a window userobject that has a lineNumber attribute (no, no get/set functions!). New windows are made by Window:new("name") of course. IMHO creators should adapt Geyser conventions (i.e. Window:new("name", {"whatever":123})
) so that we can add new attributes without shooting ourselves in the foot.

Versioning: my idea is that each of these objects has an interface version number that defaults to 1, a read-only version attribute that says so, and a version_as(X) method that returns the object but otherwise does nothing (if X==1), or returns nil,"unknown version" otherwise. If we ever decide to do an incompatible change we teach versioned to return a different object that supports the "other" API. That way you can pass one of these Window objects to another module, and the recipient can coerce it to the version it knows how to deal with.

@vadi2
Copy link
Member

vadi2 commented Nov 16, 2020

@Mudlet/lua-interface @demonnic @keneanung

@Kebap
Copy link
Contributor

Kebap commented Nov 16, 2020

Seems very interesting on first glance!
Some thoughts:

I like a structured approach after 10+ years of growing api components rather organically.
Still keeping room for further future additions.

Not sure how many % of existing api can be mapped to the suggested classes.
Need to review more detailed.

Would require scripters learn many new tools or new names for old tools, unless they stay on legacy api.

Having two apis simultaneously potentially clutters the auto compete suggestions.

@demonnic
Copy link
Member

Having two apis simultaneously also increases support burden, as now we have to support both methods.
If we don't support both APIs and instead have a cut off where old API ends and new API begins, this is essentially a new Mudlet, and we will have to convince people to change from old mudlet to new mudlet.

On top of that, Lua is not really an OOP language. I know we have a lot of what Lua passes off as OOP floating around, but that's not the same as actually being object oriented.

Also, it's a mistake to think that most Mudlet users are developers or scripters, or that somehow making a new, fully OOP API would be better just for existing and being OOP.

I'm not against wrapping up a lot of our existing functionality into OOP wrappers, or moving to sol3 or any of that, but I also want to make -very- sure this is well thought out and doesn't wind up being a lot of effort for very little gain, or perhaps even a net loss.

@vadi2
Copy link
Member

vadi2 commented Nov 17, 2020

@smurfix would you be able to curate the discussion here - update the OP with all concerns issues and tick them off as we resolve it?

@smurfix
Copy link
Contributor Author

smurfix commented Nov 17, 2020

@vadi2 Sure, can do.

@smurfix
Copy link
Contributor Author

smurfix commented Nov 18, 2020

Summarized the current state of the discussion (and some of my thoughts about it) in the OP.

@vadi2
Copy link
Member

vadi2 commented Nov 23, 2020

  1. We have to think on advantages this brings to the users to sell them on it, because we will be bringing a lot of pain to everybody by making them learn a new way of doing things. People will rightfully ask "why change?" and I feel we don't have enough of good arguments yet. Pretty much the only reason we're considering a new API is because we, developers, are feeling the pain. And we don't build Mudlet for ourselves, we build it for the users, so that's not a good argument.
    I can think of just one improvement for users and that is better discoverability, the lua display(Room) immediately shows everything you can do with rooms bit, but to rewrite an entire API just for that is an ungodly amount of work. Users can already do the same by going to https://wiki.mudlet.org/w/Manual:Lua_Functions and searching for room. Can we collectively think of more and how will it improve things?

  2. The other thing I'd like to keep in mind is performance - that's not on the list yet. The C-like binding is super fast, anything we do will be slower. Can we setup some benchmarks to make sure we don't lose it (much) in the new object-oriented land?

  3. I think we'd have to keep and expand the old API for a while for the new features we add until we have everything in place ready for a transition, then give the community a few years in place to transition, and then freeze the old API in order to keep old scripts working.

  4. Or, we use this as a moment to jump onto the latest Lua version and break backwards compatibility with old scripts - abandoning scripts that are no longer updated and leaving users in the dark. Something to think about.

  5. room.foo = bar is nice and simpler, I agree.

  6. Same with lua display(Room) immediately shows everything you can do with rooms.

  7. Shortlist of classes. good start!

I agree with #4362 (comment) in general. Positive about this, but we need to be super sure it's a net benefit. Can we all think on what benefits this brings to users, and once we're super sure of this, think on a good plan to roll this out?

@smurfix
Copy link
Contributor Author

smurfix commented Nov 23, 2020

The main benefit I see is for us so we can be more proactive adding new and expanding old features. The user benefit is new features and less baggage in the documentation. The latest fail of me trying to implement some expanded feature (like getFgColor returning four numbers) plainly tells me that the current state of affairs is not viable.

Speed: sol3 has a pretty impressive benchmark https://sol2.readthedocs.io/en/latest/benchmarks.html so personally I think that if there's any slowdown just from switching to OO the user won't notice. There's also a chance for speedups, e.g. when you do five things with a room (like add exits) or a console (set colors, background, content) the user only needs to look up the room/console once, not five times, but again I doubt that anybody would notice.

Transition: I don't think we need a "hard" transition. We can implement "Console" (or "Room" or whatever) objects, then implement a room.pos = [x,y,z] attribute, then replace setRoomCoordinates(room_nr,x,y,z) with Lua code (just call Room:by_nr(room_nr).pos = [x,y,z]) and remove the C binding from TLuaInterface.cpp. Rinse and repeat until done. Anything new would only be implemented in OO code; we just need to be careful that the new code actually is a useful and as-stateless-a-possible superset, e.g. instead of a top-level getFgColor() we'd implement a console.at(x,y) function which returns a modifyable table with attributes (content, colors, characters until attrs change) which you can use to implement the former. (No discussion on the details of this interface please, this is an example.)

The "hard transition" thing is an issue if/when we want to switch to a newer Lua version. IMHO the only realistic way to do this is to build both a Lua-5.1 and a Lua-5.4 Mudlet, ask people to please test their code with both, keep that up until everybody agrees that their scripts work with 5.4, then relegate the 5.1 branch to maintainance+serious-bugfix-only status.

I'd personally be OK with our new&shiny object-oriented code being Lua-5.4 only, but I'm not most Mudlet users (some of whom probably want to use new OO code and their old scripts), so in general I'm -1 on having one of these transitions depend on the other unless absolutely necessary. We'd have to do some actual experiments with sol3-ifying our codebase to determine whether that approach is viable; the sol3 docs state that it should work but I wouldn't bet a whole lot of money on that actually being true if you do somewhat-more-complicated mappings.

@mpconley
Copy link
Contributor

mpconley commented Nov 23, 2020 via email

@smurfix
Copy link
Contributor Author

smurfix commented Nov 23, 2020

@mpconley What do you mean by "accessibility"? As I understand the term it's all about the user interface, while this thread is about overhauling our Lua bindings. Thus in the context of this issue your comment is somewhat misplaced IMHO.

But maybe I just misunderstood what you wrote.

@Kebap
Copy link
Contributor

Kebap commented Nov 23, 2020

Yes, having blind people be able to get the game text read to them. It's currently still not working, but requested by many.
See the Mudlet roadmap discussions: #2854
We are just not too many developers and need to coordinate the efforts for big tasks like this and like that.

@vadi2
Copy link
Member

vadi2 commented Nov 23, 2020

It's about Mudlets accessibility for screen readers, which is very low at the moment since tconsole does not plug into the qt accessibility class. mpconley has done work on this, there's a branch with it available.

@mpconley

@smurfix
Copy link
Contributor Author

smurfix commented Nov 23, 2020

Ah. I am honestly sorry about not having time to work on that in addition to all the other stuff already on my plate.

For reference, the a11y issue is #3342 and far from easy to solve. There's some overlap with this issue in that we need to be able to add some hints to our UI elements: tab order, hotkeys, discoverability of links, selecting popup menu items via the keyboard, etc. etc., which strengthens the idea of using a Geyser-style table-based approach to configuring all of them.

@vadi2
Copy link
Member

vadi2 commented Nov 23, 2020

This won't change our focus. Maybe we can get @smurfix interested in helping with #1283, if no consider it an extra pair (of quite talented) hands to help in another area :)

@smurfix
Copy link
Contributor Author

smurfix commented Nov 26, 2020

I added a list of TODO items to the OP.

Of the class list, Area seems like a good candidate because it has relatively few methods, yet would benefit from cleanup (getAreaTableSwap, getRoomAreaName which returns the ID if you pass a name and vice versa, userdata access via standard Lua table syntax).

@vadi2
Copy link
Member

vadi2 commented Nov 26, 2020

I like the approach you propose and we can defer the hard 5.4 move for later, that is scope creep, I now realise. Let's try this soft approach of replacing APIs. Want to build tests for the things while at it? :)

@smurfix
Copy link
Contributor Author

smurfix commented Nov 26, 2020

I'd love to add tests, the problem is that all of these require some sort of running Mudlet.

Ideally we'd have a test MUD that uploads a test script to Mudlet when you connect to it and then puts the thing through its paces; this in turn would be a whole lot easier if we had a11y. One motivation more for somebody to work on that angle.

I've got another idea how to add tests that actually run inside Mudlet but will have to check whether that's feasible.

@vadi2
Copy link
Member

vadi2 commented Nov 26, 2020

We do have tests running inside Mudlet with Busted. If you make a profile that connects to mudlet.org, it should set them up.

@smurfix
Copy link
Contributor Author

smurfix commented Nov 29, 2020

Which port? 23 doesn't work. (Source code for these tests?)

@vadi2
Copy link
Member

vadi2 commented Dec 1, 2020

Any port. It should install the run-tests.xml package, from there see the Run Tests alias.

We tried, but haven't yet finished setting up CI to run them automatically: https://github.com/Mudlet/Mudlet/tree/add-busted-in-ci

Meanwhile demonnic runs them manually when he reviews PRs.

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

No branches or pull requests

5 participants