Closed
Conversation
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.
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.
Removed unneeded conditional #include of update.h in mudlet.cpp as it is
already in mudlet.h.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Fix the mis-handling of colour trigger numbers (because the previously used
numbers did not match the "ANSI" colour codes) in the first commit; as part
of this I have replace the pattern (matched with the regexp
"FG(\d+)BG(\d+)" used to encode the ANSI colours used for the textual
representation of a colour pattern item in a TTrigger with:
"^ANSI_COLORS_F{(\d+|DEFAULT|IGNORE)}_B{(\d+|DEFAULT|IGNORE)}$"
The processing of this pattern is abstracted to:
* (QString) TTrigger::createColorPatternText(const int, const int)
* (void) TTrigger::decodeColorPatternText(const QString&, int&, int&)
Extends the colour trigger dialog to better handle the existing 16 ANSI
colours but now also handles the remainder of the 256 ANSI colour code
numbers and provides a means to use the "default" fore or background colour
(what ever the user has set it to) and also to ignore either the fore or
background colours.
Refactored the mapping of the colour buttons in the dlgColorTrigger
form/dialog to use a QSignalMapper which removed the need for 16 almost
identical methods (one for each button) and added text to them to describe
the colours they are supposed to represent even if they have been
completely remapped to other colours by the user - or, in the future if
we include support for OSC (Operating System Command) Server setting of
colour by use of \033]4;<colour number>;<colour specification>; ... ;\007
to set the colour with the given number to the X11 name, or a 6 hex digit
RRGGBB string; with \003]104;<colour number>;...;\007 to reset them to
"standard". These come from the XTerm terminal emulator:
http://invisible-island.net/xterm/ctlseqs/ctlseqs.html#h2-Operating-System-Commands
Also added a couple of helpers to set the background colour on a QButton to
the given colour and the foreground to a contrasting colour of either
black or white given the QColor::lightness() of that colour:
* (static QString) dlgTriggerEditor::generateButtonStyleSheet(const QColor&)
* (static QColor) dlgTriggerEditor::parseButtonStyleSheetColors(
const QString&, const bool isToGetForeground = false)
A further enhancement means that editing the above colour pattern string
when the trigger item type is set to NOT be a colour trigger is picked up
and converted to the correct colour in the dialogue if the colour trigger
type is re-selected later.
To support the full range of 256 ANSI colours without breaking existing
scripts it is necessary to provide a new:
(int) TLuaInterpreter::tempAnsiColorTrigger(lua_State*)
that uses a different mapping of numbers in the range 0 to 15 and adds
-1 for ignore one of either the fore or background colours (both is a run-
time error!)
Add a (QColor) Host::getAnsiColor(const int, const bool isBackground) const
to return the correct QColor value from the Host instance given an ANSI
(0 to 255) colour code number but also supports -1 for the default fore or
background colour for that Host instance.
Also:
* rename (void) TTrigger::set[BF]gColor(QColor&) to:
(void) TTrigger::setColorizer[BF]gColorsetFgColor(QColor&) because it
helps to link them to the colouring of matching text and separates them
from methods relating to colour pattern matching.
* Remove a memory leak where TColorTable structs were not being deleted
when they were being removed from the:
(QList<TColorTable*>) TTrigger::mColorPatternList
in the TTrigger destructor and when it was being updated in
(bool) TTrigger::setRegexCodeList(QStringList, QList<int>)
Remove:
* (bool) TTrigger::mColorTrigger[BF]g as they are not needed and can be
derived from the related (int) TTrigger::mColorTrigger[BF]gAnsi being
not equal to the new TTrigger::scmIgnored constant value.
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven
commented
May 1, 2018
| bgColorR = mBgColorR; | ||
| bgColorG = mBgColorG; | ||
| bgColorB = mBgColorB; | ||
| // TODO go to using mFGColor instead of its components |
Member
Author
There was a problem hiding this comment.
Blast - forgot to take that reminder from half way through out...
Member
|
Thanks, I will review this. Could you also measure the performance impact of these changes? Speed of the rendering is really crucial to Mudlet and I'd like to be sure it's still top of the line after changes to the rendering code. |
ahmedcharles
reviewed
May 1, 2018
Contributor
ahmedcharles
left a comment
There was a problem hiding this comment.
I took a look and I don't see anything obviously wrong. :)
vadi2
reviewed
May 1, 2018
Member
vadi2
left a comment
There was a problem hiding this comment.
Very nice work, code looks a lot cleaner. Too much to review at once so I'll have more reviews later.
| (buffer[y][x].flags & TCHAR_STRIKEOUT)); | ||
|
|
||
| for (int total = P2.x(); x < total; ++x) { | ||
| // This is rather inefficient as s is only ever one QChar long |
Member
There was a problem hiding this comment.
What about a QStringView here? It's available in 5.10 which we can't use yet, but if we could, it would be a good fit yeah?
src/TBuffer.cpp
Outdated
| if (x < 0 || x >= static_cast<int>(buffer[y].size())) { | ||
| return; | ||
| } | ||
| // FIXME: RISK OF EXECEPTION getLastLineNumber() returns zero (not -1) if |
src/TBuffer.cpp
Outdated
| } | ||
| // FIXME: RISK OF EXECEPTION getLastLineNumber() returns zero (not -1) if | ||
| // the buffer is empty, so y can never be less than zero here - however that | ||
| // will cause an exeption with std::deque::at(size_t) - previously |
| void TBuffer::appendBuffer(const TBuffer& chunk) | ||
| { | ||
| if (chunk.buffer.size() < 1) { | ||
| if (chunk.buffer.empty()) { |
| } | ||
|
|
||
| inline int TBuffer::skipSpacesAtBeginOfLine(int i, int i2) | ||
| inline int TBuffer::skipSpacesAtBeginOfLine(const int row, const int column) |
| luaSendText = "main"; | ||
| std::string windowName = "main"; | ||
| if (lua_gettop(L) > 0 && !lua_isstring(L, 1)) { | ||
| lua_pushfstring(L, "getBgColor: bad argument #1 type (window name as string {\"main\" assumed if omitted} is optional, got %s!)", lua_typename(L, 1)); |
| overline = !qFuzzyCompare(1.0, 1.0 + lua_tonumber(L, s)); | ||
| } else { | ||
| lua_pushfstring(L, | ||
| "setTextFormat: bad argument #%d type (overline format as boolean {or number,\n" |
Member
There was a problem hiding this comment.
I am not sure what this is talking about
Member
Author
There was a problem hiding this comment.
The code it is copying seems to allow both a numeric or a boolean argument - but the supplied one doesn't look like either!
| reverse = !qFuzzyCompare(1.0, 1.0 + lua_tonumber(L, s)); | ||
| } else { | ||
| lua_pushfstring(L, | ||
| "setTextFormat: bad argument #%d type (reverse format as boolean {or number,\n" |
src/TLuaInterpreter.cpp
Outdated
|
|
||
| bool isAttributeEnabled; | ||
| if (!lua_isboolean(L, ++s)) { | ||
| lua_pushfstring(L, "setReverse: bad argument #%d type (enable underline attribute as boolean expected, got %s!)", s, luaL_typename(L, s)); |
| default: | ||
| // Invalid | ||
| lua_pushnil(L); | ||
| lua_pushfstring(L, "blink rate as a number %d is out of range {use 0 = off, 1 = slow, 2 = rapid}", lua_tonumber(L, s)); |
Member
There was a problem hiding this comment.
We should use strings for this for more readable code - text makes more sense than numbers
Member
Author
There was a problem hiding this comment.
Well, it accepts a boolean (for the normal blink rate) so as to match the other functions, but I'd be reluctant to hard code American English strings here so the user has to remember the right word, e.g. is it "Fast"/"fast"/"rapid"/"RAPID"/"Quick"/"QUICKLY"; logically that suggests we should add "off" and "on" for the other attributes...?
IMVHO those numbers are not an unreasonable way to do it - they are simple to validate (and are pretty much language neutral for I18n later - we just have to document the numbers in the translated manual for every language except for places not using Arabic numerals!) - and if someone gets it wrong the fix is there in the error message.
Member
There was a problem hiding this comment.
Just "off", "slow", and "fast" (more common than rapid) is enough.
Numbers are not more i18n friendly, it is easier to remember a word in English - and people will have to remember them to code - than numbers.
Member
|
Coverity is reporting some new issues in this PR: New defect(s) Reported-by: Coverity Scan
Showing 6 of 6 defect(s)
** CID 1468485: Incorrect expression (USELESS_CALL)
________________________________________________________________________________________________________
*** CID 1468485: Incorrect expression (USELESS_CALL)
/TBuffer.h: 99 in TChar::setAllDisplayAttributes(QFlags<TChar::AttributeFlag>)()
93 void setColors(const QColor& newForeGroundColor, const QColor& newBackGroundColor) {
94 mFgColor=newForeGroundColor;
95 mBgColor=newBackGroundColor;
96 }
97 // Only considers the following flags: Bold, Italic, Overline, Reverse,
98 // Strikeout, Underline, SlowBlink, FastBlink, does not consider Echo:
>>> CID 1468485: Incorrect expression (USELESS_CALL)
>>> Calling "(this->mFlags & 0xffffffffffffff00) | (newDisplayAttributes & TChar::TestMask)" is only useful for its return value, which is ignored.
99 void setAllDisplayAttributes(const AttributeFlags newDisplayAttributes) { (mFlags & ~TestMask) | (newDisplayAttributes & TestMask); }
100 void setForeground(const QColor& newColor) { mFgColor = newColor; }
101 void setBackground(const QColor& newColor) { mBgColor = newColor; }
102 void setTextFormat(const QColor& newFgColor, const QColor& newBgColor, const AttributeFlags newDisplayAttributes) {
103 setColors(newFgColor, newBgColor);
104 setAllDisplayAttributes(newDisplayAttributes);
** CID 1468484: Incorrect expression (COPY_PASTE_ERROR)
/TTrigger.cpp: 659 in TTrigger::match_color_pattern(int, int)()
________________________________________________________________________________________________________
*** CID 1468484: Incorrect expression (COPY_PASTE_ERROR)
/TTrigger.cpp: 659 in TTrigger::match_color_pattern(int, int)()
653 // This now allows matching against the current default colours (-1) and
654 // allows ONE of the foreground or background to NOT be considered (-2)
655 // Ideally we should base the matching on only the ANSI code but not
656 // all parts of the text come from the Server and can be determined to
657 // have come from a decoded ANSI code number:
658 if ( ((pCT->ansiFg == scmIgnored)||((pCT->ansiFg == scmDefault) && mpHost->mpConsole->mFgColor == (*it).foreground())||(pCT->mFgColor == (*it).foreground()))
>>> CID 1468484: Incorrect expression (COPY_PASTE_ERROR)
>>> "foreground" in "this->mpHost->mpConsole->mBgColor == (*it)->foreground()" looks like a copy-paste error.
659 &&((pCT->ansiBg == scmIgnored)||((pCT->ansiBg == scmDefault) && mpHost->mpConsole->mBgColor == (*it).foreground())||(pCT->mBgColor == (*it).background()))) {
660
661 if (matchBegin == -1) {
662 matchBegin = pos;
663 }
664 matching = true;
** CID 1468483: (EVALUATION_ORDER)
/TBuffer.cpp: 1286 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
/TBuffer.cpp: 1286 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
/TBuffer.cpp: 1291 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
/TBuffer.cpp: 1291 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
/TBuffer.cpp: 1313 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
/TBuffer.cpp: 1313 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
/TBuffer.cpp: 1317 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
/TBuffer.cpp: 1317 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
________________________________________________________________________________________________________
*** CID 1468483: (EVALUATION_ORDER)
/TBuffer.cpp: 1286 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1280 // the use of ':' would make it is possible to
1281 // separate the sub-options as compared to a whole
1282 // new SGR code...
1283 if (mIsHighOrMillionsColorModeForeground) {
1284 if (i + 2 <= codeRet) {
1285 // Have enough for all three suboptions
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "<temporary>.QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1286 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1287 mForeGroundColorLight = mForeGroundColor;
1288 mIsDefaultColor = false;
1289 } else if (i + 1 <= codeRet) {
1290 // Have enough for two suboptions, but third, blue component is zero
1291 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
/TBuffer.cpp: 1286 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1280 // the use of ':' would make it is possible to
1281 // separate the sub-options as compared to a whole
1282 // new SGR code...
1283 if (mIsHighOrMillionsColorModeForeground) {
1284 if (i + 2 <= codeRet) {
1285 // Have enough for all three suboptions
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1286 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1287 mForeGroundColorLight = mForeGroundColor;
1288 mIsDefaultColor = false;
1289 } else if (i + 1 <= codeRet) {
1290 // Have enough for two suboptions, but third, blue component is zero
1291 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
/TBuffer.cpp: 1291 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1285 // Have enough for all three suboptions
1286 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1287 mForeGroundColorLight = mForeGroundColor;
1288 mIsDefaultColor = false;
1289 } else if (i + 1 <= codeRet) {
1290 // Have enough for two suboptions, but third, blue component is zero
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "<temporary>.QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 0, 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1291 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
1292 mForeGroundColorLight = mForeGroundColor;
1293 mIsDefaultColor = false;
1294 } else if (i <= codeRet) {
1295 // Have enough for one suboption, but second and third, green and blue component are zero
1296 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), 0, 0);
/TBuffer.cpp: 1291 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1285 // Have enough for all three suboptions
1286 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1287 mForeGroundColorLight = mForeGroundColor;
1288 mIsDefaultColor = false;
1289 } else if (i + 1 <= codeRet) {
1290 // Have enough for two suboptions, but third, blue component is zero
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 0, 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1291 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
1292 mForeGroundColorLight = mForeGroundColor;
1293 mIsDefaultColor = false;
1294 } else if (i <= codeRet) {
1295 // Have enough for one suboption, but second and third, green and blue component are zero
1296 mForeGroundColor = QColor(qBound(0, mCode[i++], 255), 0, 0);
/TBuffer.cpp: 1313 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1307 mIsHighOrMillionsColorMode = false;
1308 mIsHighOrMillionsColorModeBackground = false;
1309
1310 } else if (mIsHighOrMillionsColorModeBackground) {
1311 if (i + 2 <= codeRet) {
1312 // Have enough for all three suboptions
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "<temporary>.QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1313 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1314 mIsDefaultColor = false;
1315 } else if (i + 1 <= codeRet) {
1316 // Have enough for two suboptions, but third, blue component is zero
1317 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
1318 mIsDefaultColor = false;
/TBuffer.cpp: 1313 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1307 mIsHighOrMillionsColorMode = false;
1308 mIsHighOrMillionsColorModeBackground = false;
1309
1310 } else if (mIsHighOrMillionsColorModeBackground) {
1311 if (i + 2 <= codeRet) {
1312 // Have enough for all three suboptions
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1313 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1314 mIsDefaultColor = false;
1315 } else if (i + 1 <= codeRet) {
1316 // Have enough for two suboptions, but third, blue component is zero
1317 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
1318 mIsDefaultColor = false;
/TBuffer.cpp: 1317 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1311 if (i + 2 <= codeRet) {
1312 // Have enough for all three suboptions
1313 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1314 mIsDefaultColor = false;
1315 } else if (i + 1 <= codeRet) {
1316 // Have enough for two suboptions, but third, blue component is zero
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "<temporary>.QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 0, 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1317 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
1318 mIsDefaultColor = false;
1319 } else if (i <= codeRet) {
1320 // Have enough for one suboption, but second and third, green and blue component are zero
1321 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), 0, 0);
1322 mIsDefaultColor = false;
/TBuffer.cpp: 1317 in TBuffer::translateToPlainText(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &, bool)()
1311 if (i + 2 <= codeRet) {
1312 // Have enough for all three suboptions
1313 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255));
1314 mIsDefaultColor = false;
1315 } else if (i + 1 <= codeRet) {
1316 // Have enough for two suboptions, but third, blue component is zero
>>> CID 1468483: (EVALUATION_ORDER)
>>> In "QColor(qBound(int const(0), &this->mCode[i++], int const(255)), qBound(int const(0), &this->mCode[i++], int const(255)), 0, 255)", "i" is written in "qBound(int const(0), &this->mCode[i++], int const(255))" and written in "qBound(int const(0), &this->mCode[i++], int const(255))" but the order in which the side effects take place is undefined because there is no intervening sequence point.
1317 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), qBound(0, mCode[i++], 255), 0);
1318 mIsDefaultColor = false;
1319 } else if (i <= codeRet) {
1320 // Have enough for one suboption, but second and third, green and blue component are zero
1321 mBackGroundColor = QColor(qBound(0, mCode[i++], 255), 0, 0);
1322 mIsDefaultColor = false;
** CID 1468482: Incorrect expression (EVALUATION_ORDER)
/mudlet.cpp: 3612 in mudlet::slot_updateBlinkPhase()()
________________________________________________________________________________________________________
*** CID 1468482: Incorrect expression (EVALUATION_ORDER)
/mudlet.cpp: 3612 in mudlet::slot_updateBlinkPhase()()
3606 }
3607 }
3608
3609 void mudlet::slot_updateBlinkPhase()
3610 {
3611 if (mIsBlinkingEnabled) {
>>> CID 1468482: Incorrect expression (EVALUATION_ORDER)
>>> In "this->mMasterBlinkPhase = ++this->mMasterBlinkPhase % 4", "this->mMasterBlinkPhase" is written in "this->mMasterBlinkPhase" (the assignment left-hand side) and written in "++this->mMasterBlinkPhase % 4" but the order in which the side effects take place is undefined because there is no intervening sequence point.
3612 mMasterBlinkPhase = (++mMasterBlinkPhase) % 4;
3613 emit signal_blinkingRedraw(mMasterBlinkPhase);
3614 }
3615 }
3616
3617 // state is true to disable blinking:
** CID 1468481: Control flow issues (DEADCODE)
/TBuffer.cpp: 3195 in TBuffer::bufferToHtml(bool, int, int, int, int)()
________________________________________________________________________________________________________
*** CID 1468481: Control flow issues (DEADCODE)
/TBuffer.cpp: 3195 in TBuffer::bufferToHtml(bool, int, int, int, int)()
3189 // used for "copy HTML", this is the first line of selection (because of
3190 // the padding needed)
3191 if (firstSpan) {
3192 // Must skip the close of the preceding span as there isn't one
3193 firstSpan = false;
3194 } else {
>>> CID 1468481: Control flow issues (DEADCODE)
>>> Execution cannot reach the expression "s.append(QLatin1String("</span>"))" inside this statement: "s.append(QLatin1String("</s...".
3195 s.append(QLatin1String("</span>"));
3196 }
3197
3198 s.append(QStringLiteral("<span>%1").arg(QString(spacePadding, QChar::Space)));
3199 // Pad out with spaces to the right so a partial first line lines up
3200 }
** CID 1468480: Incorrect expression (EVALUATION_ORDER)
/TBuffer.cpp: 3041 in TBuffer::applyAttribute(const QPoint &, const QPoint &, QFlags<TChar::AttributeFlag>, bool)()
________________________________________________________________________________________________________
*** CID 1468480: Incorrect expression (EVALUATION_ORDER)
/TBuffer.cpp: 3041 in TBuffer::applyAttribute(const QPoint &, const QPoint &, QFlags<TChar::AttributeFlag>, bool)()
3035 while (x < static_cast<int>(buffer.at(y).size())) {
3036 if (y >= y2) {
3037 if (x >= x2) {
3038 return true;
3039 }
3040 }
>>> CID 1468480: Incorrect expression (EVALUATION_ORDER)
>>> In "this->buffer.at(y)->at(x).mFlags = (this->buffer.at(y)->at(x++).mFlags & ~attributes.operator QFlags<TChar::AttributeFlag>::Int()) | TChar::AttributeFlags const(state ? TChar::AttributeFlags const(attributes) : TChar::AttributeFlags const(TChar::None))", "x" is read in "this->buffer.at(y)->at(x).mFlags" (the assignment left-hand side) and written in "(this->buffer.at(y)->at(x++).mFlags & ~attributes.operator QFlags<TChar::AttributeFlag>::Int()) | TChar::AttributeFlags const(state ? TChar::AttributeFlags const(attributes) : TChar::AttributeFlags const(TChar::None))" but the order of evaluation is undefined because there is no intervening sequence point.
3041 buffer.at(y).at(x).mFlags = (buffer.at(y).at(x++).mFlags &~(attributes)) | (state ? attributes : TChar::None);
3042 }
3043 }
3044 return true;
3045 } else {
3046 return false; |
…-loops
I was using multiple post-increment operators on the same variable in some
method calls but did not realise that this was undefined behaviour because
the order of processing is not specified in TBuffer.cpp. I also made a
similar error in (void) mudlet::slot_updateBlinkPhase().
Whilst searching for uses of the operator++ in TBuffer I found several
cases, including in some for-loops where the postfix form could be replaced
by the more efficient prefix form - and/or merged into another operation.
There was some unreachable code at the start of:
(QString) TBuffer::bufferToHtml(const bool showTimeStamp, const int row,
const int endColumn, const int startColumn, int spacePadding)
which has been removed.
Added run-time error message to Lua commands that set display attributes
so that trying to apply an attribute on a non-existent window is reported.
Fixed a couple of "exceptional" spelling mistakes!
Also removed three unused members that Codecy detected as being
uninitialised:
* (int) TBuffer::speedTP
* (int) TBuffer::speedSequencer;
* (int) TBuffer::speedAppend;
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
This might be caused by having non-BMP characters but is characterised by the (inner) x variable passing through zero in a negitive way without escaping the loop in (void) TTexctEdit::mouseMoveEvent(QMouseEvent). Note that there are multiple instances of 'x' and 'y' in this method and shadowing is taking place which may also be contributing to the problem...! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Member
|
Copy to html is broken - for this replay, I get the following html: https://ada-young.appspot.com/pastebin/rglxXJLC In-game: Log: |
Member
Author
|
Oops, I forgot the closing |
Member
|
@SlySven Could you also look at benchmarking the changes? |
Member
Author
|
I think I need to revise this one after #1632 goes in as they both touch the drawing of texts in |
Member
|
Yes - not a good idea to leave PRs sitting stale, they rot... |
Member
Author
|
Now that we've got the CJK duo-spaced font stuff in I can get back to this one... |
This was referenced Jun 4, 2018
This was referenced Jul 17, 2018
Member
Author
|
This had become un-mergable as it included text drawing code that was incompatible with code that had subsequently been adopted. Most of it has been reapplied to the now current code-base as #1840 . |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brief overview of PR changes/additions
A whole load of
TBufferrefactoring and some other things found in the process.Details:
7/27for On/Off and Overline53/55for On/Off to ANSI codes handled.5/6/25for Slow/Fast/Off to ANSI codes handled - with a global enabled/disabled option in the profile preferences - defaults to disabled.#defined constantsTCHAR_BOLDetc. into aQFLag/enumTChar::AttributeFlagswhich is declared and capable ofQFlagOR operations.boolsandintsas individual formatting options and colour components to take singleTChar::AttributeFlagsand one or twoQColorsinstead.TBufferas they are not needed.setBold,setItalicsetc. to take a (combinations allowed)TChar::Attributeflag value instead.(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)ints as colour components (r,g,b) inTColorTabledefined inTTriggerclass to a pair ofQColors.TBuffer::set[BF]gColor(...)overloads that take aQColorargument.select()/deselect()/isSelected()constmethods toTCharclass to hide/separate the selection process from the formatting effect. (TChar::Reversetracks the ANSI SGR reverse colour attribute and its effect is EX-ORed with the(bool) TChar::mIsSelectedflag when it is time to paint the results on-screen).QDebughelper to display prettily theTChar::AttributeFlagsvalues if sent to qDebug() and related methods.Also:
QStringargument from:(void) mudlet::setLink(...)(void) TConsole::setLink(...)(inline void) TTextEdit::drawCharacters(...)tempAnsiColorTriggerlua function that, unliketempColorTriggeruses the correct ANSI colours in the range 0-15 - although the original also handles the 256 colour range in the 16-255 correctly the first 16 values are miss-mapped and it is not possible to change them without breakage.TLuaInterpreter::debug()which would not work correctly if there was more than one value on the lua stack to print out.update.hinmudlet.cppas it is already inmudlet.h.Motivation for adding to Mudlet
I wanted to clean up some aspects of the
TBuffercode before other reworking to better handle non-ASCII texts and their painting on theirTConsoleparents and the attribute handling was going to crop up so I decide to clear that up first...!Other info (issues closed, discussion etc)
This should enable us to close long-standing issues #703 and #477 ! 😇