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

Fixed Hex mode width calculations in Viewer. #825

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

MKadaner
Copy link
Contributor

@MKadaner MKadaner commented Apr 9, 2024

Summary

Fixed a few places where the line width in the Viewer Hex mode was assumed to be 80 even though the number of bytes per line can now be changed by the user.

Also, some renaming and refactoring.

References

0ecc671

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.

Sorry, something went wrong.

@alabuzhev
Copy link
Contributor

Could you please give an example of an issue fixed by this change?

@MKadaner
Copy link
Contributor Author

Could you please give an example of an issue fixed by this change?

Here is a problematic scenario:

  • In the Viewer, open a text file containing a relatively long line, say 350 characters.
  • Press F4 to switch to Hex mode.
  • Press Ctrl+Alt+Left to reduce the width of Hex view to 8 bytes per line.
  • Press F4 again to switch to Text mode and F2 to disable line wrap.
  • Resize Far window width to 30 columns.
  • Press Ctrl+Shift+Right to position the screen to the end of the longest line.
  • Press F4 to switch to Hex mode.

Observe that the Far window is empty, and the Hex dump completely disappears. In fact, the dump now is far beyond the left edge of the window.

One can argue that this issue is "by design," because the Viewer tries to preserve the horizontal position of the content while switching between view modes. However, the ostentatious intent of the existing code is to make sure the Hex dump is always visible. I think that the latter behavior is more important and should override "preserving horizontal position."

Another concern is Viewer::XYfilepos, case VMT_HEX. The code here translates the X mouse click coordinate to the byte offset. The code obviously assumes that Hex dump is always 16 bytes per line. With user-customizable number of bytes per line in Hex dump, it is simply not true. Unfortunately, I cannot describe the scenario when this issue becomes visible.

More importantly, I think that any particular visualization issue notwithstanding, it is plain wrong to have the width of Hex view hard coded to 80 columns in several places while the actual view width is variable.

Last but not the least. I stumbled upon this issue while I was about to introduce a far:config to reverse the horizontal scrolling direction. I absolutely could not understand what was going on in two or three places in Viewer implementation, so I could not easily generalize the code to handle the swap direction option. I spent substantial time to reverse engineer the code. This PR is the outcome of this effort.

Also, some renaming and refactoring.
@MKadaner MKadaner force-pushed the mzk/viewer-hex-mode-renaming branch from 9ed1264 to d79a327 Compare April 10, 2024 04:33
Copy link

Quality Gate Passed Quality Gate passed

Issues
11 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@MKadaner
Copy link
Contributor Author

I looked through sonarcloud's warnings. All of them are preexisting issues except for:

Let me know, if you want me to fix any of these issues, even preexisting. I'll do your bid.

@alabuzhev
Copy link
Contributor

Please ignore those warnings.

@alabuzhev
Copy link
Contributor

I absolutely could not understand what was going on in two or three places in Viewer implementation

Ah yes, it's... tangled, to put it mildly.
Thanks.

@alabuzhev alabuzhev merged commit 61dcdac into FarGroup:master Apr 10, 2024
51 of 52 checks passed
@MKadaner MKadaner deleted the mzk/viewer-hex-mode-renaming branch April 10, 2024 20:55
@MKadaner
Copy link
Contributor Author

Thank you.

@rohitab
Copy link
Contributor

rohitab commented Apr 10, 2024

Another concern is Viewer::XYfilepos, case VMT_HEX. The code here translates the X mouse click coordinate to the byte offset. The code obviously assumes that Hex dump is always 16 bytes per line. With user-customizable number of bytes per line in Hex dump, it is simply not true. Unfortunately, I cannot describe the scenario when this issue becomes visible.

The issue becomes visible when you use the mouse to select bytes in the Hex dump view. If the bytes-per-line are anything other than 16, the selection starts and ends at the wrong position. For example:

  1. Open a file in Hex view
  2. Press Alt+Left to reduce the bytes-per-line to 7
  3. Press Shift+Left_Click on the byte 1 in the second line. Note that the byte 2 gets selected instead.
  4. Press Shift+Left_Click on the byte 8 in the second line.

You'll see that bytes 2-9 get selected, instead of bytes 1-8.

Unfortunately, this issue is still present in version 3.0.6304.0 x64

MKadaner added a commit to MKadaner/FarManager that referenced this pull request Apr 11, 2024
@MKadaner
Copy link
Contributor Author

Ouch! Silly me. Fixed (hopefully) in #826.

Thank you for reporting, @rohitab.

alabuzhev added a commit that referenced this pull request Apr 11, 2024
…tion

Fixed fallout of 6304 (reported in gh-825).
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.

None yet

3 participants