Skip to content
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.

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

Added AlignAnnotations to VMemu. #711

Closed
wants to merge 4 commits into from

Conversation

MKadaner
Copy link
Contributor

@MKadaner MKadaner commented Jul 5, 2023

Summary

This draft PR proposes a new feature: vertically aligning found patterns in the Find All list. Now VMenu will align items' annotations vertically on pressing caret character (^).

I, for one, very much need this feature to visually compare the near contexts of the found patterns in a large file. I am especially missing this feature when I analyze large log files and want to find something unusual in the list of otherwise similar log lines.

The new behavior can be experimented with on my dev branch. Any feedback is highly appreciated.

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 have a few questions right away:

  • The behavior of scrolling items horizontally (Alt+Arrow, Alt+Shift+Arrow) changed. Currently, short items do not scroll at all, and long items stick to the list's vertical edges, so that there is never "empty" space between the edge and the item text. With this PR, items can be scrolled far beyond the edges until one last character is visible. This behavior is very natural and desirable when scrolling horizontally the list with aligned annotations. However, it looks very strange on regular menus. Does it make sense to tune this behavior? Maybe add a new VMENU_FLAG?
  • For now, to experiment with this new feature, I used caret character (^) as a shortcut key. Apparently, it is not very convenient to type (long hand moving). I'd prefer using Numpad5 or maybe Ctrl+Numpad5 but for some reason Numpad5 is not passed to VMenu from Dialog::ProcessKey. Are there any reasons for this limitation? Would it be OK to enable Numpad5 for VMenu?
  • Actual code changes in this PR are pretty hacky and for demonstration purposes only. I'd like to seriously rewrite VMenu::ShowMenu. Are there any reasons to print text piecemeal changing color as we go, like here? The code will become much simpler if we first print the entire line in normal color, and then print hotkeys and annotations over while setting the print position as necessary. It does not look like the function GotoXY is expensive. I can see that ScreenBuf::Write takes a lock, but with this approach we will have less calls to ScreenBuf::Write. What do you think?
  • In this PR, the new command vertically aligns the first annotation. What should be done with subsequent ones? Currently the only place where annotations are used is the Find All list, so the question is moot. However, in general something should probably be done. Any thoughts?

@HamRusTal
Copy link
Contributor

HamRusTal commented Jul 5, 2023

Caret as a shortcut is far from ideal because it's a regular character one might wish to use to further filter the menu contents using the built-in menu filtering functionality. I suggest Ctrl+Shift+A or Ctrl+Alt+A (from “Align”).
Please note that shortcuts with single Alt are not good either because they are normally used to activate menu items' hotkeys.

@alabuzhev
Copy link
Contributor

I wanted to do something like this back in 2012. The only reason I didn't was my laziness.

General thoughts:

  • I don't think we need to do it on a hotkey, aligning by default and unconditionally is good enough.
  • If we align, this menu won't need horizontal scrolling: showing a limited number of characters to the left and to the right is good enough.
  • This doesn't really belong in VMenu: annotations is a private feature that is not used anywhere else and is only housed in VMenu for convenience. Editor could simply supply a list of prealigned string fragments.

@MKadaner
Copy link
Contributor Author

MKadaner commented Jul 5, 2023

Caret as a shortcut is far from ideal...

Sure! As I said, it's for experimenting only. But how about Ctrl+Numpad5? To me, it feels like this operation somewhat resembles centering panels.

@alabuzhev, I strongly disagree. I use horizontal scrolling in the Find All list left and right. This is why I so strongly insisted on adding fast scrolling. Once again, my most important use case is filtering tall and wide log files and analyzing the result. I eventually want something like TextAnalysisTool.NET in Far (the tool is horribly glitchy and not supported anymore). Granted, full functionality is better to implement as a plugin. However, I am not sure if I will ever have enough drive for that, while the Find All list is almost enough. The only two things I miss are this aligning command and showing each found line exactly once with multiple annotations. Both are easy to implement. Well, maybe saving the filtered lines to a file, but that is probably unrelated to VMenu.

Aligning by default on open is an interesting idea. I'll consider it.

@sonarcloud
Copy link

sonarcloud bot commented Oct 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 29 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link

sonarcloud bot commented Dec 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 29 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@MKadaner
Copy link
Contributor Author

MKadaner commented Feb 3, 2024

Closing in favor of #789.

@MKadaner MKadaner closed this Feb 3, 2024
@MKadaner MKadaner deleted the mzk-vmenu-align-annotation branch February 3, 2024 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants