Add way to color room characters by Delwing · Pull Request #4573 · Mudlet/Mudlet

Delwing · 2021-01-02T22:50:02Z

Brief overview of PR changes/additions

Add way to change color of room symbol through map context menu and Lua API

Map symbol picking is no longer modal as well.

Motivation for adding to Mudlet

More customization for maps of course!

Other info (issues closed, discussion etc)

closes #3988

add-deployment-links · 2021-01-02T22:50:36Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

linux: https://make.mudlet.org/snapshots/9960d7/Mudlet-4.10.1-testing-pr4573-6f3c1462-linux-x64.AppImage.tar
osx: (temporarily out of order)
windows: https://make.mudlet.org/snapshots/ccf06a/Mudlet-4.10.1-testing-pr4573-6f3c1462-windows.zip

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

SlySven

I will need to build this before I can give a more fuller review...

SlySven · 2021-01-02T23:39:39Z

src/TLuaInterpreter.cpp

+    }
+    int r = lua_tonumber(L, 2);
+    if (r < 0 || r > 255) {
+        lua_pushfstring(L, "setRoomCharColor: bad argument #2 type (red component value %d out of range (0 to 255)", r);


These are a nil + errormessage type case (value out of range rather than being the wrong type). It shouldn't throw a lua_error(...) IMO.

This is equally invalid as putting string actually.

I agree, it's an error to misuse the API. There's no case where this would ever be valid, so it can't be nil+error

SlySven · 2021-01-02T23:40:59Z

src/TMap.cpp

            }
            ofs << oldCharacterCode;
        }
+        if(mSaveVersion >= 21) {


Like wot CodeFactor sez...

Nutin.. :) Corrected.

SlySven · 2021-01-02T23:51:13Z

src/TMap.cpp

+        }
+    }
+    switch (env) {
+    case 1:


If it were me I'd collapse these all down to single line cases - with the elements all padded out to 4-space columns (like the tab key will do in QT Creator if it is set to use only 4 spaces for indentation) like this:

switch (env) { case 1: color = mpHost->mRed_2; break; case 2: color = mpHost->mGreen_2; break; case 3: color = mpHost->mYellow_2; break; case 4: color = mpHost->mBlue_2; break; case 5: color = mpHost->mMagenta_2; break; case 6: color = mpHost->mCyan_2; break; case 7: color = mpHost->mWhite_2; break; case 8: color = mpHost->mBlack_2; break; case 9: color = mpHost->mLightRed_2; break; case 10: color = mpHost->mLightGreen_2; break; case 11: color = mpHost->mLightYellow_2; break; case 12: color = mpHost->mLightBlue_2; break; case 13: color = mpHost->mLightMagenta_2; break; case 14: color = mpHost->mLightCyan_2; break; case 15: color = mpHost->mLightWhite_2; break; case 16: color = mpHost->mLightBlack_2; break; default: //user defined room color if (!customEnvColors.contains(env)) { if (16 < env && env < 232) { quint8 base = env - 16; quint8 r = base / 36; quint8 g = (base - (r * 36)) / 6; quint8 b = (base - (r * 36)) - (g * 6); r = r * 51; g = g * 51; b = b * 51; color = QColor(r, g, b, 255); } else if (231 < env && env < 256) { quint8 k = ((env - 232) * 10) + 8; color = QColor(k, k, k, 255); } break; } color = customEnvColors[env]; }

SlySven · 2021-01-03T00:02:01Z

src/dlgRoomSymbol.cpp

+{
+    if (mpSymbols.size() <= 1) {
+        return lineEdit_roomSymbol->text();
+    } else {


ℹ️ There is no need for the else { ... } as the return on the line before will mean that the formerly else code can be unindented one level - this is the coding anti-pattern that several linters call else-after-return... https://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html

This one slipped. Corrected.

SlySven · 2021-01-03T00:10:48Z

src/ui/room_symbol.ui

+  <property name="windowTitle">
+   <string>Room symbol</string>
+  </property>
+  <layout class="QGridLayout" name="gridLayout">


I think you have some redundancy here - this QGridLayout only has one member..

Leftover after playing around with layout. Removed.

vadi2 · 2021-01-03T08:33:39Z

I added Delwing#6 which improves on the design of the dialog:

…layout Improve room symbol dialog design

vadi2

This is a big piece of work, good job, you can be proud :)

vadi2 · 2021-01-03T08:34:34Z

src/ui/room_symbol.ui

+            <item>
+             <widget class="QLineEdit" name="lineEdit_roomSymbol">
+              <property name="inputMethodHints">
+               <set>Qt::ImhUrlCharactersOnly</set>


What's the reason for enabling this?

Some miscclick I guess.

There is a little arrow button you can use to unset a property, instead of having it set to none

vadi2 · 2021-01-03T08:38:52Z

src/T2DMap.cpp

-                }
-            }
-        }
+        QString inputSymbol = QString();


Suggested change

QString inputSymbol = QString();

QString inputSymbol;

Actually remove whole line. Not needed.

vadi2 · 2021-01-03T08:40:06Z

src/TRoom.h

    qreal max_x;
    qreal max_y;
    QString mSymbol;
+    QColor mSymbolColor = nullptr;


Just QColor mSymbolColor; is enough

vadi2 · 2021-01-03T08:41:39Z

src/T2DMap.cpp

+        auto symbolDialog = new dlgRoomSymbol(mpHost, this);
+        symbolDialog->init(usedSymbols, roomPtrsSet);
+        symbolDialog->show();
+        symbolDialog->raise();


You also want to set the Qt::WA_DeleteOnClose property so it gets deleted when closed to free the memory.

vadi2 · 2021-01-03T08:42:50Z

src/T2DMap.cpp

-            }
-        }
+        QString inputSymbol = QString();
+        auto symbolDialog = new dlgRoomSymbol(mpHost, this);


The new design allows spawning multiple dialogs at once, even on the same room, maybe not the best idea?

I can't figure out how custom lines dialog is done. It blocks map interactions, but does not block main window... This could be applied.
In theory it has exec call which make this window modal... but when I do set modality on symbol dialog it blocks main window as well.

Remember the fix for multiple About buttons - same here, store a pointer and if the dialog is open, don't show another one.

vadi2 · 2021-01-03T08:54:37Z

src/dlgRoomSymbol.cpp

+#include <QPainter>
+#include "post_guard.h"
+
+dlgRoomSymbol::dlgRoomSymbol(Host* pHost, QWidget* pF) : QDialog(pF), mpHost(pHost)


Sidenote, we ought to give pF a much better name everywhere in the project.

Put a new issue for future reference, so it does not get buried with this PR merged: #6375

vadi2 · 2021-01-03T08:57:08Z

src/dlgRoomSymbol.cpp

+    label_preview->setFont(font);
+}
+
+void dlgRoomSymbol::init(QHash<QString, unsigned int>& pSymbols, QSet<TRoom*>& pRooms)


Sidenote, I know this came from existing code, but unsigned here is an anti-pattern according to modern C++ guidelines: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-nonnegative

We should avoid it in the future and remove its use here. The more you know!

See also Expects: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#i6-prefer-expects-for-expressing-preconditions

Put a new issue for future reference, so it does not get buried with this PR merged: #6376

vadi2 · 2021-01-03T08:58:45Z

src/dlgRoomSymbol.cpp

