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

Let users mark notable points in target asm #600

Open
abaresk opened this issue Dec 10, 2022 · 10 comments
Open

Let users mark notable points in target asm #600

abaresk opened this issue Dec 10, 2022 · 10 comments

Comments

@abaresk
Copy link
Contributor

abaresk commented Dec 10, 2022

As you're decompiling, you sometimes have to scroll through the asm to find e.g. branch destinations, register assignments, etc. I often find it easy to lose place of where I was before I started scrolling, especially since asm instructions can all sort of blend together.

It'd be helpful to allow users to set some indicator in the diff-view gutter to mark notable parts of the asm. I'm roughly thinking of a UX that's similar to how VSCode lets you mark breakpoints in code (screenshot), but perhaps using more of an arrow-like shape.

@bates64
Copy link
Member

bates64 commented Dec 10, 2022

See also #115

@bates64
Copy link
Member

bates64 commented Dec 10, 2022

Also sounds very similar to #128

@bates64
Copy link
Member

bates64 commented Dec 10, 2022

Do you think the owner of the scratch only should be able to mark points on the target asm? Should they persist across all instances of the target asm or just for the scratch in question (e.g. if forked, would they persist)?

I'm thinking that it might work to do something like this, where user avatars are shown:
image
Then you can click on the avatar to view the comment thread / name of the "notable point"

@ethteck
Copy link
Member

ethteck commented Dec 10, 2022

we were thinking of this initially as a frontend-only feature. but we could add backend stuff to it, just not sure how necessary it is

@ethteck
Copy link
Member

ethteck commented Dec 10, 2022

and no this shouldn't persist in forks either imo

@abaresk
Copy link
Contributor Author

abaresk commented Dec 10, 2022

I think ideally though you'd be able to set "notable points" without needing to name them, since I think that could disrupt the flow a bit as you're decompiling.

I wonder if perhaps this feature should be separate from the commenting feature? This features only requires a single UI icon per line (it's just quick on/off toggle). But with comments, you might want to eventually support multiple comment threads for the same line. In which case, it might be better for these to be decoupled.

@bates64
Copy link
Member

bates64 commented Dec 11, 2022

we were thinking of this initially as a frontend-only feature. but we could add backend stuff to it, just not sure how necessary it is

I think this feature wouldn't be that useful if it were session-only. Navigating away from the page shouldn't clear these

@bates64
Copy link
Member

bates64 commented Dec 11, 2022

I think ideally though you'd be able to set "notable points" without needing to name them, since I think that could disrupt the flow a bit as you're decompiling.

This makes sense. I would expect naming them would be optional.

I wonder if perhaps this feature should be separate from the commenting feature? This features only requires a single UI icon per line (it's just quick on/off toggle). But with comments, you might want to eventually support multiple comment threads for the same line. In which case, it might be better for these to be decoupled.

The reason I started thinking about comments was that this would share a similar purpose.

We could implement this fairly easily as a small 'toggle your icon for target asm line' feature and later, potentially, develop it into comments. But it need not be comments initially

@ethteck
Copy link
Member

ethteck commented Dec 11, 2022

I'm pretty sure abaresk is mostly concerned with mid-decomping bookmarks, rather than a long-standing bookmark you'll refer back to much later (given my convo with them and the inital description). either way, can't we save the bookmarks with local browser storage?

@bates64
Copy link
Member

bates64 commented Dec 11, 2022

either way, can't we save the bookmarks with local browser storage?

This makes sense, we could do it this way for now to skip the backend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants