-
-
Notifications
You must be signed in to change notification settings - Fork 732
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
Support for custom contexts in ContextCommand #727
Conversation
Created to close: #726 |
I like this a lot and I will do a complete review this weekend, because I think it could simplify some existing code we use for the Quick initial: @mahaloz can you also add some docs about it (at least in |
@hugsy yup I can, though there was some discussion with @Grazfather about wether the user should be able to pass some explicit register_external_context_pane(pane_name, pane_function, pane_title_function) Though, I do personally like the idea of dynamic names like in the code pane. |
dynamic pane names makes registration cleaner as well as the layout mapping. I suppose it makes sense to allow users to select which of their contexts they want to show, I also just figured you could register a function and only allow the option to turn off / on ALL user contexts. Obviously this is less powerful but it's much simpler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR, some minor tweaks and we can merge
Co-authored-by: hugsy <hugsy@users.noreply.github.com>
@hugsy thanks :). All review changes have been addressed. I should also note that I modified the api a little since you last looked, so you may want to just check out the api again. I allowed users to pass a function that returns a string now so they can have dynamic pane titles, after discussion with @Grazfather. Feel free to DM me on discord for a immediate response @hugsy. |
@Grazfather take a look at my reply for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor stuff in the docs, you should be able to apply all the suggestions via GH if you agree.
Co-authored-by: hugsy <hugsy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR. LGTM except the one little remark I gave. Also it would be even more cool if you could add some test for this: e.g. add a custom pane with some generated sequences as a title and the pane's content (different ones for each) and then test if those sequences are shown in the output. Just to prevent regression in the future.
Sorry, I added a few more nitpicks |
Co-authored-by: theguy147 <37738506+theguy147@users.noreply.github.com>
@theguy147, I addressed all the requests. |
Custom Context Support
Description/Motivation/Screenshots
This patch adds a new external api:
register_external_context_pane
, which takes three parameters:pane_name
: str. The name that will be printed as the context panes titlepane_function
: function. The function that will be called to display things after the context title. Should use gef_print.pane_title_function
: function. The function returns a string which will be used for the context pane title.To be concise, this new api essentially allows you to pass it an arbitrary function that will be executed after displaying your new context's title. More things could be added to create a wrapper class, but overall this is good enough to allow users to display arbitrary text on each break event while living in the context command.
This patch will make the world better because it will allow users to have more freedom when using GEFs workflow for a specific target which requires a custom environment.
In this screenshot you can see that the last pane in the context is a custom context pane. You can also see that the pane is now a part of the actual context settings config:
Here is an example of the new api usage (also documented in the source):
Take this file and simply run
source
on it like a normal external command in gef.I'll wait on adding the docs until the main devs like the general format of the external api.
How Has This Been Tested?
make test
Checklist
dev
branch, notmaster
.