Enhance: overhaul stopwatches by SlySven · Pull Request #2516 · Mudlet/Mudlet

SlySven · 2019-04-22T21:02:23Z

This PR revamps stopwatches and allows them to be stopped and read at any time afterwards, for any number of times. As such they should behave more closely to a real stopwatch but in a manner that
existing code will not see any differences if it does not do anything that is currently invalid. In addition, this PR:

allows stopwatches to be destroyed - so ID numbers WILL get reused - with Lua ~~destroyStopWatch((int) id)~~ deleteStopWatch((int) id) function. {Edit: renamed...!}
allows them to be marked as persistent so that they are saved with the profile and reloaded again - if they were running then they will continue to increment when the profile is not loaded so they can be used to time real-time events outside of the profile/session; this uses Lua function (bool) setStopWatchPersistence((int) id, (bool) setPersistent).
allows them to be adjusted (even so they become negative) so they can be used to count down as well as count up time - whether the stopwatch concerned is running or not with (bool) adjustStopWatch((int) id, (float) adustmentInSeconds).
have argument type error messages that conform to our current style
have more run-time errors handling - most actions that produce no effect will advise that this is the case - e.g. stopping a stopwatch that was NOT running...
can handle periods of time longer than a day - the previous implementation wrapped around after 24 hours.
should not be affected by DST changes - OS permitting.
the Lua API also gains a getStopWatches() function that returns a table with the id numbers as keys and values as tables of:
- (bool) isRunning
- (bool) isPersistent
- (string) name
- (table) elapsedTime - containing broken down time:
  - (bool) negative
  - (int) days
  - (int) hours (0 to 23)
  - (int) minutes (0 to 59)
  - (int) seconds (0 to 59)
  - (int) milliSeconds (0 to 999)
  - (float) decimalSeconds floating point value of whole of the time in seconds (can be negative!)
allows stop watches to be named - which is useful as then they can be identified in scripts; this means that all the stop watch functions can now take a name string as well as a numeric argument. Using an empty string will access the first (lowest id) stopwatch that does not have a name and the createStopWatch() function will accept an optional string argument as a name. For simplicity each name must be unique and this is enforced for that function and the added setStopWatchName(id or name, newName) function. The latter will also accept an empty string as either the first or second argument; in the first case it will assign the name to the first unnamed stopwatch and the second will clear the name of the specified one.
adds getStopWatchBrokenDownTime() which returns the same broken down elements in a table for a single specified timer (day count; hours; minutes; seconds; milliseconds and whether the time is positive or negative {when preset with a negative adjustment and used as a count down})...

During debugging I found out that the process of loading an existing profile that contained createStopWatch() calls was creating them during the testing/loading phase so I had to add extra code to prevent that function from taking effect whilst (bool) Host::mIsProfileLoadingSequence is true. Then, when testing the resetProfile() function I found that the same thing was happening AND that the non-persistent stopwatches needed to be removed as well - which is now solved by also preventing stopwatch creation whilst (bool) Host::mResetProfile is set and by running a new method (void) Host::removeAllNonPersistentStopWatches()!

Signed-off-by: Stephen Lyons slysven@virginmedia.com

This PR allows them to be stopped and read at any time afterwards, for any number of times. It also: * allows stopwatches to be destroyed - so ID numbers WILL get reused - with Lua `(true) destroyStopWatch((int) id)` function. * allows them to be marked as persistent so that they are saved with the profile and reloaded again - if they were running then they will continue to increment when the profile is not loaded so they can be used to real time events outside of the profile/session; this uses Lua function `(bool) setStopWatchPersistence((int) id, (bool) setPersistent)`. * allows them to be adjusted (even so they become negative) so can be used to count down as well as count up time - whether the stopwatch concerned is running or not. * have error messages that conform to our current style * have more run-time handling - most actions that produce no effect will advise that this is the case - e.g. stopping a stopwatch that was NOT running... * can handle periods of time longer than a day - the previous implementation wrapped around after 24 hours. * should not affected by DST changes - OS permitting. * the Lua API also gains a `getStopWatches()` function that returns a table with the id numbers as keys and values as tables of: * `(bool) isRunning` * `(bool) isPersistent` * `(string) name` * `(table) elapsedTime` - containing broken down time: * `(bool) negative` * `(int) days` * `(int) hours` (0 to 23) * `(int) minutes` (0 to 59) * `(int) seconds` (0 to 59) * `(int) milliSeconds` (0 to 999) * `(float) decimalSeconds` floating point value of whole of the time in seconds (can be negative!) * allows stop watches to be named - which is useful as then they can be identified in scripts; this means that all the stop watch functions can now take a name string as well as a numeric argument. Using an empty string will access the first (lowest id) stopwatch that does not have a name and the `createStopWatch()` function will accept an optional string argument as a name. For simplicity each name must be unique and this is enforced for that function and the added `setStopWatchName(id or name, newName)` function. The latter will also accept an empty string as either the first or second argument; in the first case it will assign the name to the first unnamed stopwatch and the second will clear the name of the specified one. * added `getStopWatchBrokenDownTime(...)` which returns the same broken down elements in a table for a single specified timer (day count; hours; minutes; seconds; milliseconds and whether the time is positive or negative {when preset with a negative adjustment and used as a count down})... During debugging I found out that the process of loading an existing profile that contained `createStopWatch()` calls was creating them during the testing/loading phase so I had to add extra code to prevent that function from taking effect whilst `(bool) Host::mIsProfileLoadingSequence` is true. Then, when testing the `resetProfile()` function I found that the same thing was happening AND that the non-persistent stopwatches needed to be removed as well - which is now solved by also preventing stopwatch creation whilst `(bool) Host::mResetProfile` is set and by running a new method `(void) Host::removeAllNonPersistentStopWatches()`! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

add-deployment-links · 2019-04-22T21:02:29Z

Hey there! Thanks for helping Mudlet improve. 🌟

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

It seems that later versions are happy to compare a `QString` with a `QLatin1Char` but that version isn't... Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 · 2019-04-23T13:32:34Z

Really cool! Could you also add examples of how you see this new framework being used?

I've started reviewing, but it'll take a while to digest all the improvements.

Jieiku · 2019-04-26T21:06:52Z

I have been using getEpoch() and storing the value and then to check elapsed time I would call getEpoch() again and compare the difference. I do this for a couple of my features. Curious to check this out and see what it brings to the table. I can see that they add the ability to stop/pause the timer which could be very useful!

bduza · 2019-04-27T11:10:14Z

Gave it a whirl tonight. It's easier to use than the old version. The string name is really nice - that part alone makes it worthwhile in my opinion. I don't think I made any syntactical mistakes playing with this either, so that says something for it being intuitive at least.

If I want to throw a quick stopwatch in to time my script, it's as simple as:

createStopWatch('scripttimer')
startStopWatch('scripttimer')
--script here
stopStopWatch('scripttimer')

Whereas before I had to manage the ID (including preventing duplicates), save the result to a variable, and print that variable's value. Quite streamlined in comparison. The breakdown of time into a table with the various units is also nice.

I do wonder whether timers and stopwatches are two sides of the same coin, and could/should have been combined from the beginning. That ship has well and truly sailed though.

src/Host.cpp

SlySven

Bother - ~~spotted a bug.~~ fixed.

`(bool) Host::stopWatch()` was missing a final `return` value. The code in `(void) stopWatch::Host::adjustMilliSeconds(const qint64)` used `(QDateTime) QDateTime::addMSecs(qint64)` incorrectly in that I had thought it adjusted the QDateTime it was called upon whereas it returns a reference to the adjusted value - so needs to be invoked in a different manner. `QString stopWatch::getElapsedDayTimeString() const` made use of `int` for some intermediate local variables but on Windows platforms the int type may be 32-bit long and thus not big enough to contain 64-bit values - and some other locals can be much shorter because of the limits that the code places on their values but will give compiler warnings without static_cast <T>. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

The previous code that I wrote to check for a duplicate proposed name for a stopwatch and also determine the lowest unused positive integer to use for the id in a single pass was actually defective (and hard to comprehend after a short time away from it). So I have retreated to some- thing that is less efficient but is simpler and more readily understandable. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

I don't think wanting to do something in the future is related to measuring the time that has passed. Yes, you might want to measure how much time has passed in relation to the thing that'll happen in the future, but each action is also truly independent in its own right.

@SlySven I don't have time to extensively test this but others have - my only comment would be to use killStopWatch to be consistent with other kill* functions we've got. Let's get this merged in 3.22!

Due to a copy-pasta error `(bool) Host::initialised()` was returning the value of the wrong member! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap · 2019-07-15T22:09:24Z

The wiki currently lists these as available since Mudlet 3.20 or 3.21 respectively and needs to be revisited upon actual release at latest, better now and then again:

SlySven · 2019-08-21T01:26:23Z

I've been running a quick analysis of the core (C++ provided) Lua API functions and I can see that:

killXxxx functions are only used for the Mudlet Alias/Key/Timer/Trigger items - which are made from corresponding permXxxx or termXxxx functions - it does not have an obvious English antonym.
addXxxx is used for eight things and five of those have a matching removeXxxx function.
openXxxx is used for three things and only one of those has a matching closeXxxx (there are two of those latter type functions in total).
createXxxx is used for nine functions - though admittedly there is only one using the obvious English antonym of destroyXxxx and that is the one I am adding herein; given that there is a deleteMapLabel that undoes the effects of both createMapLabel and presumably createMapImageLabel I suppose I could be persuaded that deleteStopWatch is a plausible alternative. There are three other deleteXxxx function: xxxArea, xxxRoom and xxxLine.

In summery I do not feel that kill is linguistically the best fit here - thus I hope to see more ~~destruction~~ uses of destroyXxxx in the future but I would be willing to use deleteStopWatch as a compromise...

SlySven · 2019-09-22T23:32:03Z

... but it is refferenced as destroy in numerous locations including the lua example

Okay - yeah it was originally called destroyStopWatch but got renamed to deleteStopWatch - I'll just go and fix that in the Wiki - should be done now.

demonnic · 2019-09-23T00:29:29Z

Thoughts on also providing a geyser like interface? If so, should it be in Geyser itself?

Let me rephrase. I have already written a Geyser like interface to all this new stopwatch stuff. Should we include it in Mudlet as Geyser.Stopwatch, some other name, or should I maybe release it as its own thing after the version of Mudlet this is in drops?

new/modified stopwatch functionality seems to work from my testing both with and without the 'class' wrapping.

