Skip to content

Resource Tree Bytes and Strings Search Bar #334

Closed
dannyp303 wants to merge 32 commits into
redballoonsecurity:masterfrom
dannyp303:feature/search-bar
Closed

Resource Tree Bytes and Strings Search Bar #334
dannyp303 wants to merge 32 commits into
redballoonsecurity:masterfrom
dannyp303:feature/search-bar

Conversation

@dannyp303
Copy link
Copy Markdown
Contributor

One sentence summary of this PR (This should go in the CHANGELOG!)
Add a search bar to the ResourceTreeView to filter resources by string and bytes.

Link to Related Issue(s)

Please describe the changes in your request.

Anyone you think should look at this, specifically?
@rbs-jacob @Edward-Larson

@dannyp303 dannyp303 requested a review from rbs-jacob June 26, 2023 15:06
Comment thread frontend/src/ResourceSearchBar.svelte Outdated
Comment thread frontend/src/ResourceSearchBar.svelte Outdated
Comment thread frontend/src/ResourceSearchBar.svelte
Comment thread frontend/src/ResourceTreeNode.svelte Outdated
Comment thread frontend/src/ResourceSearchBar.svelte Outdated
Comment on lines +82 to +92
on:keyup|preventDefault="{async (e) => {
if (searchType == 'String') {
searchFilter = await rootResource.search_for_string(
searchQuery,
regex,
caseIgnore
);
} else if (searchType == 'Bytes') {
searchFilter = await rootResource.search_for_bytes(searchQuery, false);
}
}}"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should probably have some form of try/catch type error handling if the query fails with an exception. Does it make sense to use alert and pop up an error? Or does it make sense to just log to the console? Something else?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pop up alert doesn't make sense now that i'm using on:keyup, it would probably spam the browser.

Comment thread ofrak_core/ofrak/gui/server.py Outdated
Comment on lines +760 to +762
offsets = await resource.search_data(string_query)
if string_query == "":
return json_response(None)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason to return None instead of an empty list here?

Also, if string_query == "", I think it makes sense to return earlier than this. Not only does it make sense to return before actually doing the search (to avoid the expensive operation of searching for an empty string that will be present between every single character), but it probably makes sense to return before compiling the regular expression as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the first part, yes. An empty list should be the result of a search with no results. Otherwise it isn't apparent that your search is empty. And yeah you're correct, I did this fast this morning.

Copy link
Copy Markdown
Member

@rbs-jacob rbs-jacob Jun 26, 2023

Choose a reason for hiding this comment

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

First part sounds good, makes sense to return None. I'll leave this thread unresolved until the second part is fixed.

Comment thread ofrak_core/ofrak/gui/server.py Outdated
Comment thread ofrak_core/ofrak/gui/server.py
Comment thread ofrak_core/ofrak/gui/server.py Outdated
Comment thread ofrak_core/test_ofrak/unit/test_ofrak_server.py
Comment thread frontend/src/ResourceSearchBar.svelte Outdated
Comment thread frontend/src/ResourceSearchBar.svelte Outdated
Comment thread ofrak_core/ofrak/gui/server.py
Co-authored-by: Edward Larson <wasabiofip@gmail.com>
dannyp303 and others added 2 commits June 27, 2023 17:59
Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
@dannyp303 dannyp303 requested a review from rbs-jacob June 27, 2023 22:03
@rbs-jacob rbs-jacob mentioned this pull request Jun 30, 2023
1 task
@EdwardLarson EdwardLarson mentioned this pull request Jul 3, 2023
1 task
@EdwardLarson
Copy link
Copy Markdown
Contributor

Folded into #345

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