#### Brief overview of PR changes/additions
- Variables: the export skipped its refresh whenever the editor's
Variables view was on screen, so anything a script wrote into a saved
variable while it sat open was dropped from the save - including the
session's last save, which is taken with whatever view the editor was
left on. The variables are now read into a throwaway tree, which also
stops a save stranding the editor's variable search results.
- Packages: a save taken while a unit was still executing an item of a
package that had just been uninstalled wrote that package's items back
into the profile, where they returned as orphans the Package Manager
could not remove. The XML writers now skip what the units have queued
for a deferred delete, the module writer included - reloading a module
from a script used to write both the pre- and post-reload copies of its
items into the module file.
- `LuaInterface::getVars()` is now `setjmp`-guarded like every other
Lua-touching method there, so a panic cannot jump past the export's
scope with its variable tree and registry references still held.
#### Motivation for adding to Mudlet
Both are silent data loss in everyday use: quitting with the editor on
the Variables tab, and the `mpkg`/auto-updater shape of uninstalling a
package from a script.
#### Other info (issues closed, discussion etc)
From the 5.0 QA sweep, findings C13 and C14. The variables half re-opens
the loss that `20009c5ec` "fix: variables added while playing are no
longer lost when saving (#9492)" fixed, via the guard it added; the
packages half is the missing counterpart to the self-uninstall deferral
in `276e8bbfd` (#9383) and its follow-ups. #9492's own cases still pass
unchanged.
**Test case:** create a table from the command line, tick it to be saved
in the editor's Variables view, leave the editor there, run `lua
myTable.later = "x"`, quit and reopen - `later` is still there. `ctest
-R 'XMLexportVariablesTest|PackageSelfUninstallTest'` covers both
halves; all 12 new cases were verified to fail against the unfixed
source.
Assisted-by: Claude:claude-opus-5
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Remove unnecessary else/else-if after return, break, continue, and throw
statements - and in places where fixing them is more trouble than its
worth, added NOLINT.
#### Motivation for adding to Mudlet
https://clang.llvm.org/extra/clang-tidy/checks/readability/else-after-return.html,
so it doesn't pop up in PR reviews.
#### Other info (issues closed, discussion etc)
---------
Co-authored-by: Stephen Lyons <slysven@virginmedia.com>
Co-authored-by: Vadim Peretokin <vadi2@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
#### Brief overview of PR changes/additions
- Editing a decimal number in the Variables view no longer converts it
to a string (type detection and Lua round-trips used toInt(), truncating
or reclassifying floats)
- Boolean table keys now display as true/false and are clickable,
instead of appearing as blank unnamed entries
- Guards added so clicking a boolean-keyed entry can no longer rewrite
the key as a string or wipe a table value
#### Motivation for adding to Mudlet
The Variables view silently corrupted float values on save and made
boolean-keyed table entries unusable.
#### Other info (issues closed, discussion etc)
Fixes#6586. Fixes#6585.
**Test case:** Run `lua myFloat = 3.14 b = {[true]="yes", [false]="no",
[3.14]="pi"}`, open the Variables view: b's children should read
3.14/false/true. Edit myFloat to 2.71, click another variable, then `lua
print(type(myFloat))` - should print number.
https://github.com/user-attachments/assets/2ec42f86-ddd6-4657-bae5-ba38d75c97c2
---------
Signed-off-by: Vadim Peretokin <vadim.peretokin@mudlet.org>
Co-authored-by: Vadim Peretokin <vadim.peretokin@mudlet.org>
#### Brief overview of PR changes/additions
Ran clang-format on all 134 CPP files in src/ using the project's
.clang-format config
#### Motivation for adding to Mudlet
Ensures consistent code formatting across the codebase.
#### Other info (issues closed, discussion etc)
None
**Test case:** Build the project and verify it compiles successfully.
#### Brief overview of PR changes/additions
Adds detailed error messages with context to help diagnose issues:
- Process startup failures now show working directory and PATH
- GMCP authentication errors show the package name and malformed data
- Lua variable operations show variable names and type information
- File/directory operations report specific failures
#### Motivation for adding to Mudlet
When something goes wrong, vague error messages make troubleshooting
difficult. These improvements help both users and game admins quickly
identify the root cause of issues.
#### Other info (issues closed, discussion etc)
Test cases:
- Try to start a non-existent process via Lua
- Connect to a game server sending malformed GMCP auth JSON
- Trigger Lua variable rename with unsupported key types (this one would
be hard to do, UI doesnt allow it)
Sample error messages:
```
Failed to start process 'python3': No such file or directory. Working directory: '/home/user/.config/mudlet/profiles/MyGame'. PATH: '/usr/local/bin:/usr/bin:/bin'
GMCP Char.Login.Result - Failed to parse JSON: illegal value at offset 15. Received data: "{invalid: json}"
GMCP Char.Login.Result - Expected JSON object but got null.
LuaInterface::renameCVar() - Unsupported key type: boolean for variable "myVar". Expected string, number, or table.
LuaInterface::loadValue() - Value at stack index 2 is not a table for variable "config". Got type: string.
Host: failed to create error log directory: /home/user/.config/mudlet/profiles/MyGame/log
```
---------
Co-authored-by: Claude <noreply@anthropic.com>
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Ahow error message when variable can't be moved in the trigger editor
(dragged to a non-table, for example)
#### Motivation for adding to Mudlet
Better UX
#### Other info (issues closed, discussion etc)
---------
Co-authored-by: Claude <noreply@anthropic.com>
## Brief overview of PR changes/additions
Comprehensive fix for package installation crashes (issue #8154) that
builds upon the defensive checks from @vadi2's PR #8181.
This PR addresses the root causes of the crash by:
1. **Blocking concurrent operations** - Prevents package installation
while profile save is in progress
2. **Deferring event handlers** - Moves sysInstall event raising to
after package import completes
3. **Improving error recovery** - Better validation in LuaInterface when
searching for Lua tables
## Motivation for adding to Mudlet
Fixes#8154 - crash when installing packages with variables in certain
profiles
The crash occurred because:
- Variables were being loaded into Lua state while event handlers were
executing
- Profile save and package installation could run concurrently
- Event handlers triggered during XMLimport could corrupt Lua stack
state
## Other info
This PR incorporates and builds upon @vadi2's excellent diagnostic work
in PR #8181. The defensive checks from that PR are included, plus
additional fixes for the underlying race conditions.
**Changes in this PR:**
### From vadi2's PR #8181:
- Added Lua stack validation in `LuaInterface::loadValue()` before
calling `lua_gettable()`
- Added null check for Lua state in `callEventHandler()`
- Fixed stack index bug (using `-1` instead of `1` for error checking)
- Added stack cleanup on error paths
- Added emergency stop mode check
### Additional fixes in this PR:
- Added `currentlySavingProfile()` check at the start of
`Host::installPackage()` to prevent concurrent operations
- Deferred `sysInstall`, `sysInstallPackage`, and related event handlers
to execute after package import completes using `QTimer::singleShot()`
- Added validation and error reporting when searching for tables with
`index=0` in `LuaInterface::loadValue()`
Co-authored-by: @vadi2
---------
Co-authored-by: Vadim Peretokin <vadi2@users.noreply.github.com>
Co-authored-by: Vadim Peretokin <vperetokin@hey.com>
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Fix memory leak in variables view
#### Motivation for adding to Mudlet
Should not have any
#### Other info (issues closed, discussion etc)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Move all variable initializers to header classes
#### Motivation for adding to Mudlet
Cleaner code, and a better example for LLMs to follow when generating
code
#### Other info (issues closed, discussion etc)
No functional changed are expected
---------
Co-authored-by: Vadim Peretokin <vadi2@users.noreply.github.com>
Co-authored-by: Kebap <kebap_spam@gmx.net>
#### Brief overview of PR changes/additions
For Windows builds modify the `#include` lines for Lua header files to
specify the 5.1 version. Also accommodate some changes in our CI build
environment:
* For some reason (maybe because of a more modern linker) we need to
specify the original PCRE library with `-lpcre` rather than `-lpcre-1` -
the exact cause of this is not clear but thanks to @jmckisson for
finding it (and using it in his attempt to solve the same problems this
PR is doing).
* It seems the Window building is now being done in the `C:` drive
rather than the previous `D:` one, so a tweak to clean the colon
containing file-system root specifier to the alternative that
MSYS2+Mingw-w64 uses which instead uses a (POSIX) `/` root directory
followed by a single lower-case letter to specify the drive needs to be
extended to handle both drives. This is because the scripts use `rsync`
and that treats any `:` as the separator between host and path and gets
confused when it sees a "Windows" path containing it!
#### Motivation for adding to Mudlet
The default version - and the one needed for some packages like Luarocks
is a 5.4 one - and that includes header files in the "default" `include`
directory. So the headers that get pulled in are the wrong ones, which
fail to work as they are not compatible with Lua 5.1; to get the 5.1
instead I believe we need to explicitly include the version specific
sub-directory in the `#include` lines.
Other tweaks are also needed "to get things working nowadays."
#### Other info (issues closed, discussion etc)
This should be simpler to do than what is being attempted by #7841.
---------
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Mark read-only variables as const - that is, not intended to change
after they've been declared
#### Motivation for adding to Mudlet
So we don't change them by accident later on, and also to clearly state
intentions for the variables
#### Other info (issues closed, discussion etc)
This is also recommended by the [C++ Core
Guidelines](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Improve names of variables that are too short and not meaningful
#### Motivation for adding to Mudlet
So developers have an easier time working with Mudlets code - it's
crucial to be able to attract and retain folks working on the project
for its success!
#### Other info (issues closed, discussion etc)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Improve names of variables that are too short and not meaningful
#### Motivation for adding to Mudlet
So developers have an easier time working with Mudlets code - it's
crucial to be able to attract and retain folks working on the project
for its success!
#### Other info (issues closed, discussion etc)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
If you created a table right at _G with a key that was a number, the
variables view would crash.
#### Motivation for adding to Mudlet
Fix https://github.com/Mudlet/Mudlet/issues/6119
#### Other info (issues closed, discussion etc)
Fixed by adding a case alongside existing support for string keys to
support number keys as well. Function, table, userdata, etc keys are not
supported but they won't crash either.
The `Host*` is never actually used inside the `LuaInterface` class, only in
a non-default constructor. So it does not need to be retained as a class
member.
As the default is never used either we can also mark the above constructor
as `explicit`...
Also `mMaxAreaId` is declared in the `XMLimport` class but although a use
had been considered it is not currently used for anything.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Also removes unused:
* (bool) Host::mAutoReconnect
* (QString) Host::mBufferIncomingData
* (QPushButton*) Host::uninstallButton
* (QListWidget*) Host::packageList
* (QListWidget*) Host::moduleList
* (QPushButton*) Host::moduleUninstallButton
* (QPushButton*) Host::moduleInstallButton
* (bool) TConsole::mWindowIsHidden
Also add some initialisers, particularly for pointers and booleans.
I have spotted in a lot of the new TMxpXxx classes that:
* the include guards are not right at the top of the file
* private members and methods are declared first in the header files rather
than the public ones which are odd as the latter are more likely to be of
interest to anyone wishing to examine the details of the API that the
latter document
* in header files that include another header file the inclusion of that
file is done twice - once before and once after the class members and
methods are documented. Given the use of inclusion guards the second is
pointless - and if there were not such guards the second inclusion would
(I believe) break things...
I have only included fixing this in the TMxpSendTagHandler class in this
PR but there are others that also need the above matters attended to...
For me this reduces the warning count from 662 to 502.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
The latter has been present since Qt 4.7 and even on some Windows OSes
where it may be a 32-Bit value that can overflow after nearly 50 days
when the lower quality TickCounter clock is used as a fallback instead of
the PerformanceCounter one this is a better bet than the QTime based one
that will overflow (wrap) after 24 hours and will be affected by
Summer-Time changes and user/system adjustment of the system clock.
Also:
* rename three elements related to timing the network latency, their
original names were somewhat ambiguous:
* (QLineEdit*) TConsole::networkLatency
==> TConsole::mpLineEdit_networkLatency
* (double) cTelnet::networkLatency
==> cTelnet::networkLatencyTime
* (QTime) cTelnet::networkLatencyTime
==> (QElapsedTimer) cTelnet::networkLatencyTimer
* rename some other elements in a comparable way:
* (QTime) cTelnet::timeOffset
==> (QElapsedTimer) cTelnet::mRecordingChunkTimer
* (QTime) cTelnet::mConnectionTime
==> (QElapsedTimer) cTelnet::mConnectionTimer
* (int) cTelnet::lastTimeOffset
==> (int) cTelnet::mRecordLastChunkMSecTimeOffset
* simplify a C string array access - there is no need to use the address
operator AND an index when referring to the start of a C array, i.e. for
char buffer[datalen]: '&buffer[0]' is simply 'buffer' !
* the display of the network latency (if available) and the system
processing time is a UI feature but it was not being put through the
translation system, this commit now allows for that to happen.
This should remove 12 warnings (on my Linux Qt 5.14.2 system).
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
* lua_error()+return 1 => return lua_error()
* Missed fix-ups from the last cleanup patch
* Flip conditions to remove return-error-else antipatterns
* also fix TForkedProcess
* Remove QString::fromUtf8 calls
They're superfluous and make the code less readable.
Classed as "High Impact":
CID Type Detail
1492499 "Uninitialized scalar variable (UNINIT) "5. uninit_use: Using
uninitialized value error."
1485860 "No virtual destructor" "A1. dtor_in_derived:
Class `XMLimport` has a compiler-generated destructor. It is non-empty
because of its field `mpHost`. A pointer to class `XMLimport` is upcast to
class `QXmlStreamReader` which doesn't have a virtual destructor."
Classed as "Medium Impact":
1492834 "Uninitialized scalar field (UNINIT_CTOR)" "2. uninit_member:
Non-static class member `mIsEndTag` is not initialized in this constructor
nor in any functions that it calls.
4. uninit_member: Non-static class member `mIsEmptyTag` is not initialized
in this constructor nor in any functions that it calls.
6. uninit_member: Non-static class member `mReadingAttrValue` is not
initialized in this constructor nor in any functions that it calls.
8. uninit_member: Non-static class member `mOpeningQuote` is not
initialized in this constructor nor in any functions that it calls."
1488910 "Uninitialized scalar field (UNINIT_CTOR)" "2. uninit_member:
Non-static class member `mPlayerRoomStyle` is not initialized in this
constructor nor in any functions that it calls."
"4. uninit_member: Non-static class member
`mPlayerRoomOuterDiameterPercentage` is not initialized in this
constructor nor in any functions that it calls."
"6. uninit_member: Non-static class member
`mPlayerRoomInnerDiameterPercentage` is not initialized in this
constructor nor in any functions that it calls."
1478854 "Uninitialized pointer field (UNINIT_CTOR)" "4. uninit_member:
Non-static class member `mpOutOfBandDataIncomingCodec` is not initialized
in this constructor nor in any functions that it calls."
1468478 "Unchecked return value (CHECKED_RETURN)" "10. check_return:
Calling `luaL_loadstring` without checking return value (as is done
elsewhere 17 out of 21 times)."
1468477 "Unchecked return value (CHECKED_RETURN)" "14. check_return:
Calling `luaL_loadstring` without checking return value (as is done
elsewhere 17 out of 21 times)."
1468474 "Unchecked return value (CHECKED_RETURN)" "16. check_return:
Calling `luaL_loadstring` without checking return value (as is done
elsewhere 17 out of 21 times)." - x 2
1468468 "Logically dead code (DEADCODE)" "dead_error_line:
Execution cannot reach this statement: `return 1;`"
1415097 "Dereference null return value (NULL_RETURNS)" "8. dereference:
Dereferencing timer, which is known to be `nullptr`"
1415092 "Identical code for different branches (IDENTICAL_BRANCHES)"
"identical_branches: The same code is executed regardless of whether
`areaExit` is true, because the 'then' and 'else' branches are identical.
Should one of the branches be modified, or the entire 'if' statement
replaced?"
1415023 "Dereference null return value (NULL_RETURNS)" "26. dereference:
Dereferencing a pointer that might be `nullptr` `pR->name` when calling
`QString`.
1414989 "Explicit null dereferenced (FORWARD_NULL)" "81. var_deref_op:
Dereferencing null pointer
`this->originalExits.value(dirCode, TExit * const(NULL))`." x 11
1414977 "Logically dead code (DEADCODE)" "dead_error_line:
Execution cannot reach this statement: `return false;`."
Also removed unused:
* (int) cTelnet::curX & curY,
* (double) cTelnet::networkLatencyMin & networkLatencyMax
* (QMutex) TimerUnit::mTimerUnitLock
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This is a squash and merge of five commits:
* Allow setting variables to non-Latin values via GUI
* Allow deleting non-Latin named variables
* Updated rest of LuaInterface to i18n
* Wrap lua_tostring calls in utf8
* Applied review comments
clang-format has an option to sort header #include lines in source code
files - however the `#include "pre_guard.h"` and `#include "post_guard.h"`
includes are position sensitive - they must wrap around Qt's own classes'
includes in our files and they must not be tidied-up/rearranged.
This commit adds the necessary comments that commands clang tools to not
touch these lines.
We had previously done this on the XMLimport class as a test case but
during recent major branch merging operations one of these comments was
lost from the XMLimport.cpp file!
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
* BugFix: fix crash on profile auto-load (bug 1624720)
The uninitialised TVar::parent member in the default constructor is used
in the profile auto-load (but not the normal one apparently) and was the
cause for segment violations. In fixing THIS I noted that several other
members of that Class {and worryingly, quite a few *other* classes
throughout the Mudlet application!!!) were not being initialised to
sensible defaults either. This commit addresses the TVar class, but ONLY
that Class...!
In checking also found that (void) TVar::setNewName( QString ) is not used
{only the two argument (void) TVar::setNewName( QString, int ) that takes
a Lua Type number as a second argument IS used} so commented it out. Also
found that (int) TVar::nvType and (QString) TVar::nValue members are not
used so removed them as well.
Also fixed a (subtle ?) bug in local functor TVarLessThan(TVar *, TVar *) which
tested the number value of the "name" of each argument when converted to a
number without checking that the "name" could be converted to a number.
Consequently, when a non-number name was encountered it would convert to
zero and THAT zero value would be compared to the other one. If BOTH
arguments were not numbers then the caller would ALWAYS be told the first
argument was less than the second argument whereas the wanted result was
a String comparison of "lessnessness".
Refactor TVar class methods that return a "bool" type to actually return
such a thing rather than (int)(0) for false or, more suspiciously, (int)(1)
for true - though subsequent research revealed that that is how C++ does
it, (as a C ex-pat I just think of anything that isn't zero is true)! This
affects:
* (bool) TVar::setValue( const QString )
* (bool) TVar::setValue( const QString, const int )
* (bool) TVar::setValueType( const int )
Similarly, where an argument was an int type but was being used in a
binary/boolean manner I've changed it to the latter type. This affected:
* (QList<TVar *>) TVar::getChildren( const bool )
This affects what are now:
* (void) VarUnit::buildVarTree( QTreeWidgetItem *, TVar *, bool )
* (bool) XMLexport::writeHost( Host * )
* (bool) XMLexport::writeVariable( TVar *, LuaInterface *, VarUnit * )
* (void) dlgTriggerEditor::slot_search_triggers( const QString )
* (int) dlgTriggerEditor::canRecast( QTreeWidgetItem *, int, int )
* (void) dlgTriggerEditor::recurseVariablesDown( TVar *, QList<TVar *> &,
bool)
* (void) LuaInterface::getAllChildren( TVar *, QList<TVar *> *)
Tidies up code layout of TVar implementation to current coding style.
Adds const where possible to TVar class method parameters.
__This commit is a squashed down one as some later ones of the original commits were reverted directly on GitHub in the reverse of their application - this comment contains the text for the remaining parts and hopefully still make sense!__
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This also allows for memory leak checking on MSVC by having include
surrounding Qt headers, which break if you have a macro define new
to be something which tracks line information.
