logs page for application #339
Conversation
There was a problem hiding this comment.
Thanks for your contribution!
Overall looks good, however, as the package name suggests, it's just a demo. It's not meant to be a ready-to-use tool for anyone, so I don't want to add too many "high-level" features that are only some combination of the pre-existing "low-level" API calls. These features are better suited for your own folk, or the internal tool you developed completely from scratch.
A better way to contribute is adding the core logic to the adb package, then (optionally) adding the UI showing its usage in the demo.
For example, when I was working on the file manager and scrcpy demo, I found the rm -rf command is quite common, and its arguments require some quote escapement, so I added a shortcut to it in the adb package.
As the logcat is also a common command, you can add an Adb#logcat(options: LogcatOptions): LogcatStream method, where the options is an object corresponding to logcat's command line switches (It's fine to only have the appId you need now), and the return value may apply some transformations to the output to make it machine-parsable. So everyone can use logcat via API calls easier.
| @@ -54,6 +54,8 @@ function App(): JSX.Element | null { | |||
| }, []); | |||
|
|
|||
|
|
|||
| const appId = new URLSearchParams(window.location.search).get('appId') | |||
There was a problem hiding this comment.
Use the useLocation hook in Logcat.tsx to read querystring (the return value is almost same as window.location, but works with hash routers).
The final URL will look like /#/logcat?appId=com.google.android.youtube.
Benefits:
- Don't need to pass
appIddown to component here. - Won't interfere with other routes that need other queries.
| @@ -160,6 +170,7 @@ function App(): JSX.Element | null { | |||
| <CacheRoute | |||
| exact={route.exact} | |||
| path={route.path} | |||
| key={route.path} | |||
There was a problem hiding this comment.
IIRC something didn't work when I added the key, best to not change it. (or add an eslint-disable if you bother)
|
|
||
| try { | ||
| connectingRef.current = true; | ||
| const socket = await device.childProcess.shell(); |
There was a problem hiding this comment.
Does this still require an interactive shell? Can you childProcess.spawn a logcat process and "connect" it to xterm?
| const UpIconProps = { iconName: 'ChevronUp' }; | ||
| const DownIconProps = { iconName: 'ChevronDown' }; | ||
|
|
||
| class AdbTerminal extends AutoDisposable { |
There was a problem hiding this comment.
Maybe move the class to a separete file to reuse it. Or even extract the whole component so it can be used like <XtermTerminal socket={socket} />
| export const Logcat = withDisplayName('Logcat')(({ | ||
| visible, | ||
| device, | ||
| applicationId, |
There was a problem hiding this comment.
What will happen if appId is undefined? Will it output all logs from the device?
|
Note: Currently the CI is broken because the build script of |
Do you have an eta on a fix for the CI? The main benefit in this PR for me is the automatic deployment. |
That will bloat the api though? I would prefer to work with an Maybe this should be added to |
Fixed
Yes, but I don't think it currently has enough API to be bloated. 😭 One solution is to group all these binary wrappers in a separated namespace, for example
I can think of two patterns:
|
I explored this for a couple days and the above is blocking me. |
No, only one program can have access to a USB device at same time, see https://github.com/yume-chan/ya-webadb/blob/master/apps/book/src/basics/transportation.md#server |
|
I'm now working on a table-based logcat view. The remaining tasks are in #425. Thus closing this PR. |
User friendly logcat page.
For example, to share logs page for YouTube app: http://localhost:9000/?appId=com.google.android.youtube
Useful for QA teams.