Rebased2 fix replay #148
Closed
Rebased2 fix replay #148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
First bug was one that printed out a copy of the current chunk of the replay for EACH character in that chunk - fix was to move the print line to be outside of the for() loop that examines each character. Second bug was an off-by-one one that over-estimated the length of the chunk of data from the replay file. Although this commit temporarily enables the relevant debug lines I intend to comment them back out again at the end of the series of commits! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Global items renamed (and moved from middle of .cpp file to top, only used within class file): QAction * actionSpeedDisplay ==> pActionSpeedDisplay QAction * actionReplayTime ==> pActionReplayTime QLabel * replaySpeedDisplay ==> pReplaySpeedDisplay QLabel * replayTime ==> pReplayTime QTimer * replayTimer ==> pReplayTimer QToolBar * replayToolBar ==> pReplayToolBar Global items moved out of class header (as not used outside of class files): QAction * actionReplaySpeedUp ==> pActionReplaySpeedUp QAction * actionReplaySpeedDown ==> pActionReplaySpeedDown Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…time display failure The icons style/sizes were not being set and were not modified by changes made in preferences. In Qt5 a NULL QTime is invalid and now adding a time to it no longer makes it valid (this is different from Qt4 where adding a time DID make it valid). Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
For replays where input occurs less often than once a second the time display was only updated when each chunk was read with the result that the time display could be static for long periods. This commit increments the display by one second every second divided by the speed factor. Due to the way it is implemented it is somewhat approximate but the displayed time is reset to the right value when each chunk is processed. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This commit uses negative values of the mudlet class member mReplaySpeed to indicate speeds that are the reciprocal of the integer number. It tweaks the display of the speed to show a fractional value rather than a decimal one in the limited cases (1/2, 1/4, 1/8) that are coded for. As the lowest supported speed is altered by this commit it also seemed reasonable to set a reasonable upper limit of 128 so that one second of replay corresponds to just over two minutes of real-time in the original recording. At the same time the infrastructure to support translation of the widgets that contain text is implemented for future internationalisation. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Now needed for some work being done on fixing some bugs in the replay system. This is not the exact same code that was previously removed as the change to using QSharedPointer<Host> to access the HostPool members instead of the previously used direct Host * pointers. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Generate and provide messages about both replay starting success and
failure for both main Toolbar and lua script initiated replays from
the same place {QString cTelnet::loadReplay(QString & filename,
bool "called from Main Toolbar")} so that the latter invocation will not
produce messages on the main console for the profile. Only for the former
case can a message be produced when a replay ends. A future commit is
intended to provide system events related to replays to user scripts to
overcome this issue.
Moved some global items used by cTelnet::loadReplay() to top of file.
Remove redundant TConsole::loadRawFile(std::string) as all it does is
convert the file name given in a call from TLuaInterpreter::loadReplay()
into a pathFile relative to the profile's log directory before calling
cTelnet::loadReplay() and that can (and now is) done directly by the
TLuaInterpreter method revised here.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…hanges
The replay system can only handle one replay at a time, and although a user
script can initiate a replay it previously could not know about the ending
of that replay. Also if a replay is taking place in one profile, another
one will get informed that the system is busy with a replay but cannot know
without repeatedly trying to start it's own when the system is available.
This commit defines several sysReplayEvents, with additional arguments as
follows:
"myReplayStart" - a replay that this profile has requested, either from a
script or from the main tool bar has been opened SUCCESSFULLY and is
now running. Note that a failure to start the reply will be reported
back in an error return to a lua script that tries to start one so does
not need an event.
"otherReplayStart" - a replay has been started in another profile and so
replays cannot currently be started by this profile.
"myReplayOver" - the replay that this profile had started has finished.
"otherReplayOver" - the replay that another profile had started has
finished. The replay system is thus now available to this profile.
"replaySpeedChange", (float)new_replay_rate, (float)old_replay_rate - not
much use currently but notifies the profile running a replay that the
replay speed has just been changed, the following arguments are the new
and the previous rates (x0.125 = 1/8 to x128). It is anticipated that
a lua command getReplayStatus() to be added might return a current and
total replay time so combining this with that data may give useful
information if a script wants to estimate replay timings.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Found that during testing with multiple profiles closing the profile that
was running the replay caused a crash - in theory it could also happen when
using a single profile if the timer that cause the next replay chunk to be
loaded occurs during close down. Cleanest fix is to provide a
cTelnet::abortReplay() member that duplicates the actions that would happen
at the end of the replay normally. Modifies the mudlet::replayOver(...)
method so that the affected profile gets a "myReplayAborted" rather than
"myReplayOver" "sysReplayEvent" event which is not likely to be much use
at profile closedown but may have use if the toolbar or lua interpreter
gains a means to stop a replay {change the "disabled" during "replay"
toolbar button to an enabled "stop" one, or an abortReplay() lua command is
added} in the future.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…by new Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Until now the Mudlet replay files had the generic ".dat" file extension and there was no protection against the user loading a file that is NOT a replay file. Given that the file includes embedded values to control the amount to read from the file using a time sent to a QTimer in another embedded value this seems unwise. This commit adds some data to the start of a replay file that serves to unique identify it (and adds a version code so that if the format is changed in the future old formats can still be identified correctly!) It also changes the extension to ".mreplay" to stand for Mudlet replay, moving away from a three letter string as an extension as that restriction went away in operating systems probably way older than Mudlet ever supported. Old format replays will still be acceptable, but the user will have to choose to use those be changing the file type filter from the file requester dialog that is called when a replay is requested from the main toolbar button. The use of "dd-MM-yyyy#hh-mm-ss" as the time format to name the replay file may look pretty but is unhelpful in sorting as an alphabetical sort of the directory contents will group by day of month, month and then year etc. This is OK provided the date-time meta-data is preserved but if such files are archived or copied that date-time information can be lost and then picking out files by age can become very difficult if the listing takes more than one screen-full. Putting the most significant date-time item first, as is common in some world regions, makes an alphanumeric sort produce the same results as a date-time meta-data sort and preserves the sort order if the latter data is corrupted. In addition, by structuring some elements in the header in a particular layout it makes it possible to determine some characteristics about the file from the command line - this is particularly relevant on *nix platforms which offer the file(1) command using data parsing routines configured by magic(5). The differences now mean instead of receiving the lowest common denominator of file type information, the generic "data" type: '$> file ./WoTMUD_Jomin/log/23-02-2014#12-31014.dat ./WoTMUD_Jomin/log/23-02-2014#12-31014.dat: data' one instead can get: '$> file ./WoTMUD_Jomin/log/2014-09-12#17-28-11.mreplay ./WoTMUD_Jomin/log/2014-09-12#17-28-11.mreplay: Mudlet replay file, version 2, started: Fri Sep 12 17:28.11 2014(UTC), 15.106(s) long, profile: "WoTMUD_Jomin"' Some existing class members have been modified for clarity or other reasons: cTelnet::readPipe() ==> cTelnet::slot_readPipe() to emphasise it is a slot method. QTime cTelnet::timeOffset ==> QDateTime cTelnet::mRecordLastDateTimeOffset to allow for unlikely but possible records of longer than a day or for when DST changes - both of which a QTime can't handle. mudlet::mpReplayTimer ==> mpReplayDisplayTimer to differentiate it from the new mpReplayChunkTimer, the first is associated with updating the time display periodically every (second/speed) and the second with loading the next chunk after the right time period. Both are now included in the class header so they can be accessed / controlled from outside of the class which is not possible if they are declared as class globals as the first had been in the past. cTelnet::mLoadingReplay ==> cTelnet::mIsReplaying to compliment this: TConsole::mRecordReplay ==> TConsole::mIsRecording TConsole::mReplayFile ==> TConsole::mRecordFile TConsole::mReplayStream ==> TConsole::mRecordStream above two are associated with making a recording not with its playback Also: mLogFileName, local to TConsole::slot_toggleReplayRecording() MASKS a variable with the same name declared in the class header, it has been renamed to recordPathFile. Another local variable of the same name in TConsole::slot_toggleLogging() had the same effect - the local there has now been removed so that that method will use the original class instance after all. Although not marked with any licence terms I hereby state that the "magic" file entry added to the INSTALL file is, for the avoidance of any confusion to be considered as licensed as "GPLv2+" - if that is not suitable then please contact me for alternative options: Signed-off-by: Stephen Lyons <slysven@virginmedia.com> Conflicts: src/TConsole.h src/ctelnet.h
… replays As it is possible to initiate a replay from with a user Lua script it does not seem unreasonable to also provide command to abort a replay, to obtain various bits of status information about such a replay and to provide a means to control the playback speed in the same way as the toolbar does. This commit provides all of these features. Debug displays of chunk data have been commented out with an explanatory comment! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Revise TLuaInterpreter::setReplaySpeed() bad argument message to use "number" not "float" as what is required. Revise TLuaInterpreter::load RawFile() to now return THREE results, in order: * true or nil - on success or failure respectively to load and start a replay. * message string - a, possibly translated, message - include upon success, which in that case will include the absolute path to the file loaded. * number code - currently in range 0 to 5 with 0 corresponding to success, which is intended to make lua script writing easier if Mudlet becomes multilingual as the numbers won't change even if the message text does. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Closed
| @@ -285,6 +285,8 @@ Q_OBJECT | |||
| int mCurrentSearchResult; | |||
| QList<int> mSearchResults; | |||
| QString mSearchQuery; | |||
| const QByteArray replayMagic = {"MudletReplayFile"}; // Do not change, will be used to validate replay file in the future. | |||
| const quint8 replayVersion = 2; // Just in case we ever change the format, treat unversioned "old" replays as version 1 . | |||
There was a problem hiding this comment.
Aaargh - Travis is barfing on these two lines - what is the best way to fix:
In file loaded from ActionUnit.cpp:29:0:
TConsole.h:288:39: error: a brace-enclosed initializer is not allowed here before '{' token.
TConsole.h:288:58: sorry, unimplemented: non-static data member initializers
TConsole.h:288:58: error: in-class initialization of static data member 'replayMagic' of non-literal type
TConsole.h:289:41: sorry, unimplemented: non-static data member initializers
make: *** [ActionUnit.o] Error 1
make: *** waiting for unfinished jobs....
|
Note, you can force push to update a branch and just continue using the same pull request without having to create another one. |
|
I don't think we'll be translating API messages, just the user interface, when we start doing translations. |
|
It indeed doesn't compile at the moment: |
…alues Whilst interactively merging in code from development branch missed a change in TConsole::getHost(). Added some missing run-time return messages from: * TLuaInterpreter::abortReplay() * TLuaInterpreter::setReplaySpeed() * TLuaInterpreter::getReplaySpeed() Corrected trying to initialise some replay constants in the wrong place. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
| @@ -96,7 +96,6 @@ Q_OBJECT | |||
| void resetMainConsole(); | |||
| void echoUserWindow( QString & ); | |||
| Host * getHost(); | |||
| TCommandLine * mpCommandLine; | |||
There was a problem hiding this comment.
Note NOT deleted, just moved down to the rest of the members below the methods.
|
Right this should now be a go - just that as I've commented some debug lines out (that I said I would) that have now been pulled into this branch by commit a4f9e44 it is preventing an automatic merge at this time. |
|
I haven't finished reviewing and I still have objections, so please don't merge this yet. |
|
As a more general note, I'm reviewing each commit in the series, one by one, since it's easier that way. Hence, I've only reviewed the second one so far. I approved of the first one, and merged it, so it's already part of development (that's the reason the merge has conflicts, because I formatted the change with clang-format). |
|
What is the idea behind putting the profile name into the replay? It is leaking private information (the profile name, which is often the characters name) this way and I'm not sure what benefit does it add. |
ahmedcharles
force-pushed
the
development
branch
from
54ec9eb
to
c035b0c
Compare
| lua_pushstring( L, "loadRawFile: wrong argument type" ); | ||
| QString fileName; | ||
| if( ! lua_isstring( L, 1 ) ) { | ||
| lua_pushstring( L, tr( "loadRawFile: bad argument #1 (replay file, optionally with a path, as string expected, got %1)" ) |
There was a problem hiding this comment.
What does it mean 'optionally with a path'? Perhaps 'optionally as a path'?
There was a problem hiding this comment.
Meant without a path will load from //log/ folder, or can specify a path relative to that with ../..//log/ to load from a different profile.
|
In the following replay: http://ge.tt/1xN0lZy1/v/0 The timer skips from 6s, to 8s, to 10s - is that intended? |
|
@vadi2 :
When inspecting the file from the command line it is useful to know from which profile the file resulted, if you have multiple profiles or play different muds, and if you copy/move/share the file outside of the original directory you could find it hard to tell from whence it came. |
|
I can't get anything useful from that ge.tt link, I got a file that seems to be xml but it had the right file-name, so I tried "file ./2014-09-21#14-35-46.mreplay" and the "XML document" result text revealed what I suspected, I hadn't got the file: So the magic is already working for me! 😒 |
|
Try |
|
Can you rebase this at some point? Thanks :) |
|
What is the latest status on this? |
|
In progress - most of the way through - I aim to finish it by the end of the Weekend if not before (been distracted the last couple of days finding a new place to live... 😌 ) |
|
Closed in favour of #200 that takes into account some conflicting other changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces #147 as that won't merge because a recent commit (using QPointer for Host class now) touches one file too close to a change one of these commits does. This also incorporates a couple of suggestions from others that I've taken on board.
It feels a little like I'm playing "pin the tail on the donkey" - except the donkey is alive and moving around