SlySven · 2019-09-23T00:55:23Z

do not see documentation for adjustStopWatch()

~~Just writing it~~ - it slipped my attention. Now done.

Oh bother - I just realised that in resolving the merge conflict an hour ago I cause some private Host method declarations to get duplicated which of course breaks things so the CIs all crashed and burned... 😞 ... so I am going to have to revisit it.

As to incorporating it into Geyser I really do not know what the merits of each of the alternatives that you mention are. What are the pro/cons of them?

I accidentally ended up with some duplicate methods listed in the Host header file. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

demonnic · 2019-09-23T01:08:56Z

Really this becomes two questions, neither of which necessarily blocks the merging of this PR:
The first question is: do we want to include an 'object oriented' interface to the stopwatches with Mudlet?

If so, Geyser is the main example we have of interacting with base Mudlet objects in an object oriented way, I feel like there is benefit to continuing to use that name, though it may dilute the original purpose, which is a layout manager.

I haven't really thought of any better name for it if we don't want to make it a part of Geyser, but we should consider perhaps some other namespace for utility 'objects' in that case.

If we don't want to include it with Mudlet, then I'll just package it up and toss it on the forums and figure out something else to call it besides Geyser, or just make it demonnic.Stopwatch or some such. So it's not like whatever functionality it adds (mostly an 'object' like abstraction, similar to Geyser's, and a formatted getTime() function) won't be available to the community.

keneanung · 2019-09-27T09:39:21Z

src/TLuaInterpreter.cpp

+    if (lua_type(L, 1) == LUA_TNUMBER) {
+        // Flag (if true) to replicate previous (reset and start again from zero
+        // if call is repeated without any other actions being carried out on
+        // stopwatch) behaviour if only a single NUMBERIC argument (ID) supplied:


Typo: NUMBERIC

Fixed in next commit.

keneanung · 2019-09-27T09:46:19Z

src/TLuaInterpreter.cpp

+        return lua_error(L);
+    }
+
+    qint64 watchId = 0;


In the functions above, this is a simple int. Why the difference?

lua_tointeger(...) returns a lua_Integer which, according to the documentation: "By default it is a ptrdiff_t, which is usually the largest signed integral type the machine handles 'comfortably'". ~~On Windows (like other platforms) this is a 64-bit value but ints are only 32 bits on that platform and it thus provokes a warning - and this is one way to remove that.~~

Edit: actually it applies on other platforms - on my 64-bit Linux by reverting one place and enabling the Clang-code model backend (I normally disable it because the whole things is a resource sink - older versions used huge proportions of memory - newer ones - e.g. Qt Creator 4.10.0 - just tied up half the processor cores for minutes at a time):

On Windows long is the same size as int (32-bits) but pdiff_t is still going to be 64-bits and provoke this sort of warning on most OS AFAICT...

This is once more a question of consistency. Why do other functions work with normal ints then? I am not questioning the correctness since I have no experience. But looking at the code I see two versions and wonder "why?"

If you have the clang-code-model plugin enabled then the TLuaInterpreter.cpp gets lit up like a Christmas Tree! This is one source of it being irritated with our code and I was trying to not add to the bonfire. As it happens it is also silenced by the quick and dirty use of static_cast<int>(...) around the lua_tointeger(...) calls - which is the approach I have taken with a fresh commit in conjunction with switching back to int. After all, we are not likely to have the user requesting 2^32 stopwatches - or if we do - they deserve whatever nasal daemons they get.

keneanung · 2019-09-27T09:51:03Z

src/TLuaInterpreter.cpp

+        return lua_error(L);
+    }
+
+    qint64 watchId = 0;


Another qint64

For the same reason as above.

keneanung · 2019-09-27T09:52:21Z

src/TLuaInterpreter.cpp

+    }
+
+    if (!lua_isboolean(L, 2)) {
+        lua_pushfstring(L, "setStopWatchPersistence: bad argument #2 type (persistent {save between sessions} as boolean expected, got %s!)", luaL_typename(L, 2));


I'd just use persistence here and remove the curly braces. The user of the function should know what you mean when they use the function. It should, of course, be explained in the documentation.

Fair enough - I am gradually being weaned off being too helpful in Lua error messages... 😜

keneanung · 2019-09-27T09:59:05Z

src/TLuaInterpreter.cpp

+        return lua_error(L);
+    }
+
+    qint64 watchId = 0;


More qint64

For the same reason as above.

keneanung · 2019-09-27T11:10:56Z

src/Host.cpp

 }

-bool Host::resetStopWatch(int watchID)
+bool Host::makeStopWatchPersistent(const int id, const bool state)


Why is there no name overload of this function?

Because looking up the name is done in the TLuaInterpreter code and that handles producing the not found message which is the only failure mechanism (so no need to have to pass back an error message)...

keneanung · 2019-09-27T11:13:52Z

src/Host.cpp

 }

-double Host::stopStopWatch(int watchID)
+QPair<bool, QString> Host::getBrokenDownStopWatchTime(const int id) const


Why is there no name overload of this function?

keneanung · 2019-09-27T11:14:08Z

src/Host.cpp

 }

-bool Host::startStopWatch(int watchID)
+bool Host::adjustStopWatch(const int id, const qint64 milliSeconds)


Why is there no name overload of this function?

keneanung · 2019-09-27T11:14:20Z

src/Host.cpp

 }

-double Host::getStopWatchTime(int watchID)
+bool Host::destroyStopWatch(const int id)


Why is there no name overload of this function?

As I mentioned for another function - the conversion of name to Id is handled in the TLuaInterpreter class which already handles the only failure mechanism of there not being a stopwatch with the given name.

Then why are there name overloads for some functions? I'm just confused by the inconsistency.

It is because of the two forms of identification - by name and by ID can fail in slightly different ways and have different error message forms - for example consider the command to reset the stopwatch, the ID based function has a pair of messages:

QStringLiteral("stopwatch with id %1 was already reset").arg(id))

QStringLiteral("stopwatch with id %1 not found").arg(id))

whereas the name one has four, two that mention the name and another pair for the empty string (which works on the lowest ID unnamed stopwatch) case - some of which also mention the ID:

QLatin1String("no unnamed stopwatches found"))

QStringLiteral("stopwatch with name \"%1\" not found").arg(name))

QStringLiteral("the first unnamed stopwatch (id:%1) was already reset").arg(watchId));

QStringLiteral("stopwatch with name \"%1\" (id:%2) was already reset").arg(name, QString::number(watchId)))

keneanung · 2019-09-27T11:14:38Z

src/Host.cpp

+    return 0;
+}
+
+QPair<bool, double> Host::getStopWatchTime(const int id) const


Why is there no name overload of this function?

demonnic

From a Lua API perspective this seems to work as advertised. Approving based on that, not really reviewing the c++ code

demonnic · 2019-09-29T16:14:38Z

I would like to add though that we should try to keep setting a 'name' in the createStopWatch and being able to retroactively assign a name to a stopwatch as an exception for temporary items. IT makes sense here because we are adding persistence to what used to be a temp-only item. I do not think it would be a good idea to go back and add such capabilities to other temp* items, as it doesn't solve a problem for those, just moves the tracking from a numeric ID to a string ID. We have perm* versions of the temp* items which can be named already to provide the same functionality as the stopwatch naming adds here.

Revert to using `int` for the stopWatch ID number and instead use a `static_cast<int>(...)` to silence the warnings about converting the `long` return value of `lua_tointeger(...)` to an `int` type. Fix a couple of typos in comments. Remove extra detail from an error message. Use std::chrono_literals to specify some needed time intervals rather than large `long` integer literal constants. Refactor a block of code used to generate the broken down time as a Lua table so that two instances are handled by a single helper function. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Conflicts resolved in: * src/Host.h Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Needed to pull in the update to the CI system to fix build that makes AppImage or rather the .dmg equivalent for macOs platform which was failing after a recent Apple update to the OS. Signed-off-by: root <slysven@virginmedia.com>

SlySven · 2019-10-06T15:07:01Z

📖 The Wiki has been updated to flag the changes that are taking place for the 4.2.0 release which will be the first to include this functionality...

vadi2 · 2019-10-13T18:42:36Z

resetStopWatch(nil) was valid before and now it's an error, this causes scripts to fail and people to complain about the 3rd update in a row breaking things. 😕

This reverts commit e7af530. # Conflicts: # src/TLuaInterpreter.cpp

This commit is a rework of the original PR Mudlet#2516 squash-and-merged with the bugfix that I proposed as PR Mudlet#3162 but which became difficult to apply when the original PR was eliminated from the development branch because it has been reverted from the 4.2.0 release and then that had been merged into the development branch! This PR allows stopwatches to be stopped and read at any time afterwards, for any number of times. It also: * allows stopwatches to be destroyed - so ID numbers WILL get reused - with a Lua (bool) destroyStopWatch((int) id) function. * allows them to be marked as persistent so that they are saved with the profile and reloaded again - if they were running then they will continue to increment when the profile is not loaded so they can be used to real time events outside of the profile/session; this uses a new Lua function (bool) setStopWatchPersistence((int) id, (bool) setPersistent). * allows them to be adjusted (even so they become negative) so can be used to count down as well as count up time - whether the stopwatch concerned is running or not. * have error messages that conform to our current style * have more run-time handling - most actions that produce no effect will advise that this is the case - e.g. stopping a stopwatch that was NOT running... * can handle periods of time longer than a day - the previous implementation wrapped around after 24 hours. * should not affected by DST changes - OS permitting. * the Lua API also gains a getStopWatches() function that returns a table with the id numbers as keys and values as tables of: * (bool) isRunning * (bool) isPersistent * (string) name * (table) elapsedTime - containing broken down time: * (bool) negative * (int) days * (int) hours (0 to 23) * (int) minutes (0 to 59) * (int) seconds (0 to 59) * (int) milliSeconds (0 to 999) * (float) decimalSeconds floating point value of whole of the time in seconds (can be negative!) * allows stop watches to be named - which is useful as then they can be identified in scripts; this means that all the stop watch functions can now take a name string as well as a numeric argument. Using an empty string will access the first (lowest id) stopwatch that does not have a name and the createStopWatch() function will accept an optional string argument as a name. For simplicity each name must be unique and this is enforced for that function and the added setStopWatchName(id or name, newName) function. The latter will also accept an empty string as either the first or second argument; in the first case it will assign the name to the first unnamed stopwatch and the second will clear the name of the specified one. * added getStopWatchBrokenDownTime(...) which returns the same broken down elements in a table for a single specified timer (day count; hours; minutes; seconds; milliseconds and whether the time is positive or negative {when preset with a negative adjustment and used as a count down})... During debugging I found out that the process of loading an existing profile that contained createStopWatch() calls was creating them during the testing/loading phase so I had to add extra code to prevent that function from taking effect whilst (bool) ~~`Host::mIsProfileLoadingSequence`~~ *revised to use a different, new, flag: `Host::mBlockStopWatchCreation` which is cleared earlier in the loading sequence* is true. Then, when testing the resetProfile() function I found that the same thing was happening AND that the non-persistent stopwatches needed to be removed as well - which is now solved by also preventing stopwatch creation whilst (bool) Host::mResetProfile is set and by running a new method (void) Host::removeAllNonPersistentStopWatches()! This thus allows stopwatches to be created during the profile startup sequence when lua scripts are run on loading (but after a prior compilation to test for script validity has been done). The code in (void) stopWatch::Host::adjustMilliSeconds(const qint64) originally used (QDateTime) QDateTime::addMSecs(qint64) incorrectly in that I had thought it adjusted the QDateTime it was called upon whereas it returns a reference to the adjusted value - so needed to be invoked in a different manner which was already being done in the original PR. QString stopWatch::getElapsedDayTimeString() const made use of int for some intermediate local variables but on Windows platforms the int type may be 32-bit long and thus not big enough to contain 64-bit values - and some other locals can be much shorter because of the limits that the code places on their values but will give compiler warnings without static_cast <T>. Use std::chrono_literals to specify some needed time intervals rather than large long integer literal constants. Refactor a block of code used to generate the broken down time as a Lua table so that two instances are handled by a single helper function. Additional code (the bug fix) has been included to provide backwards-compatibility for Lua startStopWatch(id) function which recreates prior start stop-watch behaviour when it is created: The prior form of startStopWatch(...) would reset and restart the indicated stopwatch each time that it was called. This is not really compatible with the revised functionality which allows the recorded time to be adjusted even before the stopwatch is first used. To allow existing scripts to continue to experience the same API this commit adds an optional second boolean argument to the startStopWatch(...) call ONLY WHEN the first argument is an id number and NOT a string name. If the second argument is omitted (as it will be when using older scripts) or is true then each time the function is used the stopwatch will be reset and restarted. Just in case the same behaviour ***is*** wanted with stop watch created with a **name** then a second boolean `true` will start the stopwatch from zero. This PR replaces (and thus will) close Mudlet#3162 . Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Redo_Enhance: overhaul stopwatches This PR is a rework of the original PR #2516 squash-and-merged with the bugfix that I proposed as PR #3162 but which became difficult to apply when the original PR was eliminated from the development branch because it has been reverted from the 4.2.0 release and then that had been merged into the development branch! This PR allows stopwatches to be stopped and read at any time afterwards, for any number of times. It also: * allows stopwatches to be destroyed - so ID numbers WILL get reused - with a Lua (bool) destroyStopWatch((int) id) function. * allows them to be marked as persistent so that they are saved with the profile and reloaded again - if they were running then they will continue to increment when the profile is not loaded so they can be used to real time events outside of the profile/session; this uses a new Lua function (bool) setStopWatchPersistence((int) id, (bool) setPersistent). * allows them to be adjusted (even so they become negative) so can be used to count down as well as count up time - whether the stopwatch concerned is running or not. * have error messages that conform to our current style * have more run-time handling - most actions that produce no effect will advise that this is the case - e.g. stopping a stopwatch that was NOT running... * can handle periods of time longer than a day - the previous implementation wrapped around after 24 hours. * should not affected by DST changes - OS permitting. * the Lua API also gains a getStopWatches() function that returns a table with the id numbers as keys and values as tables of: * (bool) isRunning * (bool) isPersistent * (string) name * (table) elapsedTime - containing broken down time: * (bool) negative * (int) days * (int) hours (0 to 23) * (int) minutes (0 to 59) * (int) seconds (0 to 59) * (int) milliSeconds (0 to 999) * (float) decimalSeconds floating point value of whole of the time in seconds (can be negative!) * allows stop watches to be named - which is useful as then they can be identified in scripts; this means that all the stop watch functions can now take a name string as well as a numeric argument. Using an empty string will access the first (lowest id) stopwatch that does not have a name and the createStopWatch() function will accept an optional string argument as a name. For simplicity each name must be unique and this is enforced for that function and the added setStopWatchName(id or name, newName) function. The latter will also accept an empty string as either the first or second argument; in the first case it will assign the name to the first unnamed stopwatch and the second will clear the name of the specified one. * added getStopWatchBrokenDownTime(...) which returns the same broken down elements in a table for a single specified timer (day count; hours; minutes; seconds; milliseconds and whether the time is positive or negative {when preset with a negative adjustment and used as a count down})... During debugging I found out that the process of loading an existing profile that contained createStopWatch() calls was creating them during the testing/loading phase so I had to add extra code to prevent that function from taking effect whilst (bool) ~~`Host::mIsProfileLoadingSequence`~~ *revised to use a different, new, flag: `Host::mBlockStopWatchCreation` which is cleared earlier in the loading sequence* is true. Then, when testing the resetProfile() function I found that the same thing was happening AND that the non-persistent stopwatches needed to be removed as well - which is now solved by also preventing stopwatch creation whilst (bool) Host::mResetProfile is set and by running a new method (void) Host::removeAllNonPersistentStopWatches()! This thus allows stopwatches to be created during the profile startup sequence when lua scripts are run on loading (but after a prior compilation to test for script validity has been done). The code in (void) stopWatch::Host::adjustMilliSeconds(const qint64) originally used (QDateTime) QDateTime::addMSecs(qint64) incorrectly in that I had thought it adjusted the QDateTime it was called upon whereas it returns a reference to the adjusted value - so needed to be invoked in a different manner which was already being done in the original PR. QString stopWatch::getElapsedDayTimeString() const made use of int for some intermediate local variables but on Windows platforms the int type may be 32-bit long and thus not big enough to contain 64-bit values - and some other locals can be much shorter because of the limits that the code places on their values but will give compiler warnings without static_cast <T>. Use std::chrono_literals to specify some needed time intervals rather than large long integer literal constants. Refactor a block of code used to generate the broken down time as a Lua table so that two instances are handled by a single helper function. Additional code (the bug fix) has been included to provide backwards-compatibility for Lua startStopWatch(id) function which recreates prior start stop-watch behaviour when it is created: The prior form of startStopWatch(...) would reset and restart the indicated stopwatch each time that it was called. This is not really compatible with the revised functionality which allows the recorded time to be adjusted even before the stopwatch is first used. To allow existing scripts to continue to experience the same API this commit adds an optional second boolean argument to the startStopWatch(...) call ONLY WHEN the first argument is an id number and NOT a string name. If the second argument is omitted (as it will be when using older scripts) or is true then each time the function is used the stopwatch will be reset and restarted. Just in case the same behaviour ***is*** wanted with stop watch created with a **name** then a second boolean `true` will start the stopwatch from zero. This PR replaces (and thus will) close #3162 . A separate commit was appended and squashed in to reduce a code duplication pointed out by CodeFactor: Created a single function that is used to do the same thing in four separate stop-watch functions. Signed-off by: Stephen Lyons <slysven@virginmedia.com

