Skip to content

Enhance: overhaul stopwatches redux#3224

Merged
SlySven merged 3 commits intoMudlet:developmentfrom
SlySven:Redo_Enhance_overhaulStopwatches
Nov 30, 2019
Merged

Enhance: overhaul stopwatches redux#3224
SlySven merged 3 commits intoMudlet:developmentfrom
SlySven:Redo_Enhance_overhaulStopwatches

Conversation

@SlySven
Copy link
Copy Markdown
Member

@SlySven SlySven commented Nov 8, 2019

This commit 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 EDIT: 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 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).

(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 .

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

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>
@SlySven SlySven requested a review from a team as a code owner November 8, 2019 03:57
@SlySven SlySven requested review from a team November 8, 2019 03:57
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Nov 8, 2019

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

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.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Nov 17, 2019

I don't have time to extensively test this, but a little thing, codefactor is rightly pointing out duplication in

Mudlet/src/TLuaInterpreter.cpp

Lines 2134 to 2152 in 6ab9db1

return lua_error(L);
}
int watchId = 0;
Host& host = getHostFromLua(L);
if (lua_type(L, 1) == LUA_TNUMBER) {
watchId = static_cast<int>(lua_tointeger(L, 1));
} else {
QString name = QString::fromUtf8(lua_tostring(L, 1));
// Using an empty string will return the first unnamed stopwatch:
watchId = host.findStopWatchId(name);
if (!watchId) {
lua_pushnil(L);
if (name.isEmpty()) {
lua_pushstring(L, "no unnamed stopwatches found");
} else {
lua_pushfstring(L, "stopwatch with name \"%s\" not found", name.toUtf8().constData());
}
return 2;
,

Mudlet/src/TLuaInterpreter.cpp

Lines 2179 to 2197 in 6ab9db1

return lua_error(L);
}
int watchId = 0;
Host& host = getHostFromLua(L);
if (lua_type(L, 1) == LUA_TNUMBER) {
watchId = static_cast<int>(lua_tointeger(L, 1));
} else {
QString name = QString::fromUtf8(lua_tostring(L, 1));
// Using an empty string will return the first unnamed stopwatch:
watchId = host.findStopWatchId(name);
if (!watchId) {
lua_pushnil(L);
if (name.isEmpty()) {
lua_pushstring(L, "no unnamed stopwatches found");
} else {
lua_pushfstring(L, "stopwatch with name \"%s\" not found", name.toUtf8().constData());
}
return 2;
,

Mudlet/src/TLuaInterpreter.cpp

Lines 2218 to 2236 in 6ab9db1

return lua_error(L);
}
int watchId = 0;
Host& host = getHostFromLua(L);
if (lua_type(L, 1) == LUA_TNUMBER) {
watchId = static_cast<int>(lua_tointeger(L, 1));
} else {
QString name = QString::fromUtf8(lua_tostring(L, 1));
// Using an empty string will return the first unnamed stopwatch:
watchId = host.findStopWatchId(name);
if (!watchId) {
lua_pushnil(L);
if (name.isEmpty()) {
lua_pushstring(L, "no unnamed stopwatches found");
} else {
lua_pushfstring(L, "stopwatch with name \"%s\" not found", name.toUtf8().constData());
}
return 2;
, and

Mudlet/src/TLuaInterpreter.cpp

Lines 2340 to 2358 in 6ab9db1

return lua_error(L);
}
int watchId = 0;
Host& host = getHostFromLua(L);
if (lua_type(L, 1) == LUA_TNUMBER) {
watchId = static_cast<int>(lua_tointeger(L, 1));
} else {
QString name = QString::fromUtf8(lua_tostring(L, 1));
// Using an empty string will return the first unnamed stopwatch:
watchId = host.findStopWatchId(name);
if (!watchId) {
lua_pushnil(L);
if (name.isEmpty()) {
lua_pushstring(L, "no unnamed stopwatches found");
} else {
lua_pushfstring(L, "stopwatch with name \"%s\" not found", name.toUtf8().constData());
}
return 2;
, do you think you could extract that to a function?

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>
@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Nov 24, 2019

@PhoenixCodes checked this out and says the ire mudlet mapper works fine!

Copy link
Copy Markdown
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People say it works well!

class dlgNotepad;
class TMap;

class stopWatch {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be StopWatch since all other class names are capitalised?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, look at line 58 - that does not have an initial capital either! 😜

Unless we make it a style requirement that all Mudlet created classes have a particular naming style I am happy to leave it unchanged so as to get this epic topic (I was going to say PR but it of course is actually spread over more than one PR attempt) merged in...

Copy link
Copy Markdown
Member Author

@SlySven SlySven Nov 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... we can, of course, tweak such naming conventions (and that of the files that contain them) in a separate PR if that is felt to be warranted (I guess the GLWidget is the opposite case).

@vadi2 vadi2 assigned SlySven and unassigned vadi2 Nov 29, 2019
@SlySven SlySven merged commit f9fad62 into Mudlet:development Nov 30, 2019
@SlySven SlySven deleted the Redo_Enhance_overhaulStopwatches branch November 30, 2019 16:43
dicene pushed a commit to dicene/Mudlet that referenced this pull request Feb 19, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants