Open tokou opened 6 years ago
Hey @navstyachka could you PTAL?)
Hi @tokou! Thank you for the contribution, but as soon as this is a massive change, I will need more time to review that.
I just noticed that it does not work when opening the index file directly in the browser. It works when you serve it from a local web server though. I will look into it.
I made an update to get the logs from the DOM tree instead of reading the logcat file. It works without a web server now.
The build seems to have failed for reasons unrelated to the code. Any way to retrigger it ?
@tokou I've triggered it
Hi @navstyachka Thanks for your detailed review. As you have guessed I am not really a react dev but an Android dev that wanted to contribute. I will take the time to make corrections based on your review. I understand that it is not possible to merge the PR even after the corrections. As I said in previous comments, I tried other approaches but they all resulted in performance problems when logs get large. I would be interested to explore other approaches if you have any suggestions.
There are many ways to approach this. I decided against adding to the
window.test
object as logcat output can be very large.I kept the static html generated by the Jar. I just added an id
static_logs
in order to remove it once we load the logs in js.I made a
LogContainer
component that shows the filters, the search bar and the logs once they are loaded from the file (it replaces the static logs).I extracted the
SearchBar
component and made it generic and used byLogFilter
andSuiteFilter
I tested this with a 10k lines logcat output file. Loading is seamless thanks to the html. Search is kinda laggy but acceptable. Maybe there's a React trick out there to make it faster.