Improve: icons not coloured status text for special exits in exits dialog#5543
Conversation
…alog This is a bit more involved than the normal exit ones as we have to provide an extra class to be a "delegate" to handle the `QLineEdit` that is embedded in the `QTreeWidget` derived special `ExitsTreeWidget` when a text field (in this case the exit roomID) is edited. We use that and the code from Mudlet#5513 for normal exits to provide the same icons in that widget whilst the exit roomID is being edited and we put the same icon in a new column to the immediate right of that column when that field is not being edited. This is a major functional part extracted from the previous draft mega-PR Mudlet#5303 and deals only with the special exit roomIDs. This follows on from a separate PR Mudlet#5513 already merged to handle normal exits. 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. |
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I note that some concern has been raised about some almost identical messages being required to be translated for the different normal exit directions. I did take a look at the way those were put together and spotted that a common sentence could be separated out but though it shortened each text it did not really improve the overall situation: The first sentence is now identical to another one used in the same class but the second although it does use the same wording as another sentence, is not an exact duplicate because it needs the leading space in most - but not ALL locales - and the comment text together with the space means that there is not - I think - going to be any improvements in the translation effort required. Furthermore this PR is dealing with the Special exit case - and this point should have been made in the Normal exit one #5513 which has already been merged. |
Please compare my suggestion in #5556 |
src/dlgRoomExits.cpp
Outdated
| // A number but not valid | ||
| mpDlgRoomExits->setActionOnExit(mpEditor, mpDlgRoomExits->mpAction_invalidExit); | ||
| roomIdToolTipText = singleParagraph.arg(tr("Entered number is invalid, set the number of the room that this special exit leads to; " | ||
| "if left like this, this exit will be deleted when <tt>save</tt> is clicked.")); |
There was a problem hiding this comment.
- roomIdToolTipText = singleParagraph.arg(tr("Entered number is invalid, set the number of the room that this special exit leads to; "
- "if left like this, this exit will be deleted when <tt>save</tt> is clicked."));
+ roomIdToolTipText = doubleParagraph.arg(tr("Entered number is invalid. If left like this, this exit will be deleted when <tt>save</tt> is clicked."),
+ tr("Set the number of the room northwest of this one."));etc. in spirit of #5556
…SpecialExitsInExitsDialog Done to pull in any development branch changes that might touch the areas where this PR is working. Signed-off by: Stephen Lyons <slysven@virginmedia.com>
src/dlgRoomExits.cpp
Outdated
| roomIdToolTipText = singleParagraph.arg(tr("A positive room ID of the room that this special exit leads to is expected here; " | ||
| "if left like this, this exit will be deleted when <tt>save</tt> is clicked.")); |
There was a problem hiding this comment.
| roomIdToolTipText = singleParagraph.arg(tr("A positive room ID of the room that this special exit leads to is expected here; " | |
| "if left like this, this exit will be deleted when <tt>save</tt> is clicked.")); | |
| roomIdToolTipText = singleParagraph.arg(tr("A positive roomID is expected here. " | |
| "If left like this, this exit will be deleted when <tt>save</tt> is clicked.")); |
src/dlgRoomExits.cpp
Outdated
| mpAction_otherAreaExit->setToolTip(QString()); | ||
| mpAction_otherAreaExit->setIcon(mIcon_otherAreaExit); | ||
|
|
||
| mSpecialExitRoomIdPlaceholder = tr("(room ID)", "Placeholder, if no room ID is set for an exit."); |
There was a problem hiding this comment.
| mSpecialExitRoomIdPlaceholder = tr("(room ID)", "Placeholder, if no room ID is set for an exit."); | |
| mSpecialExitRoomIdPlaceholder = tr("(roomID)", "Placeholder, if no roomID is set for an exit."); |
src/dlgRoomExits.cpp
Outdated
| if (pSpecialExit->text(ExitsTreeWidget::colIndex_command) == mSpecialExitCommandPlaceholder) { | ||
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_command, singleParagraph.arg(tr("No command or Lua script entered, if left like this, this exit will be deleted when <tt>save</tt> is clicked."))); | ||
| } else { | ||
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_command, singleParagraph.arg(tr("(Lua scripts for some mapper packages {those using the <tt>mudlet-mapper/<tt>, i.e. I.R.E. Muds and StickMUD} need to be prefixed with \"script:\")."))); |
There was a problem hiding this comment.
I don't think it's wise naming the games here, because the list will change and this string will age unneccessarily
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_command, singleParagraph.arg(tr("(Lua scripts for some mapper packages {those using the <tt>mudlet-mapper/<tt>, i.e. I.R.E. Muds and StickMUD} need to be prefixed with \"script:\")."))); | |
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_command, singleParagraph.arg(tr("(Lua scripts for mapper packages using the <tt>mudlet-mapper/<tt> need to be prefixed with \"script:\")."))); |
Then again, I never even knew of this prerequisite, or how to describe it better maybe.
There was a problem hiding this comment.
I only found out when I looked to see where it was used - it isn't anything in the C++ core code but something in the Lua stuff that makes up that package and the original text always niggled me, it being "(Lua scripts need to be prefixed with \"script:\")" when I knew that wasn't the whole story...
There was a problem hiding this comment.
OK but I was wondering, why is that required then?
Looking at the mudlet-mapper scripts, they actually have it coded:
if string.starts(cmd, "script:") then
cmd = string.gsub(cmd, "script:", "")
loadstring(cmd)()So now I am puzzled as to why Mudlet would hint at using a game-specific feature at all.
That is not done for other games, and not done for other features of the same script either.
To me it seems now, this is a slippery slope and maybe the text should actually be removed.
The script/authors usually should take care of informing how to use it, not rely on a client tooltip.
There was a problem hiding this comment.
Let's remember that this has not been a source of confusion or a problem for anyone, I suggest not treating it as one until it actually manifests itself.
| } | ||
|
|
||
| if (pSpecialExit->text(ExitsTreeWidget::colIndex_command) == mSpecialExitCommandPlaceholder) { | ||
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_command, singleParagraph.arg(tr("No command or Lua script entered, if left like this, this exit will be deleted when <tt>save</tt> is clicked."))); |
There was a problem hiding this comment.
When there are two separate ideas, don't put a comma instead of full stop.
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_command, singleParagraph.arg(tr("No command or Lua script entered, if left like this, this exit will be deleted when <tt>save</tt> is clicked."))); | |
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_command, singleParagraph.arg(tr("No command or Lua script entered. If left like this, this exit will be deleted when <tt>save</tt> is clicked."))); |
The ideas "No command or Lua script entered, if left like this" did not have any connection intended, did they?
There was a problem hiding this comment.
Actually the ideas are connected - the absence of a command or script entry will, if not corrected, cause deletion.
There was a problem hiding this comment.
Yes the two ideas are connected, but read that part again. It says: If you leave it like this, nothing is entered. They are not connected that way, no?
What you probably intended to say was ".. and if left like this, then .." so that is where I suggest the full stop instead of "and".
src/dlgRoomExits.cpp
Outdated
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_exitRoomId, singleParagraph.arg(tr("A positive room ID of the room that this special exit leads to is expected here; " | ||
| "if left like this, this exit will be deleted when <tt>save</tt> is clicked."))); |
src/dlgRoomExits.cpp
Outdated
| pSpecialExit->setIcon(ExitsTreeWidget::colIndex_exitStatus, QIcon()); | ||
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_exitRoomId, singleParagraph.arg(tr("A positive room ID of the room that this special exit leads to is expected here; " |
src/dlgRoomExits.cpp
Outdated
| pSpecialExit->setToolTip(ExitsTreeWidget::colIndex_exitRoomId, singleParagraph.arg(tr("Entered number is invalid, set the number of the room that this special exit leads to; " | ||
| "if left like this, this exit will be deleted when <tt>save</tt> is clicked."))); |
src/ui/room_exits.ui
Outdated
| </property> | ||
| <property name="toolTip"> | ||
| <string><p>Click on an item to edit/change it, to DELETE a Special Exit set its Exit Room ID to zero.</p></string> | ||
| <string><p>Click on an item to edit/change it; to delete a Special Exit, select it and press the keyboard Delete key OR set its Exit Room ID to less than 1 OR clear the name/command entry.</p></string> |
There was a problem hiding this comment.
Keep it at 0, even though setting it to -23 could work as well, that sentence had too much info already.
Switch the options, mention the most simple one first.
Split the first idea off to make it shorter.
Replace more CAPS LOCKED words.
Use common "roomID" syntax.
| <string><p>Click on an item to edit/change it; to delete a Special Exit, select it and press the keyboard Delete key OR set its Exit Room ID to less than 1 OR clear the name/command entry.</p></string> | |
| <string><p>Click on an item to edit/change it. To delete a Special Exit, either set its roomID to 0, or select it and press the keyboard Delete key, or clear the name/command entry.</p></string> |
There was a problem hiding this comment.
Keep it at 0, even though setting it to -23 could work as well, that sentence had too much info already.
Sorry but I disagree, as it also warns that any value less than one will always cause exit deletion and not just that single value of zero - and in English, when referring to a number with an absolute value less than 100 it is usual to spell the number out in words...
Switch the options, mention the most simple one first.
The simplest one is click on the exit and press delete - and it works on multiple exits at a time (but I wasn't going to mention that)!
Split the first idea off to make it shorter.
Not sure about that as the previous point also alludes to click-and-drag to select several exits at a time. I guess it does improve the readability if it is a separate sentence.
Replace more CAPS LOCKED words.
Well, I thought that using OR helped to separate the alternatives, but will try with an "... either: xxxx; or yyyyy; or zzzz." form.
Use common "roomID" syntax.
Yeah, I am making that change throughout - even though it offends my sensibilities - as we have previously had that discussion about identity (noun) / I(dentiy) D(ocument) --> ID (abreviation) / Identity (English, capitalised, proper noun) and the democratically derived result was ID...
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…dialog This uses the same method of providing a custom item delegate has been used for the special exit roomID column in the `QTreeWidget` derived `ExitsTreeWidget` in PR Mudlet#5543 but instead of making use of an enhanced `QlineEdit` this will replace the standard `QLineEdit` with a `QSpinBox` whilst the special exit's weight is being modified. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…dialog (#5758) This uses the same method of providing a custom item delegate has been used for the special exit roomID column in the `QTreeWidget` derived `ExitsTreeWidget` in PR #5543 but instead of making use of an enhanced `QlineEdit` this will replace the standard `QLineEdit` with a `QSpinBox` whilst the special exit's weight is being modified. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…alog (Mudlet#5543) This is a bit more involved than the normal exit ones as we have to provide an extra class to be a "delegate" to handle the `QLineEdit` that is embedded in the `QTreeWidget` derived special `ExitsTreeWidget` when a text field (in this case the exit roomID) is edited. We use that and the code from Mudlet#5513 for normal exits to provide the same icons in that widget whilst the exit roomID is being edited and we put the same icon in a new column to the immediate right of that column when that field is not being edited. This is a major functional part extracted from the previous draft mega-PR Mudlet#5303 and deals only with the special exit roomIDs. This follows on from a separate PR Mudlet#5513 already merged to handle normal exits. Revised to adopt some peer review points. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
…dialog (Mudlet#5758) This uses the same method of providing a custom item delegate has been used for the special exit roomID column in the `QTreeWidget` derived `ExitsTreeWidget` in PR Mudlet#5543 but instead of making use of an enhanced `QlineEdit` this will replace the standard `QLineEdit` with a `QSpinBox` whilst the special exit's weight is being modified. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

This is a bit more involved than the normal exit ones as we have to provide an extra class to be a "delegate" to handle the
QLineEditthat is embedded in theQTreeWidgetderived specialExitsTreeWidgetwhen a text field (in this case the exit roomID) is edited. We use that and the code from #5513 for normal exits to provide the same icons in that widget whilst the exit roomID is being edited and we put the same icon in a new column to the immediate right of that column when that field is not being edited.This is a major functional part extracted from the previous draft mega-PR
# 5303#5308 and deals only with the special exit roomIDs. This follows on from a separate PR #5513 already merged to handle normal exits.Signed-off-by: Stephen Lyons slysven@virginmedia.com
Release post highlight
Coloured text in room exits dialogue for special exits now removed and replaced with status icons as the ones now used for normal exits.
Edited to correct PR reference - REMEMBER TO CORRECT THIS DETAIL IN THE COMMIT MESSAGE WHEN THIS PR IS SQUASHED AND MERGED!