Add persistent command history #6767
Conversation
This works for single word named command lines of all types, and saves them all separately in the profile's base directory. This should close Mudlet#2007. I am not yet happy with the save format - it does save the history in the order that the entries are in in the `(QStringList) TCommandLine::mHistoryList` member in a UTF-8 form (which makes for easier external modification if wanted (rather then Qt's default UTF-16 internal `QString`) but the `QDataStream` format saves a `QByteArray` with a leading int32_t size element - which, while good from a maintaining integrity point of view makes it not amenable to external editing. Testing also needs to be carried out in (possibly on a Window system) to check that things still work with names that are more challenging for an OS - e.g. including '\\' or '/' or ':'s... Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
We'll avoid using the names for the extra command lines as part of a filename by instead storing them in a per profile `.ini` format file. Using such a file to store per-profile is something that has been on my TODO list for years (I prototyped code to do it many years ago) - so I have added two functions to the Host class that hopefully can replace the existing `readProfileData(...)` and `writeProfileData(...)`. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…or '\' Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Otherwise it will grow indefinitely large as every single entry is retained. A knob for this has been provided on the profile preferences and it is saved with the profile's XML game save file (not in the base directory of the profile. It covers the range of 0 to 500 entries with a default of 50 and a step size of 10. This value is applied to ALL command lines in a profile. Note that this limit is only applied when the commend history is saved, it can still grow to be larger than the limit whilst the profile is active! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
|
Are you across the work Tim has been doing in #6733 ? |
I've only just looked at it - I started on this unaware that he was working on it, and when I became aware (yesterday) I deliberately avoided viewing it to avoid accusations of plagiarism. 😇 Now I have looked and I see that he has only considered the main command line (AFAICT) - mine treats each one separately - and can handle issues that would arise from using the names of additional ones in file names. Initial testing shows it seems to work; but I haven't exercised the mechanism to limit the number of entries to save which IS essential as otherwise the list will grow forever (assuming infinite different command are entered...!) |
|
Hmm, Tim's PR has been open for two weeks - and this is not the first time it's happened, you might want to pay attention to linked PRs:
That said, the bounty process does allow this:
So I'll put it up to the Decision maker patrons to decide when @atari2600tim's PR is ready. |
No need, I'm not interested anymore.
I wrote it for an open source project. The central task is reading and writing an array of strings to file. |
This adds three tool-buttons shown to the left of each command-line that, in order: * sets the command-line's most recently used commands (up to the per-profile limit) to be saved in a file. * sets the next command entered NOT to be saved in the history at all, as a side effect of the way it works if the exact command has been used before then entering it again with this option selected WILL erase that particular entry from the stored history (and when the profile is closed) from the file backing it up as well. After the command has been entered this option is automatically deselected and the selected option reverts to the first one. * sets the command history for this command line to NOT be saved at all - as Mudlet would behave before this PR came about. The commands are still remembered for the current session but they are not written out to a file though if they have been in a previous session than those will remain in the file as it will not be updated when the profile is closed. Also, if the number of lines to be saved is set to zero (the "None") value in the settings then all three buttons are disabled (greyed out) in all the command lines for that profile. To add buttons to control this feature in the `QPlainTextEdit` proved to be quite tricky. Unlike the `QLineEdit` class there is no provision to add `QAction`s to either end. Instead I had to create a `TCommandLineWidget` class to encapsulate a `TCommandLine` as well as the three buttons in a `QFrame` container widget. Even this was not trivial but was possible to do - unlike an initial failed attempt that tried to extend the `TCommandLine` class itself to inherit from those other widgets as well. The existing methods/interactions with the original `TCommandLine` had to be revised to either work on the new wrapper class or to access the wrapped one via that instead of working directly with it. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This PR only touches the `./src/ui/profile_preferences.ui` that has such an
out-of-order `QGridLayout` items in the one called `"gridLayout"` which is
the layout for the `QGroupBox` called `"groupBox_accessibility"` - were I
to rename that layout from the generic (probably Qt Designer created) name
I would call it `"gridLayout_groupBox_accessibility"` but I will refrain
from doing that here as it will only cloud the `diff`.
The following `QGridLayout`s that still need attention are not modified by
this PR so will not be "fixed" here:
* `./src/ui/connection_profiles.ui` --> `"gridLayout"` - out of order
* `./src/ui/custom_lines.ui` --> `"gridLayout_3"` - redundant
* `./src/ui/custom_lines.ui` --> `"gridLayout_style"` - out-of-order
* `./src/ui/custom_lines.ui` --> `"gridLayout_4"` - redundant
* `./src/ui/delete_profile_confirmation.ui`
--> `"gridLayout"` - out-of-order
* `./src/ui/vars_main_area.ui`
--> `"gridLayout_frame_vars_main_area"` - out-of-order
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
The latest windows test version does not show the title bar and accompanying buttons in windows. Which is to say I cannot resize it, maximize, or easily close it. I'm also not sure I'm a fan of the 3 new buttons attached to every single command line, it drastically changes the look of anything already using a command line without anything I can do about it. This also makes it much more unwieldy to try and incorporate it into new geyser objects, for instance a number spinner I'm working on where I'd planned to use a small command line as a number entry option. But with these extra buttons tacked on suddenly this widget is required to be twice as wide just to fit the command line in. |
Not sure that this is the cause of that - TBH it sounds like you have gotten into full-screen mode - which I know some OSes do allow outside of the application itself...
Yeah, I get where you are coming from - I am thinking that I might be able to get the three options onto the context menu - and perhaps leave a single ( |
|
It'd be a lot less difficult in the layout than 3 of them, but ideally it would be something I could hide even still, so I could get the current behavior of "just the input box" again. |
|
TBH I now see that it can be done entirely within the context menu of each command line - but I do wonder about the feature's visibility - in the "Forget this line" mode it only stays active for, guess what, the one line before dropping back to the "Save commands" mode and I'm not sure that that will be apparent without some visible status indication, yet finding a way to show that is tricky - it was a major hassle trying to get those buttons incorporated into the |
This reduces the amount of disruption this PR was causing to the `TCommandLine` class. I do have concerns about indicating to the end-user that either the next or all commands are not being saved for the command line going forward when we announce the feature that they will be saved (by default) but I have not yet come up with a foolproof scheme to show that yet. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Signed-off-by: Stephen <slysven@virginmedia.com>
🤦♂️ I forgot to do it! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…g(...) This is so that the profile wide command line setting from the preferences is also available to the Lua sub-system. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
src/XMLimport.cpp
Outdated
| } else { | ||
| // This is the default valuem though prior to the introduction of this | ||
| // it would have effectively been zero: | ||
| pHost->setCommandLineHistorySaveSize(50); |
There was a problem hiding this comment.
Oh, need to change the original default of 50 to match the 500 in the preferences now...
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
How about this take to balance out what I mentioned in #6767 (comment) ? This would be in addition to the right-click
That way most folks won't have to deal with the option and it won't get in their way, and the few that do can have it for their needs. Remember that Mudlet stored character passwords in plaintext for years until secure storage came about, but hardly anyone was bothered by it. What engineers think is important can sometimes be different from what is actually important to the players :D (I have a bit more feedback on the menu, but want to take it one step at a time) |
|
"Show fine-grained command history options" - what do you mean? Which command line - all of them or just the "main" one? If you want to simply turn the whole option off I could arrange for the context menu option to be hidden rather than just "greyed-out" and disabled for all command-lines if you select "None" in the preferences but I was thinking that showing that there was an option but it had been switched off/overridden by a master prefernce was a more informative way to go. I think that having separate controls for each command line is required given that we keep separate command histories for each command line during the session. |
This would apply to all of them.
I want the history persistence to stay, but without the additional right-click menus, because I think they would be shown far more than they are actually needed or desired to be seen.
This doesn't apply to controls, but rather visibility of them. If someone wants to see them, they can enable them, but I don't see the larger majority of players needing to see them - and they'll get in the way. |
Look I have taken the controls to a sub-menu so that the current option is visible in the main menu (and shows the status of it for that command line) but removing that altogether seems unhelpful. Remember that the "forget next command" status DOES change after a command has been entered (back to the "Save commands" one) and the icon changing is the simplest way to show that has happened - or is pending to happen.
Given that the setting can be manipulated by the Lua sub-system now - how can the end-user determine what the current state is? I suppose it might be possible to reproduce the UI as I have it now with an additional Lua package but I will be perfectly frank and say I am not the person to do it like that - and I am not sure the visual UI (with the automatically greyed out icons and the tooltext changes should the system be disabled so as to behave like Mudlet does without this PR) could be reproduced easily anyhow. As for "getting in the way" I don't see how this is any more intrusive than anything else we have put in the context menu? I can't recall the last time I used the "Search" feature but there isn't a way to hide that currently, yet I don't mind that being there... 🤔 Has there been any feedback on this PR in either direction from any testers yet? |
|
There is no search feature in the right-click menu -
My take on this is still the same as in #6767 (comment): nothing about command persistent should be added to the right-click menu default and out of the box as too few people in too few situations will find it useful. I feel strongly about this as I believe it is important for Mudlet to remain modern. This is learning from mistakes of other clients that have added so many niche options that players consider them complex and difficult to use. I would like to see if I'm alone in this or no, so anyone following this, please weigh in! |
|
I think this is not very frequent thing to change. More like set once and never change it again. Coukd be safely in regular settings screen window only. |
|
I don't foresee needing to see these settings that often myself, and I'm what you might call a "power user" in that regard. This context menu is already getting pretty lengthy, and I wouldn't be against finding a way to trim/condense it even further, but that should perhaps be a separate ticket to discuss actions beyond adding (or rather not adding) more to the context menu as part of this PR. I am hoping to get the time to test the new Lua API functions later today and weigh in on that aspect as well, haven't had quite as much time over the last week as I'd hoped I would. |
|
My thoughts on this:
|
Sorry, I was mixing it up with the
As for never changing the option I agree that once you have decided between the "save"/"don't" save options you are not going to change between them - however if you go for the "save" option you may will want access to the "don't save the next one" option occasionally and accessing that from the context menu still seems like the most intuitive method. @keneanung 's point about password entry has relevance but directly coding that to the server negotiating to perform ECHOing is not a good idea - whilst we already prevent reproducing the entered command on the output ( I do hope that in the future the "forget the next command" can be combined with a context menu option (which the user controls directly or has the option to couple or not to the Server ECHOing) to switch to a password entry mode. However that is not something that Qt offers to us because the widget from which we derive the |
I don't want to hide more from the consoles. I just don't want to save command line entries as long as that option is on. |
|
We're all in agreement then, it's desired that the command line's UI does not change in any way (nothing on the right-click menu nor as attached buttons), and I agree with @keneanung spelling out how it should work. This is a great feature, just the current UI aspects of it are a bit much. |
|
I don't think you all are getting one feature of this enhancement that will turn into a defect if there is no easy way to control it! If you are saving the command history, which we agree, most people will want on by default, what steps do we expect users to take should they want to type in something secret that they do not want written into a plain text file? They must:
|
|
It could be a problem but I can't judge how realistic it will be in practice, let's see if the current solution:
Works and if it doesn't, we'll improve on it. And in the meantime, there will be no space taken up in the UI for this uncommon scenario (which everyone seems to agree is a good idea not to do). |
|
Well, if there is no UI and you really want just the:
then I can then rip out a whole wodge of code associated with the "forget the next command (only)" functionality... 🙄 |
Conflicts resolved in: * src/TCommandLine.cpp Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Remove the UI and all the code associated with the "forget next command" option. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
| QPlainTextEdit::mousePressEvent(event); | ||
| // Process any other possible mouseReleaseEvent - which is default context | ||
| // menu handling - and which accepts the event: | ||
| QPlainTextEdit::mouseReleaseEvent(event); |
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Make it so you can do `QDebug() << myRoom` and the room name, area, and
exits will get printed to debug output
#### Motivation for adding to Mudlet
Desired by developer
#6767 (comment)
#### Other info (issues closed, discussion etc)
<!-- Keep the title short & concise so anyone non-technical can
understand it,
the title appears in PTB changelogs -->
#### Brief overview of PR changes/additions
Make it so you can do `QDebug() << myRoom` and the room name, area, and
exits will get printed to debug output
#### Motivation for adding to Mudlet
Desired by developer
Mudlet#6767 (comment)
#### Other info (issues closed, discussion etc)
Brief overview of PR changes/additions
Saves each command line's history when a profile is closed and restores the data the next time the profile is opened. Each command line is handled separately.
Motivation for adding to Mudlet
It is a bounty item!
Other info (issues closed, discussion etc)
This works for command lines of all types, and saves them all separately in the profile's base directory.
This should close #2007.
We'll avoid using the names for the extra command lines as part of a filename by instead storing them in a
QSettingsper profileprofile.iniformat file.Using such a file to store per-profile details is something that has been on my TODO list for years (I prototyped code to do it at least as far back as 2016 I think) - so I have added two functions to the Host class that hopefully can eventually replace the existing
readProfileData(...)andwriteProfileData(...).Also added code to fix a corner case where a TCommandLine has a name containing '/' or ''.
Also added a limit to the number of command line history entries to save. Otherwise it will grow indefinitely large as every single entry is retained. A knob for this has been provided on the profile preferences and it is saved with the profile's XML game save file (not in the base directory of the profile). It covers the range of 0 to 10,000 entries with a default of 500 and a logarithmic step size (1, 2, 5, 10) for each multiple of 10. This value is applied to ALL command lines in a profile.
Note that this limit is only applied when the commend history is saved, it can still grow to be larger than the limit whilst the profile is active!
Signed-off-by: Stephen Lyons slysven@virginmedia.com