Fix 'copy as image' for 2 width characters#2869
Conversation
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
src/TTextEdit.cpp
Outdated
| int characterWidth = 1; | ||
| int currentX = 0; | ||
| for (const auto character : mpBuffer->lineBuffer.at(lineNumber)) { | ||
| for (const auto& character : mpBuffer->lineBuffer.at(lineNumber)) { |
There was a problem hiding this comment.
QChar in the line of text and passing that to (uint) TTextEdit::getGraphemeBaseCharacter(const QString& str) const - which is wrong, wrong, wrong! That method's purpose is to return the first Unicode codepoint (the base character for the first grapheme) in the QString given to it. When that method was designed it was expected that the QString being passed would be a single grapheme at a time (which may contain more that one Unicode code point) as extracted by using a QTextBoundaryFinder on the source text line.
Passing every single QChar in a line to getGraphemeWidth(...) - which is what happens further down in - is likely to be what is spamming the debug output with TTextEdit::Xxxxx(...) WARN - trying to get width of a Unicode character which is unprintable, codepoint number: U+XXXX when it runs into a non-BMP codepoint (such as many modern Emojis) as that is represented by a pair of QChars (a High surrogate followed by a Low one in the UTF-16 encoding that is implicit in the QString class) - it will also barf at combining diacriticals as those should not be appearing as the first codepoint in a grapheme! 😮
Compare the usage here and in (void) TTextEdit::slot_copySelectionToClipboardImage() to the correct usage in (int) TTextEdit::drawGrapheme(QPainter& painter, const QPoint& cursor, const QString& grapheme, int column, TChar& charStyle) const and it's call (twice) from (void) TTextEdit::drawLine(QPainter& painter, int lineNumber, int lineOfScreen) const which DOE iterator through the lines in (QStringList) TBuffer::lineBuffer a grapheme at a time...
I'll take a closer look at these two suspect bits over the weekend and see if I can clean things up for Monday morning...
There was a problem hiding this comment.
Have a look at the screenshots - it clearly works for i18n 😉 It doesn't work for emoji's and other non-BMP codepoints because I'm not passing the right data in that usecase, but we can very clearly see that it works correctly for Chinese.
Let me know if you get the new PR up by Monday - Tuesday July 30th is the cutoff date for new PRs for 4.0, so if you're not able to make it by then we should merge this which is better than nothing for i18n. Not for emoji's but that is not my target here.
|
I am having some success - I think I have got those two places selecting/working on the right amount of
and here is what it looks like as a saved clipboard image:
OTOH I think I need to check something as the following is a single line containing some characters that are definitely non-BMP characters (as you can see from the analyser):
but the width is currently a bit suspicious: Similarly with combining diacriticals (which will also be important for Vietnamese as the vowels in that language are added to the consonant glyphs in the same manner):
which does not have enough width:
|
|
Okay - see how things are after merging vadi2#15 ... |
* Revise: improve mouse X position to index into TBuffer::lineBuffer This properly considers the width of the base character in a grapheme (it ignores the effect of any combining diacriticals) but DOES account for those Unicode codepoints that are NOT in the basic (first) Unicode Multi-plane. Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Revise: use QFontMetrics::averageCharWidth() instead of odd letter widths For the use made of the widths concerned it makes more sense to use it rather than the width of a space or 'W' character - and is less likely to cause a seg. fault for fonts that do not have the selected character; Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Debug: add optional debug output (can be switched on from debugger) The code concerned is very spammy so it should only be switched on when needed. Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * BugFix: get width of image correct Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Revise: remove excess debugging info and comments following peer review Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
|
Some strange selection snake happening here...
|
|
I don't have the bandwidth to deal with this right now so this up to someone else to take a look at if they'd like. macOS builds are broken and that takes priority, this is a minor issue in the scheme of things - no users have come across it so far. |
I have an idea or two which I will take a look at in the next 24 hours... |
|
Thank you. I'm looking at fixing Mac builds meanwhile. |
…age-for-chinese # Conflicts: # src/TTextEdit.cpp
* Refactor: extract a simple function that is used in several five places Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Refactor: eliminate shadowed variables in TTextEdit::mouseMoveEvent(...) The use of `x` and `y` in nested code is confusing as the inner ones mask the outer ones. This commit renames the inner copies. It also makes a local reference to a section of the text attribute buffer: `(std::deque<std::deque<TChar>>) TBuffere::buffer) that is used several times. Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * BugFix: cure the faulty selection when crossing an empty line. The use of *manhattenLength()* was incorrect when determining whether it was the start or end of the selection that was nearest to the mouse cursor, instead it now (correctly) only considers the horizontal (x) offset when the vertical offsets of the start and end point of the selection are equal. Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * BugFix: finally cures the selection of text both with and without timestamps Also: * abstract the number of spaces for the timestamp on the left to a constant `(int) TTextEdit::mTimeStampWidth`. * renames `y` and `x` variables in the `TTextEdit::mouseMoveEvent(...)` to `lineIndex` and `tCharIndex` as that more accurately describes them - and also rename another pair of shadowing/masking `y` and `x` variables in the SAME method to `yIndex` and `xIndex` respectively. * convert a few remaining random C-style `(int)` casts in `TTextEdit` class to C++ `static_cast<int>(...)` ones. NOTE: An issue with deselection something in an upward direction against the left margin remains - however the area is being correctly deselected but code elsewhere that analyses which parts of the display actually require repainting is missing that portion of the modified area of text. It appears to be within the `TTextEdit::highlight()` method but it IS a separate issue as the underlying text attribute buffer is being correctly changed to clear the selection flags. Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Revise: make time stamps show in lower pane as well as upper one It is visually disturbing for the lower pane in the main, error and debug consoles not to match the upper one when it is revealed by scrolling - or by turning on the timestamps for the main console. Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Revise: reduce space for time stamps from 15 to 13 (12 + space) characters The use of 15 rather than 13 produced the need for an additional tweak when trying to work out where the first actual game text start when trying to deduce the position of it relative to the cursor! Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Fix insertPopup and echoPopup to accept "main" as an argument (Mudlet#3013) * Update: drop support for saving to Mudlet 2.1 map format & update default This PR removes support for saving in the binary map file format of Mudlet 2.1 - i.e. format **16** (retaining the ability to read it) and adjusts the format for new map files to be **20** by default - up from the previous default of **18**. This should speed up map loading a little - as it will remove the need to convert the data to the current forms used internally as well as making some details more robust. It also allows the removal of some warning code that would have fired if area or map user data features were used and format *16* was selected for saving. #### History of recent (last six years) changes to Map File Format: * dfce0f0 (PR Mudlet#2106) - **20** Improved way that custom exit line data was held internally (so that the keys are now the same as the other exit details that are keyed by a string {doors, exit weight}. The custom exit line style is stored as the shorter and easier to code with `Qt::PenStyle enum` instead of a English `QString` and the custom exit line as a `QColor` instead of a `QList<int>` with 3 elements - (so the custom exit line could have a alpha component in the future!) Code is in place to support a workaround to work within map formats back to include version 17. * 91a08c3 (PR Mudlet#1543) - **19** Added support for more than one of any grapheme for the 2D map room symbol. Code is in place to support a workaround to work within map formats back to include version 17. * 94dd41b (associated with PR Mudlet#301) - **18** Added support for multiple user rooms in map file copied to other profiles - so that the original player room (in the other profile) is retained in a map copied over. Also revised the `TArea::rooms` from being a `QList` to a faster to look up in `QSet`. Code in place to support saving in previous formats. * fb79c62 (associated with PR Mudlet#280) - **17** Added support for area and map user data features. Warnings are issued should these be actually used and a lower map format is specified to save the map in **as that data will then be lost from the map file**. * 88ef649 - **16** Map format of Mudlet 2.1 dating back to 2013-01-02 . Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * BugFix: handle empty SGR...m sequences so they are treated as SGR0m The empty (no parameter) case is the same as the one with a single 0 which is the reset attributes case. Mudlet was not handling it as that but it will do after this commit. This will close issue Mudlet#2993 . Also clean up some tabs used for spaces in the same method: `(void) TBuffer::translateToPlainText(...)` Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Revise: some commented material in TTextExit::convertMouseXToBufferX Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
demonnic
left a comment
There was a problem hiding this comment.
Selection appears to be copying correctly to me.
|
This PR re-broke timestamp highlighting... |
|
💯 |
This is a squash and merge commit of nine commits with the following edited commit messages: * Revise: improve mouse X position to index into TBuffer::lineBuffer This properly considers the width of the base character in a grapheme (it ignores the effect of any combining diacriticals) but DOES account for those Unicode codepoints that are NOT in the basic (first) Unicode Multi-plane. * Revise: use QFontMetrics::averageCharWidth() instead of odd letter widths For the use made of the widths concerned it makes more sense to use it rather than the width of a space or 'W' character - and is less likely to cause a seg. fault for fonts that do not have the selected character; * BugFix: get width of image correct * Refactor: extract a simple function that is used in several five places * Refactor: eliminate shadowed variables in TTextEdit::mouseMoveEvent(...) The use of `x` and `y` in nested code is confusing as the inner ones mask the outer ones. This commit renames the inner copies. It also makes a local reference to a section of the text attribute buffer: `(std::deque<std::deque<TChar>>) TBuffere::buffer) that is used several times. * BugFix: cure the faulty selection when crossing an empty line. The use of *manhattenLength()* was incorrect when determining whether it was the start or end of the selection that was nearest to the mouse cursor, instead it now (correctly) only considers the horizontal (x) offset when the vertical offsets of the start and end point of the selection are equal. Also: * abstract the number of spaces for the timestamp on the left to a constant `(int) TTextEdit::mTimeStampWidth`. * renames `y` and `x` variables in the `TTextEdit::mouseMoveEvent(...)` to `lineIndex` and `tCharIndex` as that more accurately describes them - and also rename another pair of shadowing/masking `y` and `x` variables in the SAME method to `yIndex` and `xIndex` respectively. * convert a few remaining random C-style `(int)` casts in `TTextEdit` class to C++ `static_cast<int>(...)` ones. NOTE: An issue with deselection something in an upward direction against the left margin remains - however the area is being correctly deselected but code elsewhere that analyses which parts of the display actually require repainting is missing that portion of the modified area of text. It appears to be within the `TTextEdit::highlight()` method but it IS a separate issue as the underlying text attribute buffer is being correctly changed to clear the selection flags. * Revise: make time stamps show in lower pane as well as upper one It is visually disturbing for the lower pane in the main, error and debug consoles not to match the upper one when it is revealed by scrolling - or by turning on the timestamps for the main console. * Revise: reduce space for time stamps from 15 to 13 (12 + space) characters The use of 15 rather than 13 produced the need for an additional tweak when trying to work out where the first actual game text start when trying to deduce the position of it relative to the cursor! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Brief overview of PR changes/additions
Fix 'copy as image' to work for 2 width character text: it hasn't been updated from the assumption that 1 character = 1 width.
Given the following room:
Copy as image before (bad):
Now:
Motivation for adding to Mudlet
Better i18n support.
Other info (issues closed, discussion etc)
Pretty small thing as no Chinese users have discovered it yet.