Fullwidth-aware VMenu rendering and other improvements.
#1062
Conversation
| // In full-width-aware mode, it works only for 0 (left) and -1 (right). | ||
| // Otherwise, it assumes that each character occupies exactly one screen cell. | ||
| // In fact, we do not need any other alignment. Should we simplify the code here? |
There was a problem hiding this comment.
@alabuzhev, do you have any druthers about removing support of arbitrary horizontal position relative to the end of an item?
There was a problem hiding this comment.
Could you elaborate please?
There was a problem hiding this comment.
Oops! Apparently, it works. My bad. Looks like I forgot how this function works. I'll remove the comment.
Wouldn't the |
MKadaner
force-pushed
the
mzk/full-width-aware-VMenu
branch
from
87409bd to
6e6b70c
Compare
Good point. Thanks. Renamed, rebased, pushed. |
|
|
There are bugs! Fixing them... |
| // Returns true if all cells were consumed | ||
| bool ClippedText(string_view Str, const segment Bounds, bool& AllCharsConsumed) |
There was a problem hiding this comment.
Maybe it's just me, but it feels somewhat strange.
The function's primary task is, supposedly, writing the text.
If so, one might expect that it returns:
- true if "succeeded" (wrote all the text)
- false if "failed" (not enough space)
"Returns true if all cells were consumed" sounds like a function which primary task is filling the specified rectangular area.
| if (AllCharsConsumed || CurX >= Bounds.end()) | ||
| return CurX >= Bounds.end(); | ||
|
|
||
| assert(char_width::is_wide(encoding::utf16::extract_codepoint(Str))); |
There was a problem hiding this comment.
Could you explain this please?
There was a problem hiding this comment.
I presume that char_width::is_wide tells me if the codepoint occupies two cells. If I am wrong, please let me know. I'll think of something else.
Most of the time, I expect that we stopped writing because we either consumed all available cells or all characters (or both). See lines 960, 961 above.
However, there is a special case when the next codepoint in the string is wide, but there was only one cell available. In this case, write_text will back off and leave both the last codepoint and the last cell not consumed. Even though there is still one cell available, the caller must stop writing (more on the contract of this function in the other comment, tomorrow), because the next codepoint in the string will never fit, and the caller must blank the last cell (even though it could display the left half of the wide char, couldn't it?)
Now, these two asserts are because I am not 100% sure that this is the only case when write_text would stop writing before hitting either end of string or end of screen area. If such other case exists, especially if a narrow codepoint was not set to the available cell, I would like to see this case and very likely somehow adjust the implementation.
Does it all make any sense? Do you want me to add some comment here?
There was a problem hiding this comment.
back off and leave both the last codepoint and the last cell not consumed
Yes, it's the right thing to do.
If we can only get here in the aforementioned scenario, then the assert makes sense.
Just pls make sure it works as expected with fullwidth-aware rendering turned off (is_wide always returns false).
| } }; | ||
|
|
||
| WriteOrSkip(Bounds.start(), chars_to_cells); | ||
| WriteOrSkip(Bounds.end(), write_text); |
There was a problem hiding this comment.
Could you explain this please?
Is it because Bounds define a part of Str to be shown and this is basically a width-aware substr?
There was a problem hiding this comment.
Bounds are in the screen coordinate system. They specify the screen area where the string should be visible. So, in a sense it is a width-aware substr. Putting it the other way around, it's a substr in terms of screen cells, after the string was written starting at CurX. The algorithm has two nice properties:
- Correctly handles wide codepoints (those occupying two cells) straddling either left or right boundary.
- Does not allocate
real_cellsfor the part of the string to the left of the clipping area (which part can be very large).
Is there a better approach? Do you want me to add some comment here?
There was a problem hiding this comment.
Is there a better approach?
It's fine, just potentially this "width-aware substr" could be useful in other scenarios, so it could make sense to decouple it from writing to screen, e.g. have a separate function that calculates real string indices from provided visual bounds and then write the part defined by those indices using existing functions or something like that.
There's also visual_pos_to_string_pos, which perhaps could help here as well.
|
There is a failed assertion in markup_slice_boundaries: |
3 participants
Summary
VMenuis rendered correctly in fullwidth-aware mode.References
I promised to fix outstanding issues in the short discussion associated with 9f7479a. Unfortunately, I cannot find this thread now.
Checklist
If not checked, I accept that this work might be rejected in favor of a different great big ineffable plan.
Details
The main inconsistency in
VMenurendering was in calculating items' indent / hanging to vertically align all found entries (Ctrl+Numpad5).Other notable changes:
BoundedText. It takes a string to render and bounds in screen coordinates, and renders the string starting atCurXwhile hiding the parts outside of the bounds. It then advancesCurXaccordingly.VMenu.VMenu::MenuTextfunctions; usingBoundedTextinstead.intersectfunction fromalgorithm.hpptosegment.hpp.segment_t::horizontal_extentandsegment_t::vertical_extentfunctions takerectangle_t. Unfortunately,rectangle.hpp#includesalgorithm.hppwhich#includedsegment.hpp.segment.hppto be#included beforerectangle_tis defined. It could probably be resolved by forward declaringrectangle_tinsegment.hppbut movingintersectfunction is simpler and it feels belonging insegment.hppanyway.