Improve: Preserve addMapMenu order on new additions - #17
Conversation
|
Reviewed the PR - here are my findings. The main issue is that getMapMenus(true) (the new detailed mode) stores results in a string-keyed Lua table using lua_setfield with uniqueName as the key. Lua hash tables have no guaranteed iteration order when using pairs(), so the insertion order that this PR carefully preserves on the C++ side is lost by the time it reaches Lua. This defeats the purpose of the feature. The fix would be to use an integer-indexed array table (lua_rawseti with an incrementing index) so consumers can iterate with ipairs() and get the correct order. There's also an inconsistency in the parent field between the two modes. The non-detailed (backward compatible) mode maps empty parent to the string "top-level" (line 1893), while the detailed mode returns a raw empty string (line 1875). This will be confusing for API users who switch between modes or reference both. Minor suggestions: the detailed and non-detailed branches in getMapMenus duplicate the loop, contains guard, and variable extraction. These could be a single loop with the if/else branch inside the loop body, saving about 8 lines. Also, there are no existing tests for any of the map menu functions, and this PR doesn't add any. At minimum the new detailed mode return format and the order preservation behavior would benefit from test coverage. The rest of the code looks good - the order tracking in addMapMenu, the cleanup in removeMapMenu, and the defensive contains checks in populateUserContextMenus are all correct. |
|
Thanks for the review! I have made the suggested changes. |
Brief overview of PR changes/additions
Motivation for adding to Mudlet
-Creation order might be more useful to users.
Other info (issues closed, discussion etc)
Attempts to close #6