gh-750: Fixed VMenu set selection behavior around list edges.

[MKadaner]

Summary

Warning! Bugs are expected.

At the very least, deleting the last entry in the view/edit history works as expected.

References

gh-750

Checklist

• I have followed the contributing guidelines.
• I have discussed this with project maintainers:
  
  If not checked, I accept that this work might be rejected in favor of a different great big ineffable plan.
  

Details

I've rewritten substantial part of VMenu::SetSelectPos because the logic was complicated and looked as if it compensated for something which had since disappeared.

Let's see what I broke.

[MKadaner]

@alabuzhev, could you please have a look? The PR is ready for review. "Bugs are expected" because the logic around the TopPos field is cumbersome. I did my best to avoid breaking things, but I cannot guarantee. I deliberately expect shouting from beta testers, and I am going to fix the fallouts.

[alabuzhev]

Looks like the problematic behavior, described in #750, still occurs in some other places, e.g. User Menu, supposedly because deletion is handled separately there.
We probably should review and fix all such places (not necessarily under the same issue).

[yegor-mialyk]

I see Michael continues using PascalCase for new and refactored methods, whereas Alex is transitioning to underscore_case. @alabuzhev what are the rules you use? I had an impression that we have to adopt underscore_case for both refactored methods and fresh code.

[alabuzhev]

@yegor-mialyk I try to follow this naming style:

1. functions, classes, methods, enums, namespaces, concepts: snake_case (mostly because it's the language default).
2. variables, parameters: PascalCase (mostly to avoid conflicts with #1, e.g. when you have things like foo Foo).
3. members: m_PascalCase (mostly to be consistent with #2).
4. macros: SCREAMING_SNAKE_CASE (for obvious reasons).

It's not enforced anywhere though.

[MKadaner]

functions, classes, methods, enums, namespaces, concepts: snake_case (mostly because it's the language default).

To be clear, do you want me to rename the functions (and maybe some other things)? I do not mind, just wanted to make sure this is how you want them to be.

[alabuzhev]

do you want me to rename the functions

Up to you, I was just answering Yegor's question.

[MKadaner]

Looks like the problematic behavior, described in #750, still occurs in some other places, e.g. User Menu, supposedly because deletion is handled separately there.
We probably should review and fix all such places (not necessarily under the same issue).

Yes, let's wait for beta-testers' complaints. Could you please create a bug for User Menu and assign it to myself?

[MKadaner]

I squashed the commits and made it ready for merge, but if there are more issues, I'll be happy to fix them.

[MKadaner]

Another attempt. :)

[MKadaner]

As an aside (not pushing it now).

This is how it should be done:

-int find_nearest(std::ranges::contiguous_range auto const& Range, const int Pos, const auto Pred, const bool GoBackward, const bool DoWrap)
+int find_nearest(std::ranges::contiguous_range auto const& Range, const int Pos, auto&& Pred, const bool GoBackward, const 
 bool DoWrap)
 {
 	using namespace std::views;
 
 	assert(0 <= Pos && Pos < static_cast<int>(Range.size()));
 
+	const auto Filter = filter(FWD(Pred));
 	const auto FindPos =
 		[&](const auto First, const auto Second)
 		{
 			const auto FindPosPart =
 				[&](const auto Part)
 				{
-					if (auto Filtered = Range | Part | filter(Pred))
+					if (auto Filtered = Range | Part | Filter)
 						return static_cast<int>(&Filtered.front() - Range.data());
 
 					return -1;
 				};
 
 			if (const auto Found{ FindPosPart(First) }; Found != -1) return Found;
 			if (const auto Found{ FindPosPart(Second) }; Found != -1) return Found;
 			return -1;
 		};
 
 	return GoBackward
 		? (DoWrap
 			? FindPos(take(Pos + 1) | reverse, drop(Pos + 1) | reverse)
 			: FindPos(take(Pos + 1) | reverse, drop(Pos + 1)))
 		: (DoWrap
 			? FindPos(drop(Pos), take(Pos))
 			: FindPos(drop(Pos), take(Pos) | reverse));
 }

[MKadaner]

Rebased to the latest master.

[MKadaner]

Ping

[alabuzhev]

This is how it should be done:

+	const auto Filter = filter(FWD(Pred));

There's a great talk on views by Nico Josuttis:

One of the key points is that views, unfortunately, cache and the cache can sometimes backfire spectacularly.
This is, of course, not a concern in this particular case, since we're only observing the collection, just something to be aware of before applying the same pattern elsewhere.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
0.0% Duplication

[alabuzhev]

Ping

Looks good, let's merge if you have nothing to add.

[MKadaner]

Thanks for the pointer, I'll watch later.

Thank you for merging. No, I was not going to apply forwarding of the predicate; just posted the "right" pattern (to the best of my understanding). It does not matter here, just something to be aware of if we go ranges.

BTW, what I really wanted to do was to join the two Parts together and get rid of the two calls of FindPosPart. It would be perfectly possible in C# with IEnumerable.Concat<TSource> Method. I could not find how to do that in C++ without materializing the collection (of at least references) or writing a full-fledged implementation with coroutines and std::generator. As far as I understand, the problem is that the Parts views are of different types, so std::ranges::join_view does not compile.

[MKadaner]

[Tagging @alabuzhev just because the PR is already merged (thank you), and he may overlook the comment.]

One of the key points is that views, unfortunately, cache and the cache can sometimes backfire spectacularly.

I think that the word "cache" here is misleading. A view is an iterator and a sentinel, nothing more, nothing less. We all know the rules about (invalidating) iterators. The same applies to views. It is that simple. Is it a good design? It depends. As it often happens with C++, this design provides for runtime performance, particularly through avoiding indirections. The price is cognitive burden imposed on the programmer.

Views do not cache anything. They just wrap an iterator-sentinel pair in a convenient and safe object. When using views, one cannot, syntactically, match an iterator against a wrong sentinel. It's a huge advantage over the traditional C++ iterators. Do we need more? Probably. Will we have it? Hopefully.

The very word "view" is a misnomer. In C#, to denote a similar thing, they use the apt word "enumerable." C# enumerables have pretty much the same issue as C++ views, and nobody complains. Except for everyone who starts using them experiences a few pretty dramatic setbacks. But overall, the right word sets the right expectations, and people learn to grab enumerables at the right end. And in C#, enumerable are outrageously expensive, at least from C++ perspective. They are roughly equivalent to C++ std::generators. And unlike C#, in C++ one does not need to use a generator for the simple task of projecting elements of a collection. Isn't it a beauty? Which does not mean we should not aspire for a belle.