Improve: add autowrap and support for Chinese/other language word wrapping#6207
Improve: add autowrap and support for Chinese/other language word wrapping#6207vadi2 merged 55 commits intoMudlet:developmentfrom
Conversation
Modify text wrapping to use actual text width as calculated by "horizontalAdvance", this allows the wrapping to work for a language where character width is not the same across available characters.
|
Hey there! Thanks for helping Mudlet improve. 🌟 Test versionsYou can directly test the changes here:
No need to install anything - just unzip and run. |
|
|
SlySven
left a comment
There was a problem hiding this comment.
Sorry, but this needs a rework... 🙁
TBuffer now uses the more efficient at(i) instead of operator=[i] Wrapping position calculation now uses QTextBoundaryFinder::word to find boundary Auto wrapping for wrap setting below 10, and should properly autowrap all mini console.
Took what I did with TBuffer::wrap and implemented for TBuffer::append
… Text-Wrapping
Fix CodeFactor issues
wrapLine is a special version of wrap, basically wrap that only wrap one line. Code in wrap is now refactored into wrapLine. Also removed two of the debug message, since those were used only for me to figure out what was going on.
Missed a line when I did the refactoring, pSpace should be format that's passed in
Removed unused variables
Added space around =
Wrapping now accounts for LineFeed, instead of pushing it in, it actually does a new line (which is what line feed is supposed to do) wrapLine now properly does the single line mode, and uses remove -> insert , as oppose to "pop_back" "push_back". Change different place where wrapLine was being called with incorrect width.
Added comment explaining what wrapLine does
Change the QTextBoundaryFinder mode from Word to Line
src/TBuffer.cpp
Outdated
| @@ -2500,41 +2633,52 @@ inline int TBuffer::wrap(int startLine) | |||
| bool hasContent = false; | |||
|
|
|||
There was a problem hiding this comment.
👍 Well spotted - I suspect this is why previously, wrapped text (when there was a non-zero indent) was being preceded by black spaces instead of those in the correct background colour... 😀
There was a problem hiding this comment.
actually everywhere that pass in "format" is actually just passing in a generic pSpace, so this wouldn't fix that. Now I am interested in fixing that problem, but I believe it isn't related to this PR, so as such I would do a separate PR for that.
There was a problem hiding this comment.
Hmm I may not be grasping the problem discussed here, but is it really an issue?
There was a problem hiding this comment.
I think what the code "should do", is figure out what the format of the indent "should be". Now that's what I would need to find out, which is what is a generic space supposed to look like?
There was a problem hiding this comment.
actually everywhere that pass in "format" is actually just passing in a generic pSpace, so this wouldn't fix that. Now I am interested in fixing that problem,...
I am not sure that is the case - this method TBuffer::wrapLine(...) is used in more than just TConsole::luaWrapLine(int) and in those other cases (TConsole::printCommand(...), TConsole::insertText(...), TConsole::insertLink(...)) a format is supplied...
| <enum>QTabWidget::Rounded</enum> | ||
| </property> | ||
| <property name="currentIndex"> | ||
| <number>0</number> |
There was a problem hiding this comment.
ℹ️ When you save the form/dialogue from the stand-alone Qt Designer utility or the version integrated into the Qt Creator IDE it remembers the current tab you are on - you need to always switch back to the first tab (or avoid this change getting into the end PR) to prevent this (change of the "currentIndex" away from 0) from appearing...
As it happens I have put code into the dlgProfilePrefences constructor to prevent this from having any effect (it forces the current tab to the first one automatically) - but basically this change is not wanted... 😀
| </item> | ||
| <item> | ||
| <widget class="QLabel" name="label_23"> | ||
| <widget class="QLabel" name="label_wrapCharacters"> |
There was a problem hiding this comment.
👍 for giving a meaningful name to the label for the control - too many of them have been left at the default of label_ and a number...! 😀
Co-authored-by: Stephen Lyons <slysven@virginmedia.com>
Change instance where we are doing size() == 0, to do isEmpty() instead.
|
Done with optimization for this PR, ready for review |
|
Awesome, reviewing it. I'll also enable the clang-tidy review bot, it might have some suggestions. You've done so much work on this already, so I can apply those for you in case your battery is running low =) |
Minor review feedback
Added comment explaining what the variable does Renamed variable to be more descriptive
|
I've incorporated your recommendation (PR), and also addressed your questions. |
Added suggested changes to the comment Remove redundant variable Fixed an indention bug
… Text-Wrapping
#### Brief overview of PR changes/additions #6207 and subsequently #6277 have since been discovered to be the source of several bugs, and so we have decided to revert the changes in development but open a new PR for the changes themselves. #### Motivation for adding to Mudlet #### Other info (issues closed, discussion etc) resolve the following issues: #6416 #6394 #6321 #6290 Co-authored-by: Kebap <kebap_spam@gmx.net>
Modify text wrapping to use actual text width as calculated by "horizontalAdvance", this allows the wrapping to work for a language where character width is not the same across available characters.
Brief overview of PR changes/additions
Text wrapping now uses QFontMetrics(font).horizontalAdvance to determine where the wrap should happen.
Motivation for adding to Mudlet
Text wrapping was assuming all characters are equal width, thus it completely breaks if that isn't the case.
Other info (issues closed, discussion etc)
closes #5564