Feature: GUI hex search#336
Conversation
… probably doesnt belong there
rbs-jacob
left a comment
rbs-jacob
left a comment
There was a problem hiding this comment.
Requested changes.
I just reviewed by looking over stuff. I figure I'll actually run it, and probably make some CSS adjustments, after this first round of changes goes in. That way we don't spend time changing CSS when functionality will change, possibly rendering the CSS work obsolete.
rbs-jacob
left a comment
rbs-jacob
left a comment
There was a problem hiding this comment.
Actually ran it this time. Found a few small bugs/issues in terms of UI behavior, and made some CSS change recommendations. In the interest of learning, I did not commit them directly to the branch, but tried to break them out into PR comments that explain and justify.
| .searchbar { | ||
| display: flex; | ||
| align-items: left; | ||
| flex-grow: 1; | ||
| height: 2em; | ||
| position: sticky; | ||
| padding-bottom: 1em; | ||
| } |
There was a problem hiding this comment.
| .searchbar { | |
| display: flex; | |
| align-items: left; | |
| flex-grow: 1; | |
| height: 2em; | |
| position: sticky; | |
| padding-bottom: 1em; | |
| } | |
| .searchbar { | |
| display: flex; | |
| flex-direction: row; | |
| flex-wrap: nowrap; | |
| justify-content: flex-start; | |
| align-items: stretch; | |
| align-content: center; | |
| height: 2em; | |
| margin-bottom: 1em; | |
| } |
| function nextMatch() { | ||
| let nextIndex = searchResults.index + 1; | ||
| if (nextIndex >= searchResults.matches.length) { | ||
| nextIndex = 0; | ||
| } | ||
| searchResults = { matches: searchResults.matches, index: nextIndex }; | ||
| } | ||
|
|
||
| function prevMatch() { | ||
| let nextIndex = searchResults.index - 1; | ||
| if (nextIndex < 0) { | ||
| nextIndex = Math.max(searchResults.matches.length - 1, 0); | ||
| } | ||
| searchResults = { matches: searchResults.matches, index: nextIndex }; | ||
| } |
There was a problem hiding this comment.
There is a bug with these functions where it jumps one line above a location first, before jumping to the location on the second click.
Seems to be an issue exclusively when there is only one result found.
Screen.Recording.2023-06-30.at.4.46.10.PM.mov
There was a problem hiding this comment.
It's not just if there is only result found, the result has to be exactly at the start of a row. Honestly this sort of off-by-one error seems like it will take more time to fix than it's worth. The search bar still jumps basically to the match, so to the user experience is barely impacted. But this is definitely not intended behavior.
| function nextMatch() { | ||
| let nextIndex = searchResults.index + 1; | ||
| if (nextIndex >= searchResults.matches.length) { | ||
| nextIndex = 0; | ||
| } | ||
| searchResults = { matches: searchResults.matches, index: nextIndex }; | ||
| } | ||
|
|
||
| function prevMatch() { | ||
| let nextIndex = searchResults.index - 1; | ||
| if (nextIndex < 0) { | ||
| nextIndex = Math.max(searchResults.matches.length - 1, 0); | ||
| } | ||
| searchResults = { matches: searchResults.matches, index: nextIndex }; | ||
| } |
There was a problem hiding this comment.
Another strange characteristic is the way that multiple contiguous regions can be matched multiple times. Personally, I think this behavior should be considered a bug. Even if it's intended, it's unintuitive for the user what's going on.
Maybe the particular instance of the found string that corresponds to the latest find should be highlighted in a different color? Kind of how Ctrl+f in the browser will highlight all the instances it's found, but highlight the current one in a different color.
Screen.Recording.2023-06-30.at.4.59.40.PM.mov
There was a problem hiding this comment.
I did a simple implementation to try to address this, just underlining the bytes if the range they match (from the binary search) is the same index as the selected range. It sort of works, but doesn't work well for exactly the case being raised here: Multiple matching ranges overlapping, since not all of the bytes in the currently selected range are highlighted. I would guess that the binary search finds one of the overlapping ranges instead, not prioritizing the selected one.
So I think it would actually considerably complicate the logic, unless you see another simple way to try it. I think we should leave this as future work.
|
Folded into #345 |
4 participants
One sentence summary of this PR (This should go in the CHANGELOG!)
Add searchbar to HexView in order to search for a string or bytes within a resource.
Link to Related Issue(s)
Please describe the changes in your request.
Anyone you think should look at this, specifically?
@rbs-jacob
@dannyp303