Ctrl+Numpad5 vertically aligns found text in "Find All" Editor menu.

[MKadaner]

Summary

Ctrl+Numpad5 in Editor menu "All matching entries" vertically aligns found text in all items. To preserve vertical alignment while scrolling items horizontally, the items can be moved beyond the left or right window edges.

References

This PR is a replacement of #711. See the discussion there for the motivation and design choices.

I decided against automatically aligning the items on open. This would be an unwarranted behavior change, while it requires only a single keystroke to align items manually, and it can even be automated with a macro if somebody wishes it.

Checklist

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

[alabuzhev]

2. New far:config VMenu.SwapHScrollDirection changes the meaning of left and right keys in all menus and lists. See far:config for details.

I don't mind, but isn't our default policy "use macros if you want to redefine keys"?

[MKadaner]

[I]sn't our default policy "use macros if you want to redefine keys"?

My personal preferences aside, I was going to write a macro and add it to the deployment kit. However, I wanted to restrict this macro to VMenus only, because redefining arrow keys with practically all combinations of modifiers in a wider area looks like a bad idea. Apparently, there is no macro area for VMenu. Do we want to add it?

Let's keep it as far:config unless somebody here insists on a macro. And if so, please help me defining the more in a generic way, so that I do not need to repeat it for all combinations of Ctrl, Shift, and Alt.

[MKadaner]

Taking it back. MACROAREA_MENU should work.

Since I don't like macros and the current behavior is counterintuitive anyway, let me flip the default and add the macro for those who what the old directions.

[alabuzhev]

let me flip the default

This will make a lot of people very angry.

[MKadaner]

This will make a lot of people very angry.

Yeah! I know. I poked around with macros; like it even less. I'll go far:config for now.

[alabuzhev]

May I suggest splitting the work into two commits, one with vertical alignment and one with keys swapping?

Maybe even move keys swapping to a separate PR.
We have other similarly scrollable UI elements, e.g. file names in panels, so it might be better to cover everything in one go and with a single switch (or a set of macros).

[MKadaner]

I can do that.

[MKadaner]

Done! Please review.

[MKadaner]

Fixed commit and PR descriptions.

[HamRusTal]

@MKadaner @alabuzhev Pardon, it's not quite clear from the discussion, are you gentlemen going to reverse the horizontal scrolling direction by Alt+Left/Right keys?
If so, what is the justification for such a change? LOTS of FAR users have muscle memory for the current direction, and, more importantly, the current keys assignment for horizontal scrolling is uniform with that for vertical and horizontal scrolling (e.g. in the viewer) — when you press a scrolling shortcut with an arrow you see more text in that direction. Breaking this invariant would be a VERY bad idea.

[MKadaner]

@HamRusTal, I apologize for the confusion. In this PR I reverted all changes related to swapping the direction of horizontal scrolling. These changes clearly do not belong here. I am preparing another PR which will introduce a new far:config for swapping the direction across all UI elements with the default to the existing behavior. Let's decide on the merits there.

[MKadaner]

Rebased and ready for review. @alabuzhev, please have a look.

[MKadaner]

Can I help somehow with the review, like adding comments to new functions or describe the general approach here?

[alabuzhev]

Can I help somehow with the review, like adding comments to new functions or describe the general approach here?

It's fine, I just didn't have much time during the week.

[alabuzhev]

Is H for Horizontal?
Maybe add a few more letters to avoid confusion with Highlight?

[MKadaner]

I went with HorizontalPosition. I thought about renaming all other identifiers containing "hpos" but decided against it because it becomes mouthful. Let me know if you like something else renamed.

[alabuzhev]

Sonarcloud complains here, and technically quite rightfully:

1. By forwarding the argument we permit the callee to steal it.
2. The callee takes the parameter by value, so the parameter ctor technically can take advantage of that and steal from rvalue.
3. And then we pass this potentially empty thing again on the next iteration.

Given that everything here is local and we know exactly what comes in and don't need to store these callbacks or anything like that, I suggest keeping things simpler and passing callbacks by const auto& everywhere.

[MKadaner]

Ouch! I've seen Sonarcloud's complaint but missed the fact that the forwarding happens in a loop. Thank you for checking.

[sonarcloud]

Quality Gate passed

Issues
4 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[alabuzhev]

Thanks

[MKadaner]

Thank you!

[rohitab]

@MKadaner would it be possible to use Ctrl+Numpad5 to toggle this on/off? Unless I'm missing something, the only way to get back the original alignment, without manually realigning the items, is to close the dialog and repeat the search.

Thank you for implementing this. It's really useful.

[alabuzhev]

@rohitab Alt+Home should work.

[rohitab]

@alabuzhev Nice! I didn't even know about that hotkey. Thank you.

[HamRusTal]

@MKadaner Is it by intent that Ctrl+Clear/Ctrl+Numpad5 did not get its way into the menu's bottom frame?
That would have served better discoverability of the new feature.

Also, I wonder if it is easy to change Gray + there to Gray+, otherwise it looks like two keys: Gray (WTF) and + (OK).

[MKadaner]

@rohitab, Alabuzhev had beaten me with reply for a few hours. Sure, Alt+Home / Alt+End was there all the time.

@HamRusTal, I did not touch the legend on this dialog. Lots of other useful key combinations, like Alt+Home and friends, are missing. All of them are listed in the help article, and of course, the legend is easy to change, except for screen width severely limits our eloquence here. May I ask you to create a separate issue to improve the legend on the "Find all" dialog, and propose more specific text?

[rohitab]

@MKadaner I was going to write a macro to toggle the state, but there doesn't seem to be any way to determine the current state. Obviously, there are workarounds, but I think making a small change in Far might be better. Something like the following works to toggle the state from aligned to left and vice-versa. What do you think?

diff --git a/far/vmenu.cpp b/far/vmenu.cpp
index 0b54ceb55..004e6f569 100644
--- a/far/vmenu.cpp
+++ b/far/vmenu.cpp
@@ -1649,7 +1649,7 @@ bool VMenu::ProcessKey(const Manager::Key& Key)
 		case KEY_CTRLNUMPAD5:
 		case KEY_RCTRLNUMPAD5:
 		{
-			if (AlignAnnotations())
+			if (AlignAnnotations() || SetAllItemsSmartHPos(0))
 				DrawMenu();
 
 			break;

This has another benefit too. Say, I pressed Ctrl+Numpad5 to align the items, then want to check the line number (which is not visible), I can quickly tap the same keys again, check the line number, then tap them again to return to the aligned view, without having to switch between different key combinations.

[HamRusTal]

@MKadaner Do you have any idea how feasible it is to make the line numbers and their vertical separator line stay put on the left when scrolling the menu items? As they were scrolling all in accord, that looked OK but now when the lines are aligned by the found text, these prefix fragments are scattered all around and look quite confusing IMHO.
At least some menus have non-scrollable portions of their items already: e.g. check marks in history menus, A (ANSI) marks in plugin configuration menu, etc. Maybe this can be generalized and reused in the menu at hand?

[MKadaner]

@rohitab, the things will become less obvious if the user aligns all annotations and then shifts one or more individual items left or right. Now, the annotations are not aligned, so the first Ctrl+Numpad5 will restore the annotation alignment instead of aligning everything to the left, as the user likely expected. It will become even messier if after shifting an individual item horizontally the user scrolls the entire list vertically moving the unaligned item out of the viewport.

Thinking of the "Find all" dialog after Ctrl+Numpad5 as if it appears in a special state, is a misconception. The things are actually much simpler. List items can be shifted left and right (almost) freely. There are several accelerators to put all items in some interesting position (align all left or right, align all annotations). The result is not a state. It just so happened that all items visually form a nice two-dimentional shape, nothing more. In this sense, there is no way to toggle anything. Switching between left-aligned shape and annotation-aligned shape cannot be done with the same key combination.

I actually spent a few months experimenting with quite different behaviors of the list after aligning annotation. One of the behaviors I explored was the kind of state you are suggesting. I showed and discussed different behaviors with a couple of Far users I could communicate directly. Please believe me that the behavior I finally implemented is the most predictable and easy to use, though it also is not free of quirks.

I can quickly tap the same keys again, check the line number, then tap them again to return to the aligned view, without having to switch between different key combinations.

I see how this quick switching back and forth is convenient. I like such kind of behavior myself. However, if we support it, we'll incur other subtle inconsistencies. The real problem here is that line numbers can be shifted out of the viewport, while they of course should stay put, like in Excel spreadsheet for example. It is this problem that should eventually be addressed. So, please read on.

P.S. The solution you proposed, while really elegant, is suboptimal. Think of a list with hundredths of thousands of items (which is my regular use case). It is rather expensive to scan all items (inside AlignAnnotations()) only to find out that all annotations are already aligned. If we would go this way, we should maintain a bool IsAligned member variable.

[MKadaner]

@HamRusTal, this behavior is a damn nuisance I have been thinking about for years. Actually, even before my change, one could align all items to the right (Alt+End) and line numbers would become scattered all over the place. I was enjoying this sight pretty often when I used Alt+End as a poor man's substitute for aligned annotations. As such, I hold that I did not break anything. However, I admit that I did not realize it could become more of an issue for somebody else.

For me personally, aligning annotations has much higher priority than scattered line numbers, so it went first. Now aligning is done, I am warming up for line numbers. I can see at least two solutions:

1. Let each list item have more than one content string and draw these strings within their own vertical boundaries set up for the entire list.
2. Create a new dialog called Grid which would be similar to VMenu2 but would have more than one list. Vertical scrolling should affect all lists synchronously, while horizontal scrolling should be applied to the focused list only. Maybe just extend VMenu2 to contain more than one VMenu.

The resulting UX will be more or less the same. As it usually happens, each option has its own pros and cons. So, stay tuned.

[rohitab]

@MKadaner thank you for taking the time to explain in detail.

if the user aligns all annotations and then shifts one or more individual items left or right. Now, the annotations are not aligned, so the first Ctrl+Numpad5 will restore the annotation alignment instead of aligning everything to the left, as the user likely expected.

I actually don't see anything wrong with this behavior. If items were shifted, and the user pressed Ctrl+Numpad5, the user should expect the annotations to be aligned since they were out of alignment. What I meant by toggle was that, if the annotations were aligned, they will be shifted left. If they were unaligned, they should be aligned.

It will become even messier if after shifting an individual item horizontally the user scrolls the entire list vertically moving the unaligned item out of the viewport.

I see how this could be a problem. In this case, when the user pressed Ctrl+Numpad5, the items would get aligned, but to the user it would appear as if nothing happened, but that is essentially the same as how it currently works. If the user now pressed Ctrl+Numpad5 again, the items would get aligned left, as expected.

I showed and discussed different behaviors with a couple of Far users I could communicate directly. Please believe me that the behavior I finally implemented is the most predictable and easy to use, though it also is not free of quirks.

I honestly don't see any downside to what I'm suggesting. For users that like the current implementation, it should not change anything. All the existing hotkeys, Ctrl+Numpad5, Alt+Home and Alt+End would work like they currently do. The only change the users would see is if they pressed Ctrl+Numpad5 when the annotations were already aligned. But why would they, if they aren't expecting the alignment to change?

However, I'm not going to push this issue further. If you and other Far users believe this is the best implementation, I'm good with it.

It is rather expensive to scan all items (inside AlignAnnotations()) only to find out that all annotations are already aligned. If we would go this way, we should maintain a bool IsAligned member variable.

It was my understanding that this already happens. Each time you press Ctrl+Numpad5, AlignAnnotations() is called. But you're the expert here, so I'll leave all implementation details to you.

[MKadaner]

@rohitab, thank you for the explanations.

I see how this could be a problem. In this case, when the user pressed Ctrl+Numpad5, the items would get aligned, but to the user it would appear as if nothing happened, but that is essentially the same as how it currently works. If the user now pressed Ctrl+Numpad5 again, the items would get aligned left, as expected.

The UX problem will be that when all visible annotations are aligned and the user presses Ctrl+Numpad5, sometimes the list will get aligned to the left, while other times nothing happens, and the user will have to press Ctrl+Numpad5 again to left-align the list. It may become very annoying depending on how often the user shifts individual items and scrolls the list.

There is another, more hypothetical issue though. Did you notice that if the pattern is found on the same line more than once, currently Far inserts so many items in the "Find all" list, each with its unique position number (next to the line number). I actually hate this behavior. More than once was I deceived into thinking that there were many matching lines in the text, while in reality the lines were few, but the occurrences were many. Sure, I had to look at the list header where both the number of lines and the number of occurrences are clearly printed, but... that happened to me again and again.

As such, I want to change the UX and show in the list one item for each matching line with all found occurrences highlighted. If I do so, how to align annotations? I thought about using repeated Ctrl+Numpad5 presses to rotate the aligned annotation through all occurrences on the same line. Then, of course, there will be a question of what to do when different lines have different number of annotations. Granted, if we do something on these lines, we can include the "Home" position in the rotation, so that once in a while Ctrl+Numpad5 aligns left, and all other times aligns some annotations. All this is rather vague in my mind yet.

May I ask you to create a new discussion for "toggling" between "align left" and "align the found" with Ctrl+Numpad5? At the very least, I want more opinions on the subject. Again, the uncertainty of how many times one has to press Ctrl+Numpad5 to get home may be very annoying.

It is rather expensive to scan all items (inside AlignAnnotations()) only to find out that all annotations are already aligned. If we would go this way, we should maintain a bool IsAligned member variable.

It was my understanding that this already happens. Each time you press Ctrl+Numpad5, AlignAnnotations() is called. But you're the expert here, so I'll leave all implementation details to you.

Consider the normal scenario.

1. The first Ctrl+Numpad5 invokes AlignAnnotations() which scans all items and aligns annotations. This is inevitable.
2. With the implementation you proposed, the second Ctrl+Numpad5 invokes AlignAnnotations() which scans all items again only to find that everything is aligned and return false.
3. Now, the right part of operator|| has to be evaluated, so SetAllItemsSmartHPos(0) is called and scans all items again to set their positions to zero.

In this scenario, the item 2 above is pure overhead, and not a small one because AlignAnnotations() is relatively expensive. The problem is that AlignAnnotations() does many interesting things with each item no matter whether the item will need to be shifted or not. On the other hand, it is pretty cheap and simple to maintain the bool IsAligned flag which will indicate whether we need to call AlignAnnotations() or SetAllItemsSmartHPos(0) when the user pressed Ctrl+Numpad5.

[rohitab]

May I ask you to create a new discussion for "toggling" between "align left" and "align the found" with Ctrl+Numpad5?

Please see #802