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

Refactor/hexview #427

Merged
merged 49 commits into from
Feb 28, 2024
Merged

Conversation

dannyp303
Copy link
Collaborator

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 dannyp303 requested a review from rbs-jacob February 21, 2024 22:28
Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some outstanding issue that should be fixed before this can be merged.

  • Likely due to Pane refactor, this PR removes padding all over the GUI that should be fixed
  • The search bar in the tree view is no longer pinned to the top, and scrolls away when you scroll the tree
  • The minimap now floats half way between the right of the hex view and the right of the screen when it should be pinned to the right of the screen
  • The minimap no longer takes up the full height of the screen, which makes it less practical to use, less easy to see details, etc.
  • The minimap and "go to offset" input no longer have a black background, and the hex can be seen around/behind them if the hex area is thin enough to cause overlap
  • The "breadcrumb" that used to be at the top of the hex view is no longer visible (it does seem to come back in the disassembly view for some reason)
  • The hex view does not allow scrolling past the bottom when using the scroll wheel – I know there is a comment in the code that this is a bug, but I actually ended up kind of preferring it this way
  • Scrolling with the mini map does not round to the nearest line correctly (see video at the bottom)
  • The bottom of the red "view window" in the minimap is not opaque
  • Scrolling in the disassembly view no longer works
  • Scrolling in the text view no longer works

Before:
image

After:
image

Scrolling not working right:

Screen.Recording.2024-02-23.at.2.03.08.PM.mov

Left button bar lack of background:

image

Minimap needs background, and should be full height:

image

frontend/src/stores.js Outdated Show resolved Hide resolved
frontend/src/utils/SearchBar.svelte Outdated Show resolved Hide resolved
frontend/src/hex/ByteclassView.svelte Outdated Show resolved Hide resolved
frontend/src/hex/HexView.svelte Outdated Show resolved Hide resolved
frontend/src/hex/HexView.svelte Outdated Show resolved Hide resolved
frontend/src/hex/HexView.svelte Outdated Show resolved Hide resolved
frontend/src/hex/MinimapView.svelte Outdated Show resolved Hide resolved
frontend/src/utils/Pane.svelte Show resolved Hide resolved
Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edge case that may not be worth worrying about, but the minimap gets cut off if the hex view gets too small.

Screen.Recording.2024-02-26.at.4.01.04.PM.mov

frontend/src/resource/ResourceTreeNode.svelte Outdated Show resolved Hide resolved
frontend/src/views/ComponentsView.svelte Outdated Show resolved Hide resolved
frontend/src/utils/Pane.svelte Outdated Show resolved Hide resolved
frontend/src/ofrak/remote_resource.js Outdated Show resolved Hide resolved
frontend/src/hex/JumpToOffset.svelte Outdated Show resolved Hide resolved
frontend/src/hex/HexView.svelte Outdated Show resolved Hide resolved
frontend/src/hex/HexView.svelte Outdated Show resolved Hide resolved
Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

frontend/src/hex/HexView.svelte Outdated Show resolved Hide resolved
@dannyp303
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

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.

@dannyp303 dannyp303 requested a review from rbs-jacob February 28, 2024 21:33
Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all of the revisions. Looks good to me! 🤑

@rbs-jacob rbs-jacob merged commit e4b6e83 into redballoonsecurity:master Feb 28, 2024
5 checks passed
ANogin pushed a commit to ANogin/ofrak that referenced this pull request May 30, 2024
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.

2 participants