+
+void dlgRoomSymbol::initInstructionLabel()
+{
+    if (mpSymbols.size() == 0) {


Suggested change

if (mpSymbols.size() == 0) {

if (mpSymbols.empty()) {

src/dlgRoomSymbol.cpp

vadi2 · 2021-01-03T09:01:28Z

src/dlgRoomSymbol.h

+#define MUDLET_DLGROOMSYMBOL_H
+
+/***************************************************************************
+ *   Copyright (C) 2020 by Piotr Wilczynski - delwing@gmail.com            *


2021 probably, and in the other file?

Given the development period concerned it should probably be 2020-2021...

Is that really the case, or does only the merge time matter?

vadi2

This is great 👍

SlySven · 2021-01-03T17:39:55Z

When there is multiple rooms selected with different symbols there is a count missing (it is stuck at 0) for how many rooms are in the selection:

When there are multiple rooms with the SAME symbol the text does not seem to be constructed so that it reports that one is working with more than one room - I don't know if that is a defect from my original coding...!

BTW If I set the colour of the rooms for those three death-trap rooms differently then the colour used in the sample is of only one of them:

It isn't obvious on first use that one has to click on the button (?) to change the colour - and the tool tip has a typo (it also needs to be fed the selection size - an %n - but I haven't checked the code yet, only the run-time behaviour) - it might be better for it to have some contrasting text on it saying "Change color...":

SlySven · 2021-01-03T17:56:20Z

You might want to consider how you handle the case where only the specified font is to be used and one or more of the symbol glyphs cannot be shown in that font. The two selected before the symbol dialogue was opened are ones that would have the warning triangle in the symbol usage dialogue - but they are not in the map symbol font so if you use it for your sample you won't get anything useful - in the map painting code and the symbol usage code I arranged for that to be handled by inserting the replacement character instead...

SlySven · 2021-01-03T17:58:07Z

👍 This is an interesting development and it shows potential...

vadi2 · 2021-01-03T18:17:10Z

I think that both of us forgot about it shows that this font symbol fallback system is too complicated...

SlySven · 2021-01-03T20:07:32Z

It is NOT a fallback system - it is a requirement for a MUD that provides a specific font for its maps - so that the user can stay within that font when they are editing it...

... the fall-back as you describe it is to use all the fonts on the system (which IS the default) but if as a map maker you are using odd glyphs from a particular font (or even a custom one - and I have done stuff in that area myself!) that you are shipping as part of a Mud specific UI you will be trying to see that everyone gets the same map symbols from the same font.

Delwing · 2021-01-04T01:27:28Z

Used parts of logic for creating pixmaps of symbols.
Corrected room counts issues (+ text labels selection)
Fixed default color behavior (after some fixes initial default was always black)

It seems like PR Mudlet#4573 introduced a bogus use of pointer access to a `QColor` whereas the correct methodology is `QColor::isValid()` - a default constructed `QColor` is itself equivalent to an RGBA specified colour of `QColor(0,0,0,0)` and that will fail in that test. This PR fixes things so that (instead of a `nullptr` test) is used. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…4679) It seems like PR #4573 introduced a bogus use of pointer access to a `QColor` whereas the correct methodology is `QColor::isValid()` - a default constructed `QColor` is itself equivalent to an RGBA specified colour of `QColor(0,0,0,0)` and that will fail in that test. This PR fixes things so that (instead of a `nullptr` test) is used. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…gain A test for this pointer being a `nullptr` is what enables a new dialogue to be made in the 2D mapper - by not initialising it in Mudlet#4573 a random non-null value is found so the dialogue and thus the functionality can be virtually impossible to use. As it is a pointer I have taken the liberty to introduce a `p` into the initial part of the name - after the `m` to indicate it is a class member. As the defect is a regression that causes loss of functionality I am rating this as a **high** priority bug(fix) even though it does not induce any fatal termination of the application. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…gain (#4687) A test for this pointer being a `nullptr` is what enables a new dialogue to be made in the 2D mapper - by not initialising it in #4573 a random non-null value is found so the dialogue and thus the functionality can be virtually impossible to use. As it is a pointer I have taken the liberty to introduce a `p` into the initial part of the name - after the `m` to indicate it is a class member. As the defect is a regression that causes loss of functionality I am rating this as a **high** priority bug(fix) even though it does not induce any fatal termination of the application. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…gain (Mudlet#4687) A test for this pointer being a `nullptr` is what enables a new dialogue to be made in the 2D mapper - by not initialising it in Mudlet#4573 a random non-null value is found so the dialogue and thus the functionality can be virtually impossible to use. As it is a pointer I have taken the liberty to introduce a `p` into the initial part of the name - after the `m` to indicate it is a class member. As the defect is a regression that causes loss of functionality I am rating this as a **high** priority bug(fix) even though it does not induce any fatal termination of the application. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 · 2021-08-05T06:23:04Z

This never made it into the docs 😮

* add way to color room characters * accept new translations * fix qmake build * addSymbolToPixmapCache improve * cr corrections * Modernize design * remove accidental hints * remove not needed variable * minor fixes * room not exists will not throw error in room char color functions * remove properties with auto suggestion * remove not neede assignment * prevent spawning multiple symbol char dialogs at once * fix room symbol selection issues Co-authored-by: Piotr Wilczynski <piotr.wilczynski@bisnode.com> Co-authored-by: Vadim Peretokin <vperetokin@gmail.com>

…udlet#4679) It seems like PR Mudlet#4573 introduced a bogus use of pointer access to a `QColor` whereas the correct methodology is `QColor::isValid()` - a default constructed `QColor` is itself equivalent to an RGBA specified colour of `QColor(0,0,0,0)` and that will fail in that test. This PR fixes things so that (instead of a `nullptr` test) is used. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…gain (Mudlet#4687) A test for this pointer being a `nullptr` is what enables a new dialogue to be made in the 2D mapper - by not initialising it in Mudlet#4573 a random non-null value is found so the dialogue and thus the functionality can be virtually impossible to use. As it is a pointer I have taken the liberty to introduce a `p` into the initial part of the name - after the `m` to indicate it is a class member. As the defect is a regression that causes loss of functionality I am rating this as a **high** priority bug(fix) even though it does not induce any fatal termination of the application. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

add way to color room characters

bf6e589

Delwing requested a review from a team as a code owner January 2, 2021 22:50

Delwing requested review from a team January 2, 2021 22:50

Piotr Wilczynski added 3 commits January 2, 2021 23:53

accept new translations

a89fd7c

fix qmake build

4513ca3

addSymbolToPixmapCache improve

152a097

SlySven reviewed Jan 3, 2021

View reviewed changes

Piotr Wilczynski and others added 2 commits January 3, 2021 01:20

cr corrections

1a367f2

Modernize design

8f651dc

Merge pull request #6 from vadi2/room-symbol-color-selection-improve-…

cea535c

…layout Improve room symbol dialog design

vadi2 reviewed Jan 3, 2021

View reviewed changes

vadi2 changed the title ~~add way to color room characters~~ Add way to color room characters Jan 3, 2021

Piotr Wilczynski added 7 commits January 3, 2021 10:19

remove accidental hints

95f70bf

remove not needed variable

5798169

minor fixes

038b227

room not exists will not throw error in room char color functions

784eff7

remove properties with auto suggestion

26aa043

remove not neede assignment

b566718

prevent spawning multiple symbol char dialogs at once

c1d3c66

vadi2 approved these changes Jan 3, 2021

View reviewed changes

fix room symbol selection issues

6f3c146

vadi2 merged commit 42f6c8c into Mudlet:development Jan 5, 2021

SlySven mentioned this pull request Jan 25, 2021

BugFix: correct mis-use of nullptr to denote no room symbol colour #4679

Merged

SlySven mentioned this pull request Jan 26, 2021

BugFix: initialise mDlgRoomSymbol so that settng room symbols works again #4687

Merged

vadi2 added the needs documentation This pull request changes things the players would use/see and thus needs an update in the manual label Aug 5, 2021

Delwing removed the needs documentation This pull request changes things the players would use/see and thus needs an update in the manual label Sep 12, 2021

This was referenced Oct 20, 2022

make pF variable more readable #6375

Closed

Remove use of "unsigned" type #6376

Open

Remove obsolete variable "previewColor" in room symbol dialog #6377

Closed

Infrastructure: Remove obsolete "previewColor" code #6378

Closed

Kebap mentioned this pull request Sep 20, 2025

Look at the Symbol text "... enter a space to clear ...". #8255

Closed

Uh oh!

Conversation

Delwing commented Jan 2, 2021

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

Uh oh!

add-deployment-links bot commented Jan 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

SlySven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Jan 3, 2021

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

add-deployment-links bot commented Jan 2, 2021 •

edited

Loading