Skip to content

Improve screen recording flow #303

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Jan 17, 2018
Merged

Conversation

mykola-mokhnach
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach commented Jan 11, 2018

I've changed the logic here, so now it's much simpler to get the result of the screen record. there is also a possibility to upload the file to a remote location using ftp/http protocols and protection from OOM issues.

Related base driver PR: appium/appium-base-driver#168

I'll also take care about finishing the java client implementation (the PR is there, but it has been waiting for almost a year).

@imurchie
Copy link
Contributor

I don't understand the need for this. The methods need to be added to the clients, to give access to this functionality, rather than clutter up the mobile: space with duplicate functionality.

@mykola-mokhnach
Copy link
Contributor Author

mykola-mokhnach commented Jan 11, 2018

The methods need to be added to the clients

Where exactly they should be added @imurchie ?

@jlipps
Copy link
Member

jlipps commented Jan 11, 2018

I think what @imurchie is saying is that /start_recording_screen and /stop_recording_screen already exist, so we don't need to make mobile: versions of them. instead, we need to teach the appium clients (java, python, etc...) how to make calls to the existing endpoints.

@mykola-mokhnach
Copy link
Contributor Author

@jlipps I checked the current API endpoint in base driver and I don't find them very flexible and cross-platform. For example, I'd like the stopRecording endpoint to return the taken video immediately. Also, some parameters, which are currently marked as required for start_recording are optional in reality.

Don't you mind if I change the endpoints in the base drivers to make them more flexible?

@mykola-mokhnach
Copy link
Contributor Author

Also, I could add such endpoints to XCTest driver, since iOS simulator also allow to record a video.

@mykola-mokhnach mykola-mokhnach changed the title Add mobile endpoints for screen recording Improve screen recording flow Jan 12, 2018
return `${bytes} B`;
}

async function uploadRecordedMedia (adb, pathOnDevice, remotePath = null, uploadOptions = {}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all these helper functions can be also reused in iOS. Perhaps, we could move them to some common lib. the only question is to what lib

Copy link
Contributor

Choose a reason for hiding this comment

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

The only place would be appium-support.

@mayuayla
Copy link

Thank you so much for this code update. Happy that there are amazing dedicated people improving Appium. Looking forward to using this with the Python client.
-Mayu

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants