Skip to content

Improve: icons not coloured status text for special exits in exits dialog#5543

Merged
SlySven merged 4 commits intoMudlet:developmentfrom
SlySven:Improve_iconsNotColouredStatusTextForSpecialExitsInExitsDialog
Nov 7, 2021
Merged

Improve: icons not coloured status text for special exits in exits dialog#5543
SlySven merged 4 commits intoMudlet:developmentfrom
SlySven:Improve_iconsNotColouredStatusTextForSpecialExitsInExitsDialog

Conversation

@SlySven
Copy link
Copy Markdown
Member

@SlySven SlySven commented Oct 26, 2021

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 #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!

…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>
@SlySven SlySven requested a review from a team as a code owner October 26, 2021 00:40
@SlySven SlySven requested a review from a team October 26, 2021 00:40
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Oct 26, 2021

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 26, 2021

Messages
✔️

PR type: Improvement

Generated by 🚫 dangerJS against 1b3277b

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

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.

Looks good to me 👍

This adds new texts, so @Mudlet/ui-review @Kebap could you give your approval as well?

@SlySven SlySven requested review from a team and Kebap October 29, 2021 23:00
@Kebap Kebap added the hacktoberfest-accepted Not yet merged, but a welcome addition already! :-) label Oct 30, 2021
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Nov 7, 2021

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:
image

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.

@Kebap
Copy link
Copy Markdown
Contributor

Kebap commented Nov 7, 2021

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.

Please compare my suggestion in #5556

// 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."));
Copy link
Copy Markdown
Contributor

@Kebap Kebap Nov 7, 2021

Choose a reason for hiding this comment

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

-        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>
Copy link
Copy Markdown
Contributor

@Kebap Kebap left a comment

Choose a reason for hiding this comment

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

More text suggestions

Comment on lines +188 to +189
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."));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
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."));

mpAction_otherAreaExit->setToolTip(QString());
mpAction_otherAreaExit->setIcon(mIcon_otherAreaExit);

mSpecialExitRoomIdPlaceholder = tr("(room ID)", "Placeholder, if no room ID is set for an exit.");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.");

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:\").")));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's wise naming the games here, because the list will change and this string will age unneccessarily

Suggested change
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.

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.

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

Copy link
Copy Markdown
Contributor

@Kebap Kebap Nov 7, 2021

Choose a reason for hiding this comment

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

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.

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.

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.")));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When there are two separate ideas, don't put a comma instead of full stop.

Suggested change
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?

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.

Actually the ideas are connected - the absence of a command or script entry will, if not corrected, cause deletion.

Copy link
Copy Markdown
Contributor

@Kebap Kebap Nov 7, 2021

Choose a reason for hiding this comment

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

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

Comment on lines +936 to +937
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.")));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See suggestion from above

Comment on lines +930 to +931
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; "
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See suggestion from above

Comment on lines +926 to +927
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.")));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See suggestion from above

</property>
<property name="toolTip">
<string>&lt;p&gt;Click on an item to edit/change it, to DELETE a Special Exit set its Exit Room ID to zero.&lt;/p&gt;</string>
<string>&lt;p&gt;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.&lt;/p&gt;</string>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
<string>&lt;p&gt;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.&lt;/p&gt;</string>
<string>&lt;p&gt;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.&lt;/p&gt;</string>

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.

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>
@SlySven SlySven assigned Kebap and unassigned vadi2 and SlySven Nov 7, 2021
@SlySven SlySven merged commit 2b6a85f into Mudlet:development Nov 7, 2021
@SlySven SlySven deleted the Improve_iconsNotColouredStatusTextForSpecialExitsInExitsDialog branch November 7, 2021 19:09
SlySven added a commit to SlySven/Mudlet that referenced this pull request Dec 14, 2021
…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>
SlySven added a commit that referenced this pull request Dec 14, 2021
…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>
vadi2 pushed a commit to SlySven/Mudlet that referenced this pull request Jan 17, 2022
…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>
vadi2 pushed a commit to SlySven/Mudlet that referenced this pull request Jan 17, 2022
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted Not yet merged, but a welcome addition already! :-)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants