Refactor: clean up TBuffer text format aspects by SlySven · Pull Request #1840 · Mudlet/Mudlet

SlySven · 2018-07-22T01:24:49Z

Adds support for SGR Reverse (swap foreground and background colours) 7/27 for On/Off and SGR Overline 53/55 for On/Off

Remove unused cruft:

(void) TTextEdit::drawFrame(QPainter&, const QRect&)
(void) TTextEdit::updateLastLine
const QChar cLF & cSPACE in TBuffer (as it happens they are completely unused and redundant as the enum QChar::SpecialCharacter provides QChar::LineFeed and QChar::Space to provide the same constants)
(QTime) TBuffer::mTime
(void) TConsole::echoUserWindow(const QString&)
(QPoint) TBuffer::insert(QPoint&, const QString&, int, int, int, int, int, int, bool, bool, bool, bool)
(void) TConsole::printDebug(...) functionally the same as one type of (void) TConsole::print(...) just with a different order of arguments.

Convert #define constants TCHAR_BOLD etc. into a QFlag/enum TChar::AttributeFlags which is declared and capable of QFlag OR operations.

Refactor a number of methods that take lots of bools and ints as individual formatting options and colour components to take single TChar::AttributeFlags and one or two QColors instead.

Remove a large number of (int) colour value component values as member variables in TBuffer as they are not needed.

Convert highly repetitive intermediate methods to setBold, setItalics etc. to take a (combinations allowed) TChar::Attribute flag value instead.

Convert 2x3 int as colour components (r,g,b) in TColorTable defined in TTrigger class to a pair of QColors.

Remove unused QString argument from:

(void) mudlet::setLink(...)
(void) TConsole::setLink(...)

Remove unused QColor argument from:

(inline void) TTextEdit::drawCharacters(...)

Refactor arguments in:

(QString) TBuffer::bufferToHtml(QPoint P1, QPoint P2, bool allowedTimestamps, int spacePadding = 0)
to:
(QString) TBuffer::bufferToHtml(const bool showTimeStamp = false, const int row = -1, const int endColumn = -1, const int startColumn = 0, int spacePadding = 0)

Convert to const references some method arguments.

Add TBuffer::set[BF]gColor(...) overloads that take a QColor argument.
Add selection state methods select()/deselect()/isSelected() const methods to TChar class to hide/separate the selection process from the formatting effect. (TChar::Reverse tracks the ANSI SGR reverse colour attribute and its effect is EX-ORed with the (bool) TChar::mIsSelected flag).

Add a new tempAnsiColorTrigger lua function that, unlike tempColorTrigger uses the correct ANSI colors in the range 0-15 - although the original also handles the 256 colour range in the 16-255 correctly those first 16 values are miss-mapped and it is not possible to change them without breakage.

Adds 256-color support to Editor GUI for color triggers - and allows choosing the default (unmodified) fore or background colors to match one (the previous did not) and also allows one of the fore or background color to be ignored so only the other is considered. The ignored color case is
saved in the profile data and can exported but MAY not work in previous Mudlet versions which cannot handle the value used! It is also reported as an error to have a color trigger with both fore and background ignored in both the lua functions and in the GUI.

Also:
Fixed a code structure issue in TLuaInterpreter::debug() which would not work correctly if there was more than one value on the lua stack to print out.

This will close issues #477 and #703. It is a replacement for PR #1633 but it does NOT try to implement blink support (I found that the original could not be merged into the current codebase cleanly and that there was an issue in the blink code that I could not fix either in the original PR or in this one - it initially seemed to work but broke when the console text was scrolled and the mechanism of why was not clear).

Signed-off-by: Stephen Lyons slysven@virginmedia.com

Adds support for SGR Reverse (swap foreground and background colours) "7"/"27" for On/Off and SGR Overline "53"/"55" for On/Off Remove unused cruft: * (void) TTextEdit::drawFrame(QPainter&, const QRect&) * (void) TTextEdit::updateLastLine * const QChar cLF & cSPACE in TBuffer (as it happens they are completely unused and redundant as the enum QChar::SpecialCharacter provides QChar::LineFeed and QChar::Space to provide the same constants) * (QTime) TBuffer::mTime * (void) TConsole::echoUserWindow(const QString&) * (QPoint) TBuffer::insert(QPoint&, const QString&, int, int, int, int, int, int, bool, bool, bool, bool) * (void) TConsole::printDebug(...) functionally the same as one type of (void) TConsole::print(...) just with a different order of arguments. Convert #define constants TCHAR_BOLD etc. into a QFLag/enum TChar::AttributeFlags which is declared and capable of QFlag OR operations. Refactor a number of methods that take lots of bools and ints as individual formatting options and colour components to take single TChar::AttributeFlags and one or two QColors instead. Remove a large number of (int) colour value component values as member variables in TBuffer as they are not needed. Convert highly repetitive intermediate methods to setBold, setItalics etc. to take a (combinations allowed) TChar::Attribute flag value instead. Convert 2x3 int as colour components (r,g,b) in TColorTable defined in TTrigger class to a pair of QColors. Remove unused QString argument from: * (void) mudlet::setLink(...) * (void) TConsole::setLink(...) Remove unused QColor argument from: * (inline void) TTextEdit::drawCharacters(...) Refactor arguments in: *(QString) TBuffer::bufferToHtml(QPoint P1, QPoint P2, bool allowedTimestamps, int spacePadding = 0) to: (QString) TBuffer::bufferToHtml(const bool showTimeStamp = false, const int row = -1, const int endColumn = -1, const int startColumn = 0, int spacePadding = 0) Convert to const references some method arguments. Add TBuffer::set[BF]gColor(...) overloads that take a QColor argument. Add selection state methods select()/deselect()/isSelected() const methods to TChar class to hide/separate the selection process from the formatting effect. (TChar::Reverse tracks the ANSI SGR reverse colour attribute and its effect is EX-ORed with the (bool) TChar::mIsSelected flag). Add a new tempAnsiColorTrigger lua function that, unlike tempColorTrigger uses the correct ANSIcolors in the range 0-15 - although the original also handles the 256 colour range in the 16-255 correctly those first 16 values are miss-mapped and it is not possible to change them without breakage. Adds 256-color support to Editor GUI for color triggers - and allows choosing the default (unmodified) fore or background colors to match one (the previous did not) and also allows one of the fore or background color to be ignored so only the other is considered. The ignored color case is saved in the profile data and can exported but MAY not work in previous Mudlet versions which cannot handle the value used! It is also reported as an error to have a color trigger with both fore and background ignored in both the lua functions and in the GUI. Also: Fixed a code structure issue in TLuaInterpreter::debug() which would not work correctly if there was more than one value on the lua stack to print out. This will close issues Mudlet#477 and Mudlet#703. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

Nice, the code improvements are great, let's iron out the issues.

There are bugs with text selection - it is no longer accurate. Left is this PR, right is the current code: https://streamable.com/g34ge

Copy to html got broken

Creating a new colour trigger and changing out of it loses your selection: https://streamable.com/ihlfu

src/TTrigger.h

src/TTextEdit.cpp

vadi2 · 2018-07-22T06:54:07Z

src/TBuffer.cpp

-    for (int i = sub_start; i < length; i++) { //FIXME <=substart+sub_end muss nachsehen, ob wirklich noch teilbereiche gebraucht werden
-        if (text.at(i) == '\n') {
+    for (int i = sub_start; i < length; ++i) {
+        //FIXME <=substart+sub_end muss nachsehen, ob wirklich noch teilbereiche gebraucht werden


@Kebap can you translate this?

Sure! It is: Have to check, if partial areas are really still needed.

src/TBuffer.cpp

vadi2 · 2018-07-22T06:55:28Z

src/TBuffer.cpp

-        if (mEchoText) {
-            c.flags |= TCHAR_ECHO;
+    for (int i = sub_start; i < length; ++i) {
+        //FIXME <=substart+sub_end muss nachsehen, ob wirklich noch teilbereiche gebraucht werden


@Kebap Same here?

Yep. Same text as above. But it wasn't in originally ..?!

Copy-paste of boiler-plate IIRC.

Maybe unintended - but until I know the translation may be relevant here as well.

Translation see above

vadi2 · 2018-07-22T07:56:52Z

src/TTrigger.cpp

-                mColorPatternList.push_back(nullptr);
+            if (textAnsiBg == scmIgnored && textAnsiFg == scmIgnored) {
+                setError(QStringLiteral("<b><font color='blue'>%1</font></b>")
+                                 .arg(tr("Error: in item %1, no colors to match were set - at least <i>one</i> of the foreground or background must not be <b>ignored</b>.")


I don't think emphasis is necessary here.

Nested b tags won't produce

I believe bold is used for other messages in this position in the Editor. I will convert the inner one to italics - as it is picking out the control settings' values that are a problem.

Must not be ignored -- must be given?

Ignored is the significant word in the reported settings "Foreground color ignored" cannot be set at the same time as "Background color ignored" - because then it is not a Color trigger (or rather it would fire on EVERYTHING)!

src/TTrigger.cpp

vadi2 · 2018-07-22T08:06:00Z

src/dlgColorTrigger.cpp



-dlgColorTrigger::dlgColorTrigger(QWidget* pF, TTrigger* pT, int mode) : QDialog(pF), mpTrigger(pT), mMode(mode)
+dlgColorTrigger::dlgColorTrigger(QWidget* pF, TTrigger* pT, const bool isBackGround, const QString& title)


Old dialog:

New dialog:

The new one is more powerful, but a lot harder to use and less intuitive. We need to design this better.

Window title like: Mudlet color selection
Add first line before the three options with the current title as explanation.
Start all 3 options with words like Select or Select from

I haven't got ideas on how to improve this yet, but perhaps try reaching out to the community at https://ux.stackexchange.com @SlySven ?

This is a short-term transient dialogue - and saying what it is doing is consistent with other dialogues in the application - so identifying that it is setting the foreground or background for a particular trigger's item in the title is the right thing, IMHO.

It splits the colour range into the three groups because it is not feasible to provide a push-button for every one of the 256 colours as well as the default (the colour used after an SGR0 and ignore (do not pay any attention to this fore/background - only consider the other one) settings.

Ideally I'd like to convert the three QGroupBox's checkboxes into Radio buttons so it is more obvious that one and only one is active at a time. I've partially achieved that by greying out the top set of buttons when that groupbox is deselected and by greying out the text in the other two when they, similarly, deselected (although the latter happens automatically anyhow).

It is useful to report the colour "names" for the first sixteen - because they can be reallocated so that they do not appear as what they are called - but the name is still what the MUD will be thinking is being displayed.

The current proposal means that:

for the basic 16 colour triggers the actions are:

(only for an existing one where one of the other 240 colours was used) - click on the group-box checkbox

click to choose colour.

for a 6x6x6 RxGxB colour:

(unless an existing one where one of the other 216 was used) - click on the group-box checkbox

click/slide three times to required red/green/blue value

click on set button

for a grey scale colour:

(unless an existing grey scale one where one of the other 23 was used) - click on the group-box checkbox

click/slide to required grey value

click on set button

It is all very logical but it is just way too complicated and I can certainly bet that's what people say of it and give up trying to figure it out. It does need to be simpler to be used without any explanation like the one above was.

I have never seen instructions or explanations in window title. Some OS may display differently. Asking again to move this to the inside of the window

There are others within this application - so far I have picked out:

... QFileDialog::getExistingDirectory(nullptr, "Where do you want to save the package?"...

... QFileDialog::getExistingDirectory(this, tr("Where should Mudlet save log files?")...

... QFileDialog::getOpenFileName(this, tr("choose sound file")...

... QString fileName = QFileDialog::getOpenFileName(this, tr("Seclect Icon")...

titleText = tr(R"(Exits for room: "%1" [*])").arg(pR->name);

QString title = "Enter label text.";

newSymbol = QInputDialog::getItem(this, tr("Enter room symbol") ...

QString newWeightText = QInputDialog::getItem(this, tr("Enter room weight"), ...

What alternative should be put into the title bar for the dialogue if not something about what it is for? 😕

Oh, apparently macOs applications do not take title bar texts - at least for the QMessageBox class:

void QMessageBox::setWindowTitle(const QString &title)
This function shadows QWidget::setWindowTitle().
Sets the title of the message box to title. On macOS, the window title is ignored (as required by the macOS Guidelines).

This function was introduced in Qt 4.2.

Okay, I have added a (smaller text) explanation inside the dialogue I hope that helps...

vadi2 · 2018-07-22T08:06:54Z

src/dlgColorTrigger.cpp

+    // The ignore button means do not consider this fore or back ground (as per
+    // mIsBackground) for the colour trigger - only use the other one (which
+    // must be set)
+    connect(buttonBox->button(QDialogButtonBox::Ignore), SIGNAL(pressed()), this, SLOT(slot_resetColorClicked()));


Let's use the functor-based connectors here - QtCreator offers a right-click refactor option to update this.

Does it now? I hadn't seen that but if it is there that makes things simpler...

Will see if I can do that...

Ah, it is a feature added in Qt Creator 4.7.0...

... it does not handle code using QSignalMapper because Qt claims that is obsolete now that lambda functions can be used as slot methods. However I cannot quite get my head around that at present.

That feature has been around for quite a while in Qt Creator. What do we need QSignalMapper for? I don't think that is used in this connect.

I used it because it enables a single slot to be used for multiple signals from similar but distinguishable controls - i.e. you set up a mapping of the signal generators then the slot can tell which control sent the signal and can behave accordingly. In this situation it eliminated 15 of 16 previous slot methods that each handled a single colour selection button - combining all the signals into a larger single slot method.

BTW I upgraded Qt Creator and somehow ended up enabling the Clang code model or something and then the code lit up like a blooming 🎄 with warnings all over the place.

The biggest cause in the code I am currently munging is because TBuffer stores:

the text as a QStringList (effectively a QList<QList<QChar>>) which uses int indexes (which are obviously capable of being signed even though Qt does not permit negative indexes to be used in operator[#] or at(#))

the attributes in a std::deque<std::deque<TChar>> which uses std::deque::size_type a.k.a. unsigned long for indexes.

This means anywhere where the code has to index into both parts there are miss-matches between signed and unsigned integral types and also miss-matches between 32- and 64-bit integral types - it is a real mess for type safety...

But the above post is way off topic here...

src/ui/color_trigger.ui

SlySven · 2018-07-22T13:53:36Z

I think I need to put some tool-tips on some items.

SlySven · 2018-07-22T17:44:49Z

"There are bugs with text selection - it is no longer accurate. Left is this PR, right is the current code: https://streamable.com/g34ge" - humm, not sure about that but it is almost as if there is an off-by-one error in which character gets selected/deselected. I'll take a closer look.

"Copy to html got broken" - it is a mistake I made in the original try and copied across - a missing close \" in a  - putting the fix into place now.

"Creating a new colour trigger and changing out of it loses your selection: https://streamable.com/ihlfu" - check that that only happens if you leave it in the (invalid, but initial default) both fore and background colour ignored state before moving away - all the other cases should leave the underlying ANSI_COLOR_F{###}_B{###} text but that particular case - which generates an error message is deliberately cleared back to the empty QLineEdit case to clean up properly. (The fact that the autosave is so aggressive about trying to save partially entered data means that I was getting the error message popping up very frequently during testing - which wasn't an issue in the past... *sigh*)

Converted some `QObject::connect` calls to the new Qt5 compile time version. Removed "256" text from the renamed `./src/icons/mudlet_color_256.png` to `./src/icons/mudlet_color.png` icon. Fixed an off-by one error in HTML copying that missed the last character in the selection. Removed an unused `TTrigger*` argument from `dlgColorTrigger::setupBasicButtons(...)`. Removed an unused flag: * `(bool) TConsole::mSaveLayoutRequested Also spotted some dead code from abandoned attempt to support blinking, some reordering that a new version of Qt spotted as needed in the TBuffer and TChar constructor initialisation lists, and a operator precedence item that could be clarified with an addition pair of `(`...`)`s. A previous error in the prior PR that this one is attempting to replace had a problem in HTML generation that I reproduced here and which needed the same fix (a missing escaped `"` mark). An upgraded Qt Creator pointed out to me some initiliser list issues in `TBuffer` and `TConsole`; and some C-style casts in some font settings in the latter class. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2018-07-23T23:34:02Z

OK I think I have fixed the gross defects in this one now... 🙏

src/TBuffer.cpp

vadi2

Will do another QA round later tonight

src/TBuffer.cpp

src/mudlet.qrc

src/TBuffer.h

Also a comment revision. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 · 2018-07-25T05:02:07Z

Bugs seem gone now, but the colour selection dialog is still an issue.

there is a lot of information that feels overwhelming on the first try - it's not obvious that there are 3 separate things you can choose from, it feels like there is one giant thing right in your face and that takes time to figure out. What if we split basic and 256 into 2 tabs or a tool box so the basic is shown by default?
basic colour picking is confusing. I pick a colour, and then the only option is... "Ignore". The standard procedure is to press an "OK" button, that should be there in addition to "Ignore".
when I "disable" basic ansi colours, they just change colours to something else, which is confusing. They should all be uniform disabled buttons - I recommend stripping their colouring for this.
"Ignore" should be "Ignore this colour" imho for clarity.
playing around with sliders for the 216 colour selection is a cute minigame but picking a colour should not be a minigame - and figuring out which of the 216 colours a line is would be a nightmare. How about just going with a colour picker? That would be way simpler.

Remove a debugging output line that is not useful now and will be spammy. Modernise a triplet of QObject::connect(...) calls that will otherwise clash harder when merged into development after "upgrade-to-qt5-connect" PR has also been merged into the main branch. NOTE: If merging this PR into development after the "upgrade-to-qt5-connect" PR it will be necessary to resolve the referred to merge clash so that the development branch's: * (void) dlgTriggerEditor::slot_set_pattern_type_color(int type) gets replaced by/split into: * (QColor) dlgTriggerEditor::itemTypeColor(const int type) * (void) dlgTriggerEditor::setupPatternControls(const int type, dlgTriggerPatternEdit* pItem) Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

It had been previously noted that there was a use of bold html tags inside a block of test that was already emboldened so that the inner ones did not have any effect. Fix a coding error when the initial use of (void) dlgColorTrigger::setupBasicButtons(bool, bool isFirstTime = false) was not called with the second defaulted argument supplied with a non-default argument to include some run-once code. Also: * add some explanation/help text to the dlgColorTrigger dialog. * revise the text explaining the formula (232 + grey scale value 0..23) used for colours from the 24 grey-scale part of the 256 colour range. * add some tool-tips to parts of the dlgColorTrigger dialog. * add a generic static method to the mudlet class to provide a consistent and re-usable "HTML" wrapper around text - which will be particularly useful for tool-tip generation. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2018-07-26T14:45:42Z

I had a review of most of those points but I was running a compilation on my portable development system and I set make -j # too high and ran out of memory and seem to have lost the details in the web-browser on that machine when it hung itself. Will try and remember what I put, later on today...

SlySven · 2018-07-27T19:22:41Z

Okay I seem to have resolved the conflicts in dlgTriggerEditor.cpp with the development branch by using the web-interface here...

SlySven · 2018-08-01T16:04:09Z

Ah, it seems my response had not got totally lost and I can post it after all:

Responding to some of the points:
"2. basic colour picking is confusing. I pick a colour, and then the only option is... "Ignore". The standard procedure is to press an "OK" button, that should be there in addition to "Ignore"." - NO, pressing a basic colour button immediately selects THAT colour and closes the dialogue (as does the "Set to RGB value" and "Set to Greyscale value") that is what make it a one (or two in worst case) click selection for the Basic 16 colours - as it was before.

"3. when I "disable" basic ansi colours, they just change colours to something else, which is confusing. They should all be uniform disabled buttons - I recommend stripping their colouring for this." - they are physically disabled then and I was trying to go with the greyed out approach - I was not sure whether to complete remove all traces of colour and just leave varying levels of greyness but decided to leave a trace of colour to hint that that is there but not currently available (until that group box is enabled - in the same way that that text in the other group boxes gets greyed out) - but you seem to be suggesting either to remove all colour and leave a colour related grey or to go the whole hog and paint them all the same (I guess) mid-grey. I would note that the most obvious comparison we have both probably seen is the off-line members list on Discord for Guild with a total membership either below or above a threshold (a 1000 users I think it was) - I can't remember which but one of them show inactive members in a greyed out colour compared to what they have when active - like aliyss is slightly green because of their OS-??? role:

Which would be better - all different 16 shades of grey or all a uniform mid grey (or have I convinced you that the current implementation is not as bad as all that)?

"4. "Ignore" should be "Ignore this colour" imho for clarity." - If I make it more than a single word then it will probably have to elongate the button disproportionally compared to the others - its meaning, of course being "ignore the fore or background color setting for this item as a color trigger" - I think it would be reasonable to say something like this as a tooltip if that would help?

"5. playing around with sliders for the 216 colour selection is a cute minigame but picking a colour should not be a minigame - and figuring out which of the 216 colours a line is would be a nightmare. How about just going with a colour picker? That would be way simpler." - I really do not want to have to populate a dialogue with over 250 buttons - and without using a QSignalMapper the code would be horrendous (with it would be handle-able but still not pretty with that many repetitions of a couple of lines of code). At least the way I have done it does give a bit of an explanation of how ANSI 256-color mode works - what we may have to provide is a way that users can identify what colour a particular grapheme on-screen has been drawn in so that they can select the correct ANSI colour number to match it against - this is especially true if they have redefined one of the basic colours. TBH I suspect a user may have a great deal of difficulty picking out which of the 256 colours they are encountering - especially if it is one that occurs at more than one point in the 256 colour palette.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap · 2018-08-02T08:55:07Z

How about ditching the complicated 3x color chooser interface altogether and returning to the "click my color" way of designing that window? Here is an overview of all 256 xterm colors. This can surely be shrunk in size a little more, to result in a nice interface like this w3schools color picker example, which already incorporates 127 colors in a nice easy-to-use and easy-to-understand interface. Add their preview section, where you could also put the names of the selected color, so the actual chooser can shrink even more in size.

Man I really need to set up my Qt fu so I can show practical example instead of mere ideas in text form. :)

vadi2 · 2018-08-02T09:00:48Z

That sounds better, but the user still has to figure out which colour do they need to use. I think we should remove the guesswork entirely and go with a https://docs.gimp.org/en/gimp-tool-color-picker.html in complement to the above idea.

SlySven · 2018-08-02T13:17:58Z

My objection though still applies - that means a dialogue with over 250 buttons and that is going to be horrible to make - especially without using the, now I understand, QSignalMapper class. 🤢

Ideally for the 6x6x6 colour range we could have a 3D picture each colour in the right position with six buttons along each edge to allow the user to select the three needed for that range but we have to work in a 2D space (the user's screen)!

I am taking a quick look at #1579 (merging in the current development branch and fixing a sneaky little code sequence error that I am now seeing with a different compiler/OS/PC combination) - to see if it would be feasible to report the Fore/Back ground colour numbers for each character on the analysed line - which would help the user get the right ones I suppose.

SlySven · 2018-08-02T13:19:22Z

I mean this may not be the most pretty UI but it does what it says on the ~~tin~~ title line.

Kebap · 2018-10-28T01:50:30Z

Well, what do you know: There actually is a color-picker already included in Mudlet mapper's right click menu "color" - "define new color" - "pick screen color"

SlySven · 2018-12-15T17:40:12Z

This will need some reworking to accommodate subsequent changes to the code-base - even within the TBuffer class! 😮

SlySven · 2019-01-13T01:07:56Z

That colour picker is not going to be usable I think - the right hand side is based effectively on the user choosing from a 256x256x256 RxGxB colour space and the left on some "basic colours" but it is not obvious to me that we can pre-populate the latter and the former does not represent the 16 + 6x6x6 + 24 ranges that the ANSI-256 colour range covers...

vadi2 · 2019-01-13T03:39:54Z

OK, what about #1840 (comment) ?

This follows a suggestion from a peer in the review process. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Conflicts resolved in: * src/Host.cpp * src/Host.h * src/TBuffer.cpp * src/TBuffer.h * src/TConsole.cpp * src/TLuaInterpreter.cpp * src/TLuaInterpreter.h * src/TTextEdit.cpp * src/TTextEdit.h * src/XMLexport.h * src/dlgTriggerEditor.cpp * src/mudlet.cpp * src/mudlet.h Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

… > 15 Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap

Commenting on some UI texts

src/dlgColorTrigger.cpp

Kebap · 2019-01-27T19:54:11Z

src/dlgColorTrigger.cpp

-        connect(pButton, SIGNAL(clicked()), mSignalMapper, SLOT(map()));
-        mSignalMapper->setMapping(pButton, ansiColor);
-    }
+    // TODO: Eliminate use of QSignalMapper and use a lambda function


How about an issue here?

TBH I ~~haven't got a clue~~ am rather uncertain as to how it would be done. Until I understand that it can be done / and how, I would not want go beyond leaving that particular reminder.

I do understand your desire to migrate as many of these as possible to become GitHub issues.

src/dlgColorTrigger.cpp

Kebap · 2019-01-27T21:31:36Z

src/ui/color_trigger.ui

     <property name="text">
-      <string comment="Ensure that &quot;Default&quot;, &quot;Ignore&quot; and &quot;Cancel&quot; in this instruction are the same as used for the controls elsewhere on this dialog.">&lt;small&gt;Choose:&lt;ul&gt;&lt;li&gt;one of the three ranges below then follow the instructions to select a color from that part of the 256 colors supported&lt;/li&gt;
+      <string comment="Ensure that &quot;Default&quot;, &quot;Ignore&quot; and &quot;Cancel&quot; in this instruction are the same as used for the controls elsewhere on this dialog.">&lt;small&gt;Choose:&lt;ul&gt;&lt;li&gt;one of the basic 16 colors below&lt;/li&gt;
+&lt;li&gt;click the &lt;i&gt;more&lt;/i&gt; button to get access to other colors in the 256-color set, then follow the instructions to select a color from that part of the 256 colors supported; if such a color is already in use then that part will already be showing&lt;/li&gt;


Choose one of the basic 16 colors below or click More colors to select from all 256 ANSI colors.

KISS

Yes, there will be instructions, but we don't need to be instructed to follow instructions.

If a users already did this before and selected an advanced color, they will understand their visibility.

I feel the "Default" and "Ignore" options need more instructions, but not sure if this is the correct place at all, or only after having clicked them.

The "Cancel" button sould be fairly self-explanatory, right?

Too long texts can be overwhelming for regular users and create lots of workload for translation as well.

Above comment also references the next few lines which are not shown in github web preview

That was why I broke all the different options down into separate lines to explain exactly what each one did - some of them {"Default", "Ignore", "Cancel" or one of the basic 16 colours} immediately take effect and close the dialogue; others require further interactions. The mechanism for the 6x6x6 RGB and the 24 Grey range might take a moment or more to understand - and that the "Set to RGB/Grayscale" are what commits a colour only after the user has dialled in the components needed.

See above (the auto-numbering here does not handle skipped numbers well!)

I didn't want to have to code for a only show this the first time approach.

They are both new options in this area and the ignore is new to the whole application - the tooltips do give an explanation and can provide that before they are clicked on. As it is the effect of the default one can be a bit subtle!

Yeah, but it could be be interpreted to mean - remove the effect of this element (foreground or background) from the overall colour trigger - in fact that action is assigned to the ignore action but as there is scope for confusion it is best to say it explicitly.

src/TBuffer.cpp

src/TMap.cpp

src/TTrigger.cpp

src/XMLimport.cpp

src/TBuffer.h

src/TConsole.cpp

src/mudlet.qrc

SlySven · 2019-01-27T23:02:50Z

src/ui/color_trigger.ui

     <property name="text">
-      <string comment="Ensure that &quot;Default&quot;, &quot;Ignore&quot; and &quot;Cancel&quot; in this instruction are the same as used for the controls elsewhere on this dialog.">&lt;small&gt;Choose:&lt;ul&gt;&lt;li&gt;one of the three ranges below then follow the instructions to select a color from that part of the 256 colors supported&lt;/li&gt;
+      <string comment="Ensure that &quot;Default&quot;, &quot;Ignore&quot; and &quot;Cancel&quot; in this instruction are the same as used for the controls elsewhere on this dialog.">&lt;small&gt;Choose:&lt;ul&gt;&lt;li&gt;one of the basic 16 colors below&lt;/li&gt;
+&lt;li&gt;click the &lt;i&gt;more&lt;/i&gt; button to get access to other colors in the 256-color set, then follow the instructions to select a color from that part of the 256 colors supported; if such a color is already in use then that part will already be showing&lt;/li&gt;


That was why I broke all the different options down into separate lines to explain exactly what each one did - some of them {"Default", "Ignore", "Cancel" or one of the basic 16 colours} immediately take effect and close the dialogue; others require further interactions. The mechanism for the 6x6x6 RGB and the 24 Grey range might take a moment or more to understand - and that the "Set to RGB/Grayscale" are what commits a colour only after the user has dialled in the components needed.

See above (the auto-numbering here does not handle skipped numbers well!)

I didn't want to have to code for a only show this the first time approach.

They are both new options in this area and the ignore is new to the whole application - the tooltips do give an explanation and can provide that before they are clicked on. As it is the effect of the default one can be a bit subtle!

Yeah, but it could be be interpreted to mean - remove the effect of this element (foreground or background) from the overall colour trigger - in fact that action is assigned to the ignore action but as there is scope for confusion it is best to say it explicitly.

src/dlgColorTrigger.cpp

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

…er_Redux

vadi2

Looks amazing, thanks for this huge amount of work!

Perhaps the really bright colouring is not necessary? We should stick with what we've got, it works well.

src/TConsole.cpp

Fix a spelling mistake in a comment. Co-Authored-By: SlySven <slysven@virginmedia.com>

SlySven · 2019-01-28T20:04:42Z

Perhaps the really bright colouring is not necessary? We should stick with what we've got, it works well.

No it doesn't - if you are using a dark theme - see #2132 !

vadi2 · 2019-01-29T07:00:10Z

Most people aren't, and this definitely is not the solution to it either. Aesthetically, it looks really off...

SlySven added enhancement labels Jul 22, 2018

SlySven assigned vadi2 Jul 22, 2018

SlySven requested review from a team July 22, 2018 01:24

SlySven mentioned this pull request Jul 22, 2018

Refactor TBuffer class #1633

Closed

vadi2 reviewed Jul 22, 2018

View reviewed changes

vadi2 reviewed Jul 24, 2018

View reviewed changes

src/TBuffer.cpp Outdated Show resolved Hide resolved

vadi2 reviewed Jul 24, 2018

View reviewed changes

src/TBuffer.cpp Outdated Show resolved Hide resolved

src/TBuffer.cpp Outdated Show resolved Hide resolved

src/mudlet.qrc Show resolved Hide resolved

SlySven commented Jul 24, 2018

View reviewed changes

src/TBuffer.h Outdated Show resolved Hide resolved

Revise: a couple of further small peer-review points

3086dae

Also a comment revision. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 mentioned this pull request Jul 25, 2018

Upgrade a few classes to newer Qt connect style #1846

Merged

SlySven added 2 commits July 26, 2018 01:53

Merge branch 'development' into Refactor_TBuffer_Redux

02b2134

Revise: address a couple of peer-review points

034d8f5

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap changed the title ~~Refactor: clean up TBuffer text format aspects~~ WIP. Refactor: clean up TBuffer text format aspects Sep 10, 2018

Kebap changed the title ~~WIP. Refactor: clean up TBuffer text format aspects~~ Refactor: clean up TBuffer text format aspects Oct 29, 2018

SlySven added the 0 - Backlog label Dec 15, 2018

Kebap removed the 0 - Work in progress label Jan 24, 2019

Revise: change colour trigger UI to hide colours 16-255 by default

b33c06e

This follows a suggestion from a peer in the review process. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven mentioned this pull request Jan 26, 2019

Triggers text impossible to read in a darker QT5 theme #2132

Closed

SlySven added 3 commits January 27, 2019 19:16

Update: add Wiki documentation link comment

19ac4dd

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Bugfix: ensure "More" colours button is initially depressed on colors…

fe463c1

… > 15 Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap reviewed Jan 27, 2019

View reviewed changes

SlySven commented Jan 28, 2019

View reviewed changes

Revise: some tweaks brought about by peer review

fbdf1fc

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven mentioned this pull request Jan 28, 2019

Color oddities with Cleft of Dimensions #1958

Closed

Merge remote-tracking branch 'origin/development' into Refactor_TBuff…

c5a41c2

…er_Redux

vadi2 approved these changes Jan 28, 2019

View reviewed changes

src/TConsole.cpp Outdated Show resolved Hide resolved

Update src/TConsole.cpp

0201ec8

Fix a spelling mistake in a comment. Co-Authored-By: SlySven <slysven@virginmedia.com>

SlySven merged commit 160d30a into Mudlet:development Jan 28, 2019

SlySven deleted the Refactor_TBuffer_Redux branch January 28, 2019 23:24

This was referenced Feb 1, 2019

Trigger Colorizer hides affected text #2273

Closed

Update function list for autocompletion #2283

Merged

setTextFormat colours reversed #2315

Closed

SlySven mentioned this pull request Oct 2, 2020

tempAnsiColorTrigger doesn't work #4109

Closed

Kebap mentioned this pull request Nov 4, 2020

Translate remaining German comments #4276

Merged

SlySven mentioned this pull request Jan 18, 2021

BugFix: improve handling of TConsole::mSystemMessageBgColor #4650

Merged



		dlgColorTrigger::dlgColorTrigger(QWidget* pF, TTrigger* pT, int mode) : QDialog(pF), mpTrigger(pT), mMode(mode)
		dlgColorTrigger::dlgColorTrigger(QWidget* pF, TTrigger* pT, const bool isBackGround, const QString& title)

Uh oh!

Conversation

SlySven commented Jul 22, 2018

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

SlySven commented Jul 22, 2018

Uh oh!

SlySven commented Jul 22, 2018

Uh oh!

SlySven commented Jul 23, 2018

Uh oh!

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SlySven commented Aug 1, 2018 •

edited

Loading

Kebap commented Aug 2, 2018 •

edited

Loading

SlySven commented Aug 2, 2018 •

edited

Loading