Refactor/hexview - Githubissues

redballoonsecurity / ofrak

OFRAK: unpack, modify, and repack binaries.

https://ofrak.com

Other

1.82k stars 128 forks source link

Refactor/hexview #427

Closed dannyp303 closed 4 months ago

dannyp303 commented 4 months ago

[X] I have reviewed the OFRAK contributor guide and attest that this pull request is in accordance with it.

One sentence summary of this PR (This should go in the CHANGELOG!)

Refactor HexView and related components to use mousewheel instead of scroll and compartmentalize all comonents to src/hex. Link to Related Issue(s)

Please describe the changes in your request.

Anyone you think should look at this, specifically?

dannyp303 commented 4 months ago

One additional observation about the pane changes is that the vertical and horizontal scrolls are handled differently in a way that I think is potentially confusing for a user. Specifically: there are horizontal scroll bars, but they're way at the bottom and can go out of view. Also if you middle click in the tree with the mouse wheel, it only lets you scroll horizontally.

Similarly, horizontal scrolling the hex view doesn't work if it's too small.

I've included a video, but I don't consider fixing the horizontal scroll critical for this PR. If it's an easy fix, we should do it. But it shouldn't block the PR getting merged.

Screen.Recording.2024-02-27.at.5.37.07.PM.mov

Horizontal scrolling here would be a bit weird, I think the best solution would be to adjust the alignment size based on the hexview element width. In the interest of time, i think this should be a separate PR.

rbs-jacob commented 4 months ago

Horizontal scrolling here would be a bit weird, I think the best solution would be to adjust the alignment size based on the hexview element width. In the interest of time, i think this should be a separate PR.

I'm more concerned about horizontal scrolling in the tree view being weird than about not being able to horizontally scroll the hex view. The only reason I bring them up together is that I think they're likely related.

Like I said, no need to block this PR on that.

dannyp303 commented 4 months ago

Horizontal scrolling here would be a bit weird, I think the best solution would be to adjust the alignment size based on the hexview element width. In the interest of time, i think this should be a separate PR.

I'm more concerned about horizontal scrolling in the tree view being weird than about not being able to horizontally scroll the hex view. The only reason I bring them up together is that I think they're likely related.

Like I said, no need to block this PR on that.

Not related, the hex view doesnt "scroll" so i have scrolling totally turned off for that view, if I enable it, it does the "double scroll" that initiated this entire PR. Regardin the resource view, easy fix, theres an overflow-x: auto in the treebox div that isnt required any more.