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

Code comments #1127

Merged
merged 8 commits into from
Mar 4, 2021
Merged

Code comments #1127

merged 8 commits into from
Mar 4, 2021

Conversation

skylot
Copy link
Owner

@skylot skylot commented Mar 2, 2021

Add comments feature to jadx-gui (fixes #359)

  • All comments stored in project file. Make sure you save it before exit (or enable autosave in preferences).
  • Multi-line comments supported
  • Comments attached by raw instructions offset, so it is possible to see them in fallback mode.
  • Search for comments also supported (global or in current file)

@Surendrajat
Copy link
Contributor

Surendrajat commented Mar 2, 2021

@skylot somehow comments search view is always empty for me. Am I doing something weird? :)

@skylot
Copy link
Owner Author

skylot commented Mar 2, 2021

@Surendrajat thanks! Looks like issue reproduces for inner classes. With warning in log:

WARN : Failed to get ref node for comment: JadxCodeComment{...

@Surendrajat
Copy link
Contributor

Surendrajat commented Mar 2, 2021

@skylot seems like I did not realize I was only testing inner-classes ;P
Works fine in other places.

Copy link
Contributor

@Surendrajat Surendrajat left a comment

Choose a reason for hiding this comment

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

Last commit fixed the remaining issue, so I'm happy with this feature. Thanks again!

@busmaker
Copy link
Contributor

busmaker commented Mar 2, 2021

Get rid of the "offset" field in the CodePosition is so exciting, it makes rename without reloading classes and update index much more easier.

I think it's better not use CodePosition as a map key, so we can just update its relevant position after rename no need to reindex anymore.

Since CodePosition is absolutely sorted, I think use a list and binary search is enough for efficiency.

@skylot
Copy link
Owner Author

skylot commented Mar 2, 2021

@lbj-the-goat you are right, but only partially.
CodePosition now store duplicate information and can be replaced with "text offset from start" (soo many offset in the code 🤣)

But I don't think it is a good idea to use inplace renaming, because it is not general enough, and as a result we will need to implement 2 ways of applying rename or comments or any other code modifications.
Also, now text index holds prepared lines objects and just show them in search results without creating over an over again. Not sure how can affect removing of such cache.

@busmaker
Copy link
Contributor

busmaker commented Mar 2, 2021

@skylot resource search doesn't has a cache it's not slow. We only want a match in a line and it's in-memory, so I think a full document search shouldn't be a big problem.
We can try to make the text and usage search always refer to the code text and positions of ICodeInfo and CodePosition, by doing so we can just take care this two classes and forget about the caches and indexes 🤗🤗🤗 Isn't it greate?

@skylot
Copy link
Owner Author

skylot commented Mar 3, 2021

@lbj-the-goat sure, code index not really needed, but it slightly speeds up search results generation by caching them. So we need to make some PoC to check how such changes will affect performance and memory usage.
Anyway, I plan to refactor search to lazy pageable model, because now, in most cases, more than a hundred results not really needed, but search spent time and memory to prepare them.

And for this PR: I fixed some small search issues, and not found any new one, so I think it is ready for merge.

@busmaker
Copy link
Contributor

busmaker commented Mar 4, 2021

@skylot Great plan, I once tried to change search featrue to multithreaded, but failed.
btw I am doing a smali debugger, by now it's almost 8k lines, would you accept a PR with such many lines but no test files for it? just for curiosity :)

@skylot
Copy link
Owner Author

skylot commented Mar 4, 2021

I am doing a smali debugger, by now it's almost 8k lines, would you accept a PR with such many lines but no test files for it?

Well, tests are a good thing, but now jadx-gui don't have them, so I think it is ok 🤣

@skylot skylot merged commit 4e5fac4 into master Mar 4, 2021
@skylot skylot deleted the code-comments branch March 4, 2021 14:45
@jpstotz
Copy link
Collaborator

jpstotz commented Mar 4, 2021

In a quick test of the new code comment feature I wasn't able to use the key combinations proposed by the context menu. May be because there is no ; key on a German keyboard layout (I have to press Shift + , to get a ;). I assume this will be also true for other languages and will raise issues.

@skylot
Copy link
Owner Author

skylot commented Mar 4, 2021

@jpstotz hm, I thought, these keystroke bindings are kinda language independent, and now I have no idea how to fix this. I will try to search for a solution anyway. And thanks for share this issue!

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.

Comments feature
4 participants