Skip to content

Not storing Annotation in menu item and segment improvement #1112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alabuzhev merged 3 commits into from
Jun 15, 2026
Merged

Not storing Annotation in menu item and segment improvement #1112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6083b21
Do not store `Annotation` in menu item.
MKadaner Jun 13, 2026
2d06495
Added `segment::start_or` and `segment::end_or`
MKadaner Jun 13, 2026
5121918
Changelog and vbuild
MKadaner Jun 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions far/changelog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
--------------------------------------------------------------------------------
MZK 2026-06-14 09:12:38-04:00 - build 6701

1. Instead of storing Annotation in menu item,
retrieving it from the menu owner on demand.

2. Refactoring.

--------------------------------------------------------------------------------
drkns 2026-06-14 10:28:08+01:00 - build 6700

Expand Down
4 changes: 4 additions & 0 deletions far/common.tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1258,6 +1258,10 @@ TEST_CASE("segment.constexpr")
STATIC_REQUIRE(segment{ 5, segment::sentinel_tag{ 7 } }.start() == 5);
STATIC_REQUIRE(segment{ 5, segment::sentinel_tag{ 7 } }.length() == 2);
STATIC_REQUIRE(segment{ 5, segment::length_tag{ 2 } }.end() == 7);
STATIC_REQUIRE(segment{ 5, segment::sentinel_tag{ 7 } }.start_or(42) == 5);
STATIC_REQUIRE(segment{ 5, segment::length_tag{ 2 } }.end_or(42) == 7);
STATIC_REQUIRE(segment{}.start_or(42) == 42);
STATIC_REQUIRE(segment{}.end_or(42) == 42);
STATIC_REQUIRE(segment{}.ray() == segment{ 0, segment::sentinel_tag{ std::numeric_limits<int>::max() } });
STATIC_REQUIRE(segment{}.ray(42) == segment{ 42, segment::sentinel_tag{ std::numeric_limits<int>::max() } });
STATIC_REQUIRE((segment{}.ray(42).iota() | std::views::drop(3) | std::views::take(3)).front() == 45);
Expand Down
14 changes: 11 additions & 3 deletions far/common/segment.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

//----------------------------------------------------------------------------

template<typename T>
template<std::integral T>

@alabuzhev alabuzhev Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

@MKadaner MKadaner Jun 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have done it from the very beginning. I was not sure about availability of the concepts on all platforms. One can say I was lazy to do it right. Here are roughly my considerations.

Without this constraint, segment_t can be instantiated with double. With a few comparison operators sprinkled here and there, the behavior could be surprising. On the other hand, we do not need floating-point segments, so restricting is simpler and more natural than adding proper floating-point support.

More immediately, I wanted to allow passing a "compatible" type to start_or / end_or, not only exact T (because why not). However, there is no std::nothrow_convertible_to which would be required to guarantee noexcept-ness of these functions. There are basically two options: defining nothrow_convertible_to or constraining the entire class to deal with integrals only (then std::convertible_to<T> combined with std::integral T will allow to safely promise noexcept). Putting both considerations together, I decided to tighten the entire class.

@MKadaner MKadaner Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then std::convertible_to<T> combined with std::integral T will allow to safely promise noexcept

Well, not exactly. One can imagine a non-trivial operator int(). 🙁

So, I'd say we need both, integral and nothrow_convertible_to. Do you want me to fix it? If so, where should I define the concept? Is there a dedicated place?

@alabuzhev alabuzhev Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just not a big fan of overconstraining everything "just in case".

I've seen a compilation error somewhere recently, I don't remember the details, but essentially someone wanted to emulate __int128, not supported natively, naturally via a custom integer-like class, and it was used to instantiate some trivial algorithm elsewhere, but someone else decided to be overly meticulous and used this integral concept there, with somewhat predictable consequences.

This usage of concepts undermines the core feature of templates - duck typing, and turns them into some C#-like generics where T must be known beforehand.

I'm not saying it's that important in this particular case (feel free to leave as is), I just don't like that this pattern sort of becomes the default in the industry.

@MKadaner MKadaner Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This usage of concepts undermines the core feature of templates - duck typing

I do not agree, both in this case and in general.

With segment, any integral type (and we enjoy 10 of them in C++) will work. The implementation does not assume any particular size or signedness, but it is about as much as we can support. segment implementation requires usual arithmetic and comparison operators on T and even slightly depends on integer overflow behavior (assert(length() >= 0);) so it cannot blindly accept just anything which quacks adds and compares. Even double may produce unexpected results and should be excluded. Thus, requiring std::integral T is not really over-constraining. It is requesting the minimum required to guarantee (without sending implementation complexity through the roof) the sane and expected behavior.

As an aside, I like to say that "here we are not in the business of writing a general-purpose publicly available library," (compare to STL) so we do not need to go full-generic; our users (our beloved selves) will always be careful and won't do silly things. Nevertheless, when all we need is to drop in a one-word standard concept, we can afford it and get great benefit of extra type safety which eventually translates into overall software safety.

At the philosophical (or computer science) level, I think constrained C++ templates are still templates. Generally (unlike in this case), the (algorithm) implementation requires a type providing certain behavior (or combination of behaviors). If the type does not provide such and such functions or properties, ideally, the implementation does not compile and template SFINAE. That's fine, except for two things. First, sometimes it may accidentally compile with some obscure type, and Boom! Second, looking at the algorithm definition, one cannot easily say whether his shiny new Int128 will work with it.

The concepts and requires are the great way to document templates. Sometimes a concept is just that, a concept. For example, many range-related concepts have associated semantic requirements (e.g., std::ranges::sized_range). Such requirements are for documentation purposes only. They are not syntactically enforced. "The burden to ensure that library templates are instantiated with template arguments that satisfy these requirements is on the programmer."

As long as a type faithfully behaves per the documented requirements (and supports necessary functions), it can be used in substitution. To me, it amounts to duck typing. The documentation helps avoiding possibly expensive mistakes.

@alabuzhev alabuzhev Jun 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the type does not provide such and such functions or properties, ideally, the implementation does not compile and template SFINAE

That's another place where concepts are too often misused.
SFINAE was a hack with a rather narrow application: emulating overload resolution in templates. That's literally in the name: it's not the end of the world if one substitution fails, another might succeed. And if none of them succeeds, you get a compilation error (great) with a rather horrible error message (not great, but better than nothing).

Now we got "concepts" built upon exactly the very same principles, and people started putting them into template signatures, effectively turning everything into SFINAE, even the templates that were never supposed to participate in overload resolution.

There is a rather noticeable difference between "the compiler found a matching template, attempted to instantiate it and failed to do so because this and that" and "the compiler found no candidates because all of them got rejected because <100 lines of instantiation stack for each of the candidates>. We can always improve the former with static_assert, but the latter is kinda unreadable by definition.

@MKadaner MKadaner Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other compilers do a better job of it. I think that MSVC and clang's outputs for the concept-based variant are the cleanest. It is not a coincidence. One of the design goals of concepts was to enable compiler emitting meaningful, easy to interpret diagnostics. Here, the concept is used to documenting the contract rather than suppressing unwanted instantiation. Modern compilers understand this specification and are able to explain the problem in pretty clean words.

I admit that "static assertion failed" is not bad. However, the explanation is still indirect, something on the lines, "I tried to instantiate it and hit a static assert. Oops!" While with the concept in the signature, it is straightforward "constraints are not satisfied." The compiler did not even try to instantiate anything; it's simply "no matching overloaded function found."

The beauty of specifying constraints in the signature is that both compilers and humans understand them the same way.

class segment_t
{
[[nodiscard]]
Expand Down Expand Up @@ -77,6 +77,14 @@ class segment_t
[[nodiscard]]
constexpr T end() const noexcept { assert(!empty()); return m_End; }

template<std::convertible_to<T> U>
[[nodiscard]]
constexpr T start_or(U default_value) const noexcept { return empty() ? default_value : start(); }

template<std::convertible_to<T> U>
[[nodiscard]]
constexpr T end_or(U default_value) const noexcept { return empty() ? default_value : end(); }

[[nodiscard]]
constexpr auto iota() const noexcept { return empty() ? std::views::iota(T{}, T{}) : std::views::iota(start(), end()); }

Expand All @@ -94,14 +102,14 @@ class segment_t
: segment_t{ InitialPoint, length_tag{ domain_max() } };
}

template<typename U>
template<std::convertible_to<T> U>
[[nodiscard]]
static constexpr segment_t horizontal_extent(const rectangle_t<U>& rect) noexcept
{
return { rect.left, length_tag{ rect.width() } };
}

template<typename U>
template<std::convertible_to<T> U>
[[nodiscard]]
static constexpr segment_t vertical_extent(const rectangle_t<U>& rect) noexcept
{
Expand Down
11 changes: 10 additions & 1 deletion far/editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3527,7 +3527,6 @@ namespace
void add_item(FindCoord FoundCoords, string_view ItemText)
{
menu_item_ex Item{ string{ ItemText } };
Item.Annotation.emplace(FoundCoords.Pos, segment::length_tag{ FoundCoords.SearchLen });
Item.ComplexUserData = FoundCoords;
m_Menu->AddItem(Item);

Expand Down Expand Up @@ -3606,6 +3605,16 @@ namespace
return false;
}
);
m_Menu->ListBox().RegisterItemAnnotationProvider(
[](const menu_item_ex& Item)
{
if (const auto* Coord{ std::any_cast<FindCoord>(&Item.ComplexUserData) })
{
return segment{ Coord->Pos, segment::length_tag{ Coord->SearchLen } };
}
return segment{};
}
);
}

void toggle_zoom()
Expand Down
2 changes: 1 addition & 1 deletion far/vbuild.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
6700
6701
30 changes: 17 additions & 13 deletions far/vmenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,11 +527,6 @@ namespace
return Item.Name;
}

int safe_get_item_annotation(menu_item_ex const& Item)
{
return Item.Annotation.transform([](const auto Annotation) { return Annotation.start(); }).value_or(0);
}

std::pair<int, int> item_hpos_limits(const int ItemLength, const int TextAreaWidth, const item_hscroll_policy Policy) noexcept
{
using enum item_hscroll_policy;
Expand Down Expand Up @@ -949,7 +944,7 @@ int VMenu::AddItem(menu_item_ex&& NewItem,int PosAdd)

const auto ItemLength{ GetItemVisualLength(NewMenuItem) };
UpdateMaxLength(ItemLength);
m_HorizontalTracker->add_item(NewMenuItem.HorizontalPosition, ItemLength, safe_get_item_annotation(NewMenuItem));
m_HorizontalTracker->add_item(NewMenuItem.HorizontalPosition, ItemLength, SafeGetItemAnnotationStart(NewMenuItem));

const auto NewFlags = NewMenuItem.Flags;
NewMenuItem.Flags = 0;
Expand All @@ -967,13 +962,12 @@ bool VMenu::UpdateItem(const FarListUpdate *NewItem)

auto& Item = Items[NewItem->Index];
m_HorizontalTracker->remove_item(
Item.HorizontalPosition, GetItemVisualLength(Item), safe_get_item_annotation(Item));
Item.HorizontalPosition, GetItemVisualLength(Item), SafeGetItemAnnotationStart(Item));

// Освободим память... от ранее занятого ;-)
if (NewItem->Item.Flags&LIF_DELETEUSERDATA)
{
Item.ComplexUserData = {};
Item.Annotation.reset();
}

Item.Name = NullToEmpty(NewItem->Item.Text);
Expand All @@ -982,7 +976,7 @@ bool VMenu::UpdateItem(const FarListUpdate *NewItem)

const auto ItemLength{ GetItemVisualLength(Item) };
UpdateMaxLength(ItemLength);
m_HorizontalTracker->add_item(Item.HorizontalPosition, ItemLength, safe_get_item_annotation(Item));
m_HorizontalTracker->add_item(Item.HorizontalPosition, ItemLength, SafeGetItemAnnotationStart(Item));

SetMenuFlags(VMENU_UPDATEREQUIRED | (bFilterEnabled ? VMENU_REFILTERREQUIRED : VMENU_NONE));

Expand Down Expand Up @@ -1013,7 +1007,7 @@ int VMenu::DeleteItem(int ID, int Count)
--ItemHiddenCount;

m_HorizontalTracker->remove_item(
I.HorizontalPosition, GetItemVisualLength(I), safe_get_item_annotation(I));
I.HorizontalPosition, GetItemVisualLength(I), SafeGetItemAnnotationStart(I));
}

// а вот теперь перемещения
Expand Down Expand Up @@ -2363,7 +2357,7 @@ bool VMenu::SetItemHPos(menu_item_ex& Item, const auto& GetNewHPos)
return GetNewHPos(Item.HorizontalPosition, ItemLength);
}();

m_HorizontalTracker->update_item_hpos(Item.HorizontalPosition, NewHPos, ItemLength, safe_get_item_annotation(Item));
m_HorizontalTracker->update_item_hpos(Item.HorizontalPosition, NewHPos, ItemLength, SafeGetItemAnnotationStart(Item));

if (Item.HorizontalPosition == NewHPos) return false;
Item.HorizontalPosition = NewHPos;
Expand Down Expand Up @@ -2457,7 +2451,7 @@ bool VMenu::AlignAnnotations()
return SetAllItemsHPos(
[&](const menu_item_ex& Item)
{
return AlignPos - static_cast<int>(visual_string_length(get_item_text(Item).substr(0, safe_get_item_annotation(Item))));
return AlignPos - static_cast<int>(visual_string_length(get_item_text(Item).substr(0, SafeGetItemAnnotationStart(Item))));
});
}

Expand Down Expand Up @@ -2934,7 +2928,7 @@ std::tuple<string, segment> VMenu::GetItemTextWithHighlight(const menu_item_ex&
const auto GetHighlight{
[&]
{
if (Item.Annotation) return *Item.Annotation;
if (m_ItemAnnotationProvider) return m_ItemAnnotationProvider(Item);
if (HotkeyPos != string::npos) return segment{ static_cast<int>(HotkeyPos), segment::length_tag{ 1 } };
if (Item.AutoHotkey) return segment{ static_cast<int>(Item.AutoHotkeyPos), segment::length_tag{ 1 } };
return segment{};
Expand Down Expand Up @@ -3427,6 +3421,11 @@ void VMenu::RegisterExtendedDataProvider(extended_item_data_getter&& ExtendedDat
m_ExtendedDataSetter = std::move(ExtendedDataSetter);
}

void VMenu::RegisterItemAnnotationProvider(item_annotation_provider&& ItemAnnotationProvider)
{
m_ItemAnnotationProvider = std::move(ItemAnnotationProvider);
}

FarListItem *VMenu::MenuItem2FarList(const menu_item_ex *MItem, FarListItem *FItem)
{
if (FItem && MItem)
Expand Down Expand Up @@ -3589,6 +3588,11 @@ int VMenu::GetItemVisualLength(const menu_item_ex& Item) const
return static_cast<int>(CheckFlags(VMENU_SHOWAMPERSAND) ? visual_string_length(ItemText) : HiStrlen(ItemText));
}

int VMenu::SafeGetItemAnnotationStart(const menu_item_ex& Item) const
{
return m_ItemAnnotationProvider ? m_ItemAnnotationProvider(Item).start_or(0) : 0;
}

#ifdef ENABLE_TESTS

#include "testing.hpp"
Expand Down
6 changes: 5 additions & 1 deletion far/vmenu.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ struct menu_item_ex: menu_item
: menu_item{ std::forward<T>(Name), Flags }
{}

std::optional<segment> Annotation;
std::any ComplexUserData;
intptr_t SimpleUserData{};

Expand Down Expand Up @@ -251,6 +250,9 @@ class VMenu final: public Modal
using extended_item_data_setter = std::function<bool(menu_item_ex&, const extended_item_data&)>;
void RegisterExtendedDataProvider(extended_item_data_getter&& ExtendedDataGetter, extended_item_data_setter&& ExtendedDataSetter);

using item_annotation_provider = std::function<segment(const menu_item_ex&)>;
void RegisterItemAnnotationProvider(item_annotation_provider&& ItemAnnotationProvider);

int GetSelectPos() const { return SelectPos; }
int GetLastSelectPosResult() const { return SelectPosResult; }
int GetSelectPos(FarListPos *ListPos) const;
Expand Down Expand Up @@ -324,6 +326,7 @@ class VMenu final: public Modal

[[nodiscard]] int CalculateTextAreaWidth() const;
[[nodiscard]] int GetItemVisualLength(const menu_item_ex& Item) const;
[[nodiscard]] int SafeGetItemAnnotationStart(const menu_item_ex& Item) const;

int GetItemPosition(int Position) const;
bool CheckKeyHiOrAcc(DWORD Key, int Type, bool Translate, bool ChangePos, int& NewPos);
Expand Down Expand Up @@ -361,6 +364,7 @@ class VMenu final: public Modal
fixed_column_provider m_FixedColumnProvider;
extended_item_data_getter m_ExtendedDataGetter;
extended_item_data_setter m_ExtendedDataSetter;
item_annotation_provider m_ItemAnnotationProvider;
window_ptr CurrentWindow;
bool PrevCursorVisible{};
size_t PrevCursorSize{};
Expand Down
1 change: 0 additions & 1 deletion far/vmenu2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,6 @@ int VMenu2::AddItem(const menu_item_ex& NewItem, int PosAdd)

auto& Item = at(PosAdd);
Item.AccelKey=NewItem.AccelKey;
Item.Annotation = NewItem.Annotation;

Resize();
return n;
Expand Down
Loading