This commit is a squash-and-merged of a number of commits with the following edited summary of individual messages: This PR allows stopwatches to be stopped and read at any time afterwards, for any number of times. It also: * allows stopwatches to be destroyed - so ID numbers WILL get reused - with a Lua (bool) destroyStopWatch((int) id) function. * allows them to be marked as persistent so that they are saved with the profile and reloaded again - if they were running then they will continue to increment when the profile is not loaded so they can be used to real time events outside of the profile/session; this uses a new Lua function (bool) setStopWatchPersistence((int) id, (bool) setPersistent). * allows them to be adjusted (even so they become negative) so can be used to count down as well as count up time - whether the stopwatch concerned is running or not. * have error messages that conform to our current style * have more run-time handling - most actions that produce no effect will advise that this is the case - e.g. stopping a stopwatch that was NOT running... * can handle periods of time longer than a day - the previous implementation wrapped around after 24 hours. * should not affected by DST changes - OS permitting. * the Lua API also gains a getStopWatches() function that returns a table with the id numbers as keys and values as tables of: * (bool) isRunning * (bool) isPersistent * (string) name * (table) elapsedTime - containing broken down time: * (bool) negative * (int) days * (int) hours (0 to 23) * (int) minutes (0 to 59) * (int) seconds (0 to 59) * (int) milliSeconds (0 to 999) * (float) decimalSeconds floating point value of whole of the time in seconds (can be negative!) * allows stop watches to be named - which is useful as then they can be identified in scripts; this means that all the stop watch functions can now take a name string as well as a numeric argument. Using an empty string will access the first (lowest id) stopwatch that does not have a name and the createStopWatch() function will accept an optional string argument as a name. For simplicity each name must be unique and this is enforced for that function and the added setStopWatchName(id or name, newName) function. The latter will also accept an empty string as either the first or second argument; in the first case it will assign the name to the first unnamed stopwatch and the second will clear the name of the specified one. * added getStopWatchBrokenDownTime(...) which returns the same broken down elements in a table for a single specified timer (day count; hours; minutes; seconds; milliseconds and whether the time is positive or negative {when preset with a negative adjustment and used as a count down})... During debugging I found out that the process of loading an existing profile that contained createStopWatch() calls was creating them during the testing/loading phase so I had to add extra code to prevent that function from taking effect whilst (bool) Host::mIsProfileLoadingSequence is true. Then, when testing the resetProfile() function I found that the same thing was happening AND that the non-persistent stopwatches needed to be removed as well - which is now solved by also preventing stopwatch creation whilst (bool) Host::mResetProfile is set and by running a new method (void) Host::removeAllNonPersistentStopWatches()! It seems that Qt versions later than 5.7 (which was still usable when this PR was started are happy to compare a QString with a QLatin1Char but that version wasn't... The code in (void) stopWatch::Host::adjustMilliSeconds(const qint64) originally used (QDateTime) QDateTime::addMSecs(qint64) incorrectly in that I had thought it adjusted the QDateTime it was called upon whereas it returns a reference to the adjusted value - so needs to be invoked in a different manner. QString stopWatch::getElapsedDayTimeString() const made use of int for some intermediate local variables but on Windows platforms the int type may be 32-bit long and thus not big enough to contain 64-bit values - and some other locals can be much shorter because of the limits that the code places on their values but will give compiler warnings without static_cast <T>. Revise: provide backwards-compatibility for Lua startStopWatch(id) function The prior form of startStopWatch(...) would reset and restart the indicated stopwatch each time that it was called. This is not really compatible with the revised functionality which allows the recorded time to be adjusted even before the stopwatch is first used. To allow existing scripts to continue to experience the same API this commit adds an optional second boolean argument to the startStopWatch(...) call ONLY WHEN the first argument is an id number and NOT a string name. If the second argument is omitted (as it will be when using older scripts) or is true then each time the function is used the stopwatch will be reset and restarted. If the second argument is false then the new behaviour of starting the stopwatch only if it is not already running and continuing from whatever the cumulative past time is on the stopwatch already is carried out - as it will be if the first argument is a name. Also remove some extraneous semi-colons I had accidentally put in in the past...! Revised changes things after peer review: Revert to using int for the stopWatch ID number and instead use a static_cast<int>(...) to silence the warnings about converting the long return value of lua_tointeger(...) to an int type. Fix a couple of typos in comments. Remove extra detail from an error message. Use std::chrono_literals to specify some needed time intervals rather than large long integer literal constants. Refactor a block of code used to generate the broken down time as a Lua table so that two instances are handled by a single helper function. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

This reverts commit e7af530. # Conflicts: # src/TLuaInterpreter.cpp

Redo_Enhance: overhaul stopwatches This PR is a rework of the original PR Mudlet#2516 squash-and-merged with the bugfix that I proposed as PR Mudlet#3162 but which became difficult to apply when the original PR was eliminated from the development branch because it has been reverted from the 4.2.0 release and then that had been merged into the development branch! This PR allows stopwatches to be stopped and read at any time afterwards, for any number of times. It also: * allows stopwatches to be destroyed - so ID numbers WILL get reused - with a Lua (bool) destroyStopWatch((int) id) function. * allows them to be marked as persistent so that they are saved with the profile and reloaded again - if they were running then they will continue to increment when the profile is not loaded so they can be used to real time events outside of the profile/session; this uses a new Lua function (bool) setStopWatchPersistence((int) id, (bool) setPersistent). * allows them to be adjusted (even so they become negative) so can be used to count down as well as count up time - whether the stopwatch concerned is running or not. * have error messages that conform to our current style * have more run-time handling - most actions that produce no effect will advise that this is the case - e.g. stopping a stopwatch that was NOT running... * can handle periods of time longer than a day - the previous implementation wrapped around after 24 hours. * should not affected by DST changes - OS permitting. * the Lua API also gains a getStopWatches() function that returns a table with the id numbers as keys and values as tables of: * (bool) isRunning * (bool) isPersistent * (string) name * (table) elapsedTime - containing broken down time: * (bool) negative * (int) days * (int) hours (0 to 23) * (int) minutes (0 to 59) * (int) seconds (0 to 59) * (int) milliSeconds (0 to 999) * (float) decimalSeconds floating point value of whole of the time in seconds (can be negative!) * allows stop watches to be named - which is useful as then they can be identified in scripts; this means that all the stop watch functions can now take a name string as well as a numeric argument. Using an empty string will access the first (lowest id) stopwatch that does not have a name and the createStopWatch() function will accept an optional string argument as a name. For simplicity each name must be unique and this is enforced for that function and the added setStopWatchName(id or name, newName) function. The latter will also accept an empty string as either the first or second argument; in the first case it will assign the name to the first unnamed stopwatch and the second will clear the name of the specified one. * added getStopWatchBrokenDownTime(...) which returns the same broken down elements in a table for a single specified timer (day count; hours; minutes; seconds; milliseconds and whether the time is positive or negative {when preset with a negative adjustment and used as a count down})... During debugging I found out that the process of loading an existing profile that contained createStopWatch() calls was creating them during the testing/loading phase so I had to add extra code to prevent that function from taking effect whilst (bool) ~~`Host::mIsProfileLoadingSequence`~~ *revised to use a different, new, flag: `Host::mBlockStopWatchCreation` which is cleared earlier in the loading sequence* is true. Then, when testing the resetProfile() function I found that the same thing was happening AND that the non-persistent stopwatches needed to be removed as well - which is now solved by also preventing stopwatch creation whilst (bool) Host::mResetProfile is set and by running a new method (void) Host::removeAllNonPersistentStopWatches()! This thus allows stopwatches to be created during the profile startup sequence when lua scripts are run on loading (but after a prior compilation to test for script validity has been done). The code in (void) stopWatch::Host::adjustMilliSeconds(const qint64) originally used (QDateTime) QDateTime::addMSecs(qint64) incorrectly in that I had thought it adjusted the QDateTime it was called upon whereas it returns a reference to the adjusted value - so needed to be invoked in a different manner which was already being done in the original PR. QString stopWatch::getElapsedDayTimeString() const made use of int for some intermediate local variables but on Windows platforms the int type may be 32-bit long and thus not big enough to contain 64-bit values - and some other locals can be much shorter because of the limits that the code places on their values but will give compiler warnings without static_cast <T>. Use std::chrono_literals to specify some needed time intervals rather than large long integer literal constants. Refactor a block of code used to generate the broken down time as a Lua table so that two instances are handled by a single helper function. Additional code (the bug fix) has been included to provide backwards-compatibility for Lua startStopWatch(id) function which recreates prior start stop-watch behaviour when it is created: The prior form of startStopWatch(...) would reset and restart the indicated stopwatch each time that it was called. This is not really compatible with the revised functionality which allows the recorded time to be adjusted even before the stopwatch is first used. To allow existing scripts to continue to experience the same API this commit adds an optional second boolean argument to the startStopWatch(...) call ONLY WHEN the first argument is an id number and NOT a string name. If the second argument is omitted (as it will be when using older scripts) or is true then each time the function is used the stopwatch will be reset and restarted. Just in case the same behaviour ***is*** wanted with stop watch created with a **name** then a second boolean `true` will start the stopwatch from zero. This PR replaces (and thus will) close Mudlet#3162 . A separate commit was appended and squashed in to reduce a code duplication pointed out by CodeFactor: Created a single function that is used to do the same thing in four separate stop-watch functions. Signed-off by: Stephen Lyons <slysven@virginmedia.com

SlySven added the 1 - Ready for review label Apr 22, 2019

SlySven added this to the 3.20.0 milestone Apr 22, 2019

SlySven requested review from a team April 22, 2019 21:02

SlySven assigned vadi2 Apr 22, 2019

BugFix: make new code compile in Qt 5.7

50098be

It seems that later versions are happy to compare a `QString` with a `QLatin1Char` but that version isn't... Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven mentioned this pull request May 9, 2019

Enhance: add getTextFormat() function to Lua API #2491

Merged

vadi2 modified the milestones: 3.20.0, 3.21.0 May 12, 2019

SlySven commented May 30, 2019

View reviewed changes

src/Host.cpp Show resolved Hide resolved

SlySven commented May 30, 2019

View reviewed changes

SlySven added 0 - Work in progress and removed 1 - Ready for review labels May 30, 2019

SlySven assigned SlySven and unassigned vadi2 May 30, 2019

SlySven added 1 - Ready for review and removed 0 - Work in progress labels Jun 5, 2019

vadi2 previously approved these changes Jun 13, 2019

View reviewed changes

vadi2 removed this from the 3.21.0 milestone Jun 16, 2019

BugFix: fix debugging use only method to return correct member

56b20b5

Due to a copy-pasta error `(bool) Host::initialised()` was returning the value of the wrong member! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Merge branch 'development' into Enhance_overhaulStopwatches

4b1d687

BugFix: correct a recent merge conflict resolution failure

e7086bd

I accidentally ended up with some duplicate methods listed in the Host header file. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

keneanung reviewed Sep 27, 2019

View reviewed changes

keneanung removed their assignment Sep 27, 2019

demonnic approved these changes Sep 29, 2019

View reviewed changes

SlySven added 4 commits October 3, 2019 20:34

Merge branch 'development' into Enhance_overhaulStopwatches

65b0d2f

Conflicts resolved in: * src/Host.h Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Merge branch 'development' into Enhance_overhaulStopwatches

42fc9e2

SlySven merged commit e7af530 into Mudlet:development Oct 6, 2019

SlySven deleted the Enhance_overhaulStopwatches branch October 6, 2019 14:27

SlySven mentioned this pull request Oct 6, 2019

BugFix: fix function names in a couple of copy/paste errors in error texts #3147

Merged

vadi2 added a commit that referenced this pull request Oct 14, 2019

Revert "Enhance: overhaul stopwatches (#2516)"

104438d

This reverts commit e7af530. # Conflicts: # src/TLuaInterpreter.cpp

SlySven mentioned this pull request Nov 8, 2019

Enhance: overhaul stopwatches redux #3224

Merged

dicene pushed a commit to dicene/Mudlet that referenced this pull request Feb 19, 2020

Revert "Enhance: overhaul stopwatches (Mudlet#2516)"

6256474

This reverts commit e7af530. # Conflicts: # src/TLuaInterpreter.cpp

SlySven restored the Enhance_overhaulStopwatches branch June 22, 2020 18:00

SlySven deleted the Enhance_overhaulStopwatches branch June 22, 2020 18:37

Kebap mentioned this pull request Jan 5, 2021

Document persistent stopwatches #4585

Closed

Uh oh!

Conversation

SlySven commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

add-deployment-links bot commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jieiku commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bduza commented Apr 27, 2019

Uh oh!

Uh oh!

SlySven left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Kebap commented Jul 15, 2019

Uh oh!

SlySven commented Aug 21, 2019

Uh oh!

SlySven commented Sep 22, 2019

Uh oh!

demonnic commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SlySven commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

demonnic commented Sep 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Sep 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven commented Apr 22, 2019 •

edited

Loading

add-deployment-links bot commented Apr 22, 2019 •

edited

Loading

vadi2 commented Apr 23, 2019 •

edited

Loading

Jieiku commented Apr 26, 2019 •

edited

Loading

SlySven left a comment •

edited

Loading

demonnic commented Sep 23, 2019 •

edited

Loading

SlySven commented Sep 23, 2019 •

edited

Loading

SlySven Sep 29, 2019 •

edited

Loading