Fullwidth-aware VMenu rendering and other improvements.

[MKadaner]

Summary

VMenu is 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

• I have followed the contributing guidelines.
• I have discussed this with project maintainers: 9f7479a
  
  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 VMenu rendering was in calculating items' indent / hanging to vertically align all found entries (Ctrl+Numpad5).

Other notable changes:

1. Introduced new text rendering function BoundedText. It takes a string to render and bounds in screen coordinates, and renders the string starting at CurX while hiding the parts outside of the bounds. It then advances CurX accordingly.
2. Used this function in rendering menu items with highlight and in other places around VMenu.
3. Removed VMenu::MenuText functions; using BoundedText instead.
4. Moved segment intersect function from algorithm.hpp to segment.hpp.
   • This change was required because the new segment_t::horizontal_extent and segment_t::vertical_extent functions take rectangle_t. Unfortunately, rectangle.hpp #includes algorithm.hpp which #included segment.hpp.
   • This dependency cycle caused segment.hpp to be #included before rectangle_t is defined. It could probably be resolved by forward declaring rectangle_t in segment.hpp but moving intersect function is simpler and it feels belonging in segment.hpp anyway.
5. Some refactoring.

[HamRusTal]

renders the string starting at CurX while hiding the parts outside of the bounds

Wouldn't the ClippedText name rather than BoundedText be more appropriate then?

[MKadaner]

renders the string starting at CurX while hiding the parts outside of the bounds

Wouldn't the ClippedText name rather than BoundedText be more appropriate then?

Good point. Thanks. Renamed, rebased, pushed.

[MKadaner]

There are bugs! Fixing them...

[alabuzhev]

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.

[MKadaner]

Yeah! The function signature is awkward. Let me concentrate on the function's contract for now. Then, maybe you can suggest something better. In the following explanations I will continue using the name ClippedText for the lack of better options. So:

The only caller of ClippedText is VMenu::DrawItemText (there are other callers, but they are non-essential.) DrawItemText needs to write a potentially long string piece by piece. Let's call these pieces "slices." The string may start far beyond the left boundary of the visible area and may extend far beyond the right boundary. There may be slice(s) at the beginning which are fully hidden, then a slice that needs to be written partially (clipped), then maybe some visible slices, and a slice hanging beyond the right boundary which needs to be clipped as well.

DrawItemText knows where to start writing the entire string and the positions (or sizes) of the slices. *) It wants to set CurX (GotoXY(Bounds.start() + Item.HorizontalPosition, Y);) and call something repeatedly for each slice (while alternating colors) without bothering about how to clip the slices. DrawItemText must know (a) whether the right horizontal scroll indicator is needed (the left one is easy), and (b) whether it must stop writing slices. †)

Accordingly, ClippedText takes the string and the clipping area (Bounds should probably be renamed) and returns two Booleans. One indicates whether there are no more cells in the clipping area (even if the last one could not be filled) and is used to stop writing slices. The other Boolean indicates whether the entire string was written and is used to decide about the right scroll indicator.

The first version of ClippedText returned std::pair<bool, bool>. I did not like it because:

• It is confusing; which flag shows what?
• In VMenu::DrawFixedColumns we do not need the AllCharsConsumed flag. Such an overload is impossible. Of course, I could say something like const auto [AllCellsConsumed, _]{ ClippedText(CellText, CellArea) }; but it would make the call site unnecessary verbose.

I also tried signatures like these:

void ClippedText(string_view Str, const segment Bounds, bool& AllCellsConsumed, bool& AllCharsConsumed);
void ClippedText(string_view Str, const segment Bounds, bool& AllCellsConsumed);

Unfortunately, they require defining out variables at all call sites, which is also verbose.

I'll be happy to change the signature of this function to something better. If you have any ideas, please let me know. Also, if the contract can be simplified, it will also be great.

---

*) I am going to refactor markup_slice_boundaries to return slice sizes instead of boundaries. With this new function, it will be more convenient.

†) The stop signal is not just an optimization to avoid unnecessary calls to ClippedText. Because the latter can stop before hitting either the end of the string or the end of the clip area (see discussion in another comment about a straddling wide codepoint at the right boundary), DrawItemText cannot depend on comparing CurX with the right boundary. It really needs an authoritative sign.

[MKadaner]

There is another interesting issue about ClippedText's contract. Please look at this picture.

Here I suppressed the left horizontal scroll indicators in the first four menu items (but still draw them in all other items). Please compare strings 1|7 and 3|7. The former demonstrates how ClippedText actually clips wide character on the left boundary. To skip characters before the left boundary, ClippedText calls chars_to_cells passing to it the left boundary which is the next cell after the left scroll indicator cell. The こ character straddles the left boundary, so chars_to_cells leaves it in the string. Then ClippedText advances CurX to the left boundary minus one (according to chars_to_cells's output) and calls write_text for the rest of the string.

Thus, ClippedText actually wrote something into the cell previous to the left boundary! That's bad. The problem is immediately compensated by VMenu::DrawItemText which overwrites this garbled cell with the left scroll indicator. I designed ClippedText this way intentionally (and almost forgot this quirk) because the code looks nice and simple, and the whole thing is guaranteed to work. I can see two possible solutions:

1. Make this behavior part of ClippedText's contract and describe it in the comment. In other words, declare that it expects one extra cell to the left of the left boundary and the caller is expected to overwrite this cell with something. Of course, the caller will want the scroll indicator, or a border, or something. 😀
2. Fix it.

The obvious drawback of legitimizing this behavior is that the caller will be coupled with ClippedText even tighter than it seems. Who can use this function besides VMenu? Viewer probably? The Editor has neither horizontal scroll indicators (why?) nor borders. Anybody else?

On the other hand, handling this special case should not be very difficult. Just add a check similar to the one at the end of ClippedText.

What do you think?

[alabuzhev]

Thus, ClippedText actually wrote something into the cell previous to the left boundary! That's bad.

Not good indeed. Functions should not write to random locations and expect that something else happens to fix that.

Who can use this function besides VMenu?

It's in interf.cpp, so potentially anyone. I think it's better to fix it.

Anybody else?

If you, say, press F7, paste こ and then start typing 1, then eventually こ will straddle the left boundary of the edit control. If I'm not mistaken, it is handled here:

FarManager/far/edit.cpp

Lines 290 to 304 in 8814bb2

	if (ReconstructedVisualLeftPos < LeftPos)
	{
	auto NextReal = RealLeftPos;
	int NextVisual;
	for (;;)
	{
	NextVisual = RealToVisual.get(++NextReal);
	if (NextVisual > LeftPos)
	break;
	}
	OutStr.insert(0, NextVisual - LeftPos, L' ');
	RealLeftPos = NextReal;
	}

Not saying you should do exactly the same, but maybe something similar.

[MKadaner]

Of course, it must be fixed. It was more of a rhetorical question on my part. 😀

I've seen this place and even considered to reuse something. But first, it seems to be coupled to the client code, and second, which may be even worse, it is rather low-level, dealing with individual characters. I really think my approach is more elegant while as efficient.

When I fix the left boundary behavior, this new function can probably be reused in other places, Edit included. But this is a different story.

[alabuzhev]

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?

[MKadaner]

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:

1. Correctly handles wide codepoints (those occupying two cells) straddling either left or right boundary.
2. Does not allocate real_cells for 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?

[alabuzhev]

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.

[MKadaner]

Actually, my first implementation went along these lines, but handling of wide codepoints straddling boundaries, especially the right one, appeared to be non-trivial, and the resulting code was counterintuitive. Particularly, I had to artificially increase the number of available screen cells by one, otherwise in certain scenarios the penultimate codepoint was not written. I could not clearly explain the reason. It was something about the last codepoint was dropped during preliminary calculations, then the last-but-one codepoint was dropped during actual writing.

Clearly, it should be possible to make it work and implement in maintainable way, but I started thinking about a different approach and the result mightily pleases me aesthetically.

As for visual_pos_to_string_pos, yes it could probably be used here. However, with the minor refactoring of the Text function, both phases of ClippedText rely on the same low-level string_to_cells algorithm, which guarantees that a wide codepoint on the left boundary is correctly handed off from one phase to the other.

If you have no serious objections, I will keep the implementation as is.

[MKadaner]

I thought about it a bit more.

[P]otentially 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

Unfortunately, such function is difficult to define. Consider please a wide codepoint straddling across a visual boundary. Which code unit index in the string should be returned for this boundary?

[alabuzhev]

It was something about the last codepoint was dropped during preliminary calculations, then the last-but-one codepoint was dropped during actual writing.

This doesn't sound right. Do you have exact steps to reproduce?

a wide codepoint straddling across a visual boundary

Just round it down.
The most common substr-ish pattern in UI scenarios is "we have N available cells and an arbitrary string, we want to cut a part of that string that would fit into those N cells, and maybe decorate it somehow", e.g.

• We want to set a title in some dialog (" PLACEHOLDER ", 11 cells + 2 spaces, 13 total).
• The new title is ダチヂッツヅテデトドナニヌネノハ
• The basic idea is to take a substr that would occupy at most 11 cells, prepend and append:

auto Part = visual_substr(Str, 0, 11);
auto Title = " " + Part + " ";

The final result we want:

• Only 5 wide characters can fit into 11 cells.
• In this case it's irrelevant that the final title is only 12 cells and not 13 - the dialog engine will still center it properly.
• In the cases when we want the exact size, it's not a problem to check the length and append a space if needed.
• Currently this can be achieved with visual_pos_to_string_pos(Str, 11, 1).

[MKadaner]

This scenario does not require truncating the string from the left. In the dialog title scenario, it is not needed.

For a title-like scenario, it would be natural to round the left side up. But then we would have to somehow convey to the caller whether the substrings should start at the exact left boundary or one cell to the right. This is because in VMenu (and in Viewer) scenario, all items should scroll horizontally en-block. So, I cannot truncate the string from the left, round straddling wide character up, and start writing at the left edge of the text area. The string will appear one cell to the left relative to other strings.

There are two basic scenarios:

1. When we need to get a substring which would fit into given number of cells.
2. When we need to precisely position the entire string on the screen and then actually write only the part visible in some kind of viewport.

While substring can be used to implement the second scenario, it:

1. Would require a more complex substring contract (should we skip one cell before writing the substring?), and
2. Would be suboptimal because the visible part of the string will have to be scanned twice (once to calculate substring, and second time to actually write substring.

Thus, for the second scenario the proposed ClippedText function fits more naturally.

Are there any scenarios when a substring should start in the middle of the string? If not, visual_pos_to_string_pos is all we need.

[MKadaner]

It was something about the last codepoint was dropped during preliminary calculations, then the last-but-one codepoint was dropped during actual writing.

This doesn't sound right. Do you have exact steps to reproduce?

I scratched the original implementation; it was complete nonsense. However, after writing the above, I think I know what the problem was. I did a moral equivalent of width-aware substring, then wrote this substring starting from the left boundary. At that time, I did not yet realize all details about wide codepoints straddling boundaries. So, sometimes, on the left boundary, I was getting exactly the scenario I described above. However, when I looked at the result, I was concentrating on the right boundary (I decided to deal with the left side later.) So, the string was shifted one cell to the left (which I did not notice), the last codepoint was wide and was truncated, and I observed two-space padding at the right, when there should be no more than one. The quick-n-dirty solution was to tell string_to_cells that there is one more, imaginary cell available, and then Global->ScrBuf->Write the buffer truncated at the right boundary.

As I said, the implementation was complicated and hard to understand to say the least. At certain point, I realized that it cannot be this way and something totally different is required, so I scratched everything and did what you see now.

[alabuzhev]

There is a failed assertion in markup_slice_boundaries:
https://github.com/FarGroup/FarManager/actions/runs/20736571090/job/59534925247?pr=1062#step:7:591

[MKadaner]

There is a failed assertion in markup_slice_boundaries: https://github.com/FarGroup/FarManager/actions/runs/20736571090/job/59534925247?pr=1062#step:7:591

Thanks. Yeah! I've seen it. markup_slice_boundaries deliberately rejects empty input segment, and I accidentally dropped the check during refactoring. There is another assert is triggered as well. I'll fix them, hopefully during weekend.

[alabuzhev]

There is another assert is triggered as well. I'll fix them, hopefully during weekend.

No problem.
It was actually useful - it helped me realize that we do not capture assertion messages in bug reports.
Now we do 😄

[MKadaner]

@alabuzhev, I fixed ClippedText and it can be considered complete. It became even more elegant. I also wrote a substantial comment explaining suggested usage and the contract. Please have a look. The PR is still work in progress, so I turned it into draft for now.

We still need to decide upon the function signature. I have two suggestions:

1. The signature can now be void ClippedText(string_view& Str, const segment ClippingSegment);. After all fixes and improvements, the caller can trivially use CurX >= ClippingSegment.end() instead of the return value and Str.empty() instead of AllCharsConsumed. Then there will be only one overload, and the call sites will still look reasonably clean.
2. Move the function into vmenu.cpp. While it can be used in some other contexts, it is most useful in VMenu::DrawItemText. Moving it out of interf may naturally lower the quality expectations of naming, signature, and the contract.

Please let me know whether you like me to apply either of the suggestions, or both.

[MKadaner]

@alabuzhev, please review this PR. It is ready. Now it all makes sense (at least to me 😀).

There are indeed some rendering glitches in non-full-width aware rendering mode. They are pretty similar to what is happening in the Editor. To see an example of these glitches, please open this file Multilingual.txt.zip in the Editor (with full-width aware rendering off), go to line three, and move the caret to the right, beyond the window edge. At some point, you should see something like this:

Nevertheless, I suggest proceeding with this PR as is. Later I will try to debug this issue from VMenu perspective. However, since the issue seems to be with some low-level function(s), I believe it should not be a showstopper.

[alabuzhev]

please review this PR. It is ready

Apologies, I've been somewhat busy recently. Hope to get to it soon.

There are indeed some rendering glitches in non-full-width aware rendering mode.

There is absolutely no point in looking for any glitches with any funny characters in non-FWA mode.

[MKadaner]

Rebased and merged. Normal characters in FWA-mode work just fine.

[alabuzhev]

Please move it before the corresponding test block.

[MKadaner]

Actually, moved the test next to the other segment tests where "common/segment.hpp" was already #included. I believe this is a better arrangement.

[alabuzhev]

Could you remind me the context please?
I thought at some point we were discussing that the codepoint should not be considered consumed if it doesn't fit?

Also:

• "write behind the end sounds fishy, is it avoidable?
• "padding is added at the leading or trailing cell" - what exactly does it mean? How can we have space for the trailing cell but not for the codepoint?

[MKadaner]

Thank you for catching it. It was actually a bug; I was generalizing the code a bit too eagerly.

Please see if it is easier to understand now. The padding is, of course, required and added as necessary. However, now the code is in a different place where it should not raise anyone's eyebrow.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud