gh-802: Macro API to report VMenu alignment state. Part 1: Backend implementation. #895
Conversation
…ckend implementation. Implemented `vmenu_horizontal_tracker` which knows whether menu items are currently aligned. Surfacing the state maintained by `vmenu_horizontal_tracker` is coming soon.
|
|
|
||
| return m_HorizontalTracker.start_bulk_update( | ||
| vmenu_horizontal_tracker::alignment::Right, TextAreaWidth + NewHPos + 1, TextAreaWidth, Policy); | ||
| }() }; |
There was a problem hiding this comment.
IIFE looks somewhat complicated here, maybe ternary operator?
const auto Cookie = NewHPos >= 0?
m_HorizontalTracker.start_bulk_update(vmenu_horizontal_tracker::alignment::Left, ...) :
m_HorizontalTracker.start_bulk_update(vmenu_horizontal_tracker::alignment::Right, ...);There was a problem hiding this comment.
Unfortunately, it requires copy constructor:
1>D:\DEV\FarManager\far\vmenu.cpp(2188,19): error C2280: 'vmenu_horizontal_tracker::cookie::cookie(const vmenu_horizontal_tracker::cookie &)': attempting to reference a deleted function
1> const auto Cookie{
1> ^ (compiling source file '/vmenu.cpp')
1> D:\DEV\FarManager\far\vmenu.hpp(169,3):
1> see declaration of 'vmenu_horizontal_tracker::cookie::cookie'
1> NONCOPYABLE(cookie);
1> ^
1> D:\DEV\FarManager\far\vmenu.hpp(169,3):
1> 'vmenu_horizontal_tracker::cookie::cookie(const vmenu_horizontal_tracker::cookie &)': function was explicitly deleted
1> NONCOPYABLE(cookie);
1> ^
What do you think of this?
const auto Cookie{
m_HorizontalTracker.start_bulk_update(
NewHPos >= 0 ? vmenu_horizontal_tracker::alignment::Left : vmenu_horizontal_tracker::alignment::Right,
NewHPos >= 0 ? NewHPos : TextAreaWidth + NewHPos + 1,
TextAreaWidth,
Policy) };
And it reminded me that the cookie should be either non-movable or should properly implement move operations.
There was a problem hiding this comment.
Unfortunately, it requires copy constructor
Looks like it's a compiler bug 😆
Please ignore my previous comment or use whatever compiles.
| // Everything is relative to menu_layout::TextArea::first (Left edge). | ||
| class vmenu_horizontal_tracker | ||
| { | ||
| struct cookie |
There was a problem hiding this comment.
I'm not sure that "cookie" conveys the intended meaning.
Perhaps saying what it does ("scoped bulk update") oh what it actually does ("hold stray items") could be better?
There was a problem hiding this comment.
It's the name we use throughout in our codebase at work. I think the stress is on it being small and opaque to the caller.
What do you think of bulk_update_scope_guard?
There was a problem hiding this comment.
It feels like caller needs to understand its meaning after all, i.e. "keeping the thing alive enforces some specific behavior". bulk_update_scope_guard sounds good, or maybe just bulk_update.
| // Everything is relative to menu_layout::TextArea::first (Left edge). | ||
| class vmenu_horizontal_tracker | ||
| { | ||
| struct cookie |
There was a problem hiding this comment.
It should be possible to forward-declare this struct and define it in the cpp.
There was a problem hiding this comment.
It is returned by value from start_bulk_update. The question is moot if we remove vmenu_horizontal_tracker from the header. Let me know please.
There was a problem hiding this comment.
It will work if the type definition precedes start_bulk_update definition in the cpp.
Indeed, it's irrelevant if we move the whole thing, I've mentioned just in case we don't for any reason.
| // Keeps track of the horizontal state of all items. | ||
| // The tracking is best effort. See comments below. | ||
| // Everything is relative to menu_layout::TextArea::first (Left edge). | ||
| class vmenu_horizontal_tracker |
There was a problem hiding this comment.
Do we really need all these implementation details exposed in the header?
Maybe forward-declare it and have a unique_ptr to it in the menu?
Yes, it's overhead, but one per menu is nothing.
There was a problem hiding this comment.
Keeping it by std::unique_ptr means an extra allocation. If it's fine with you, I can do that.
There was a problem hiding this comment.
Ouch! Moved the definition of vmenu_horizontal_tracker to vmenu.cpp and changed the type of m_HorizontalTracker to std::unique_ptr<vmenu_horizontal_tracker>. Got a bunch of "can't delete an incomplete type" errors in many other files: https://github.com/MKadaner/FarManager/tree/mzk/gh-802/retrieve-vmenu-alignment-api-WIP
Never mind. Silly me. Working on it.
| namespace | ||
| { | ||
| MenuItemEx far_list_to_menu_item(const FarListItem& FItem) | ||
| const FarListItem string_to_far_list_item(const wchar_t* NewStrItem) |
There was a problem hiding this comment.
Remove const from the return value please.
| { | ||
| NONCOPYABLE(cookie); | ||
| explicit cookie(vmenu_horizontal_tracker* Tracker) noexcept: m_Tracker{ Tracker } {} | ||
| ~cookie() noexcept { m_Tracker->m_IsBulkUpdate = false; } |
There was a problem hiding this comment.
dtors are noexcept by default, do we need to state it explicitly?
| NONCOPYABLE(cookie); | ||
| explicit cookie(vmenu_horizontal_tracker* Tracker) noexcept: m_Tracker{ Tracker } {} | ||
| ~cookie() noexcept { m_Tracker->m_IsBulkUpdate = false; } | ||
| vmenu_horizontal_tracker* m_Tracker{}; |
There was a problem hiding this comment.
It's always initialized in the ctor anyway.
Summary
Implemented
vmenu_horizontal_trackerwhich knows whether menu items are currently aligned.Surfacing the state maintained by
vmenu_horizontal_trackeris coming soon.References
Checklist
If not checked, I accept that this work might be rejected in favor of a different great big ineffable plan.