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

add apis for recording the screen #606

Closed
wants to merge 19 commits into from

Conversation

heeseon
Copy link
Contributor

@heeseon heeseon commented Mar 20, 2017

Change list

add apis for recording the screen

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

I added three apis for recording the screen

@TikhomirovSergey
Copy link
Contributor

@SrinivasanTarget Why is this on hold? Is there lack of functuinality on the server side?

@heeseon could you provide some tests?

@saikrishna321
Copy link
Member

@TikhomirovSergey

Is there lack of functuinality on the server side?

Yes

@TikhomirovSergey
Copy link
Contributor

TikhomirovSergey commented Jun 6, 2017

@saikrishna321 Hi. Is there any update on the server side to try to test/merge this PR? It seems related issues were closed.

@SrinivasanTarget
Copy link
Member

@TikhomirovSergey Its here now appium/appium-android-driver#218. Not yet complete at server end.

@heeseon
Copy link
Contributor Author

heeseon commented Jun 24, 2017

Its here now appium/appium-android-driver#218 completed. please review this PR.

@SrinivasanTarget
Copy link
Member

@heeseon Will review once a driver is released .

@SrinivasanTarget
Copy link
Member

Meanwhile can you fix the merge conflicts @heeseon

@heeseon
Copy link
Contributor Author

heeseon commented Jul 6, 2017

I fixed the merge conflicts.

Copy link
Contributor

@TikhomirovSergey TikhomirovSergey left a comment

Choose a reason for hiding this comment

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

@heeseon Hi
Sorry for the late response. Could you provide some test to be sure that everything is ok)

@@ -151,7 +156,10 @@
postC("/session/:sessionId/appium/performanceData/types"));
commandRepository.put(GET_PERFORMANCE_DATA,
postC("/session/:sessionId/appium/getPerformanceData"));

commandRepository.put(START_RECORDING_SCREEN,
postC("/session/:sessionId/appium/startRecordingScreen"));
Copy link
Member

Choose a reason for hiding this comment

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

There is a change in API here, so update here accordingly as start_recording_screen or else it will throw unsupported command exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I request to change from start_recording_screen to startRecordingScreen

Copy link
Member

Choose a reason for hiding this comment

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

@heeseon Didn't get you? It was changed in server side as per standards or convention appium follows.

commandRepository.put(START_RECORDING_SCREEN,
postC("/session/:sessionId/appium/startRecordingScreen"));
commandRepository.put(STOP_RECORDING_SCREEN,
postC("/session/:sessionId/appium/stopRecordingScreen"));
Copy link
Member

Choose a reason for hiding this comment

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

Same here stop_recording_screen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I request to change from stop_recording_screen to stopRecordingScreen

@heeseon
Copy link
Contributor Author

heeseon commented Aug 15, 2017

@TikhomirovSergey I added test case ^^

@TikhomirovSergey TikhomirovSergey added this to the 5.1.0 milestone Aug 28, 2017
@heeseon
Copy link
Contributor Author

heeseon commented Sep 7, 2017

for the codacy-bot comment 22 days ago
Codacy Issue found: Do not hardcode /sdcard.

how I can resolve this?

@mykola-mokhnach
Copy link
Contributor

@heeseon Simply move the string to a constant. Also, there are many conflicts there. Please rebase your branch with recent master.

@appium appium deleted a comment Sep 7, 2017
@appium appium deleted a comment Sep 7, 2017
@appium appium deleted a comment Sep 7, 2017
@appium appium deleted a comment Sep 7, 2017
@appium appium deleted a comment Sep 7, 2017
@appium appium deleted a comment Oct 3, 2017
…-client into readperformancedata

* 'readperformancedata' of https://github.com/heeseon/java-client: (156 commits)
  build error
  Do not hardcode
  Do not hardcode
  Update README.md
  Update README.md
  Code style issues which were found by reviewer were fixed
  Code style issues which were found by reviewer were fixed
  The addition to appium#738 - following dependencies were updated:   `org.seleniumhq.selenium:selenium-java` to 3.6.0   `com.google.code.gson:gson` to 2.8.2   `org.springframework:spring-context` to 5.0.0.RELEASE   `org.aspectj:aspectjweaver` to 1.8.11
  do not zero out implicit wait during location call
  rename DEFAULT_IMPLICITLY_WAIT_TIMEOUT to DEFAULT_TIMEOUT
  Update README.md
  some minor things that were found by reviewers were improved
  code style issues were got fixed
  appium#732 FIX
  ServerBuilderTest: ip calculation was improved
  ServerBuilderTest: the path resolving
  ServerBuilderTest: magic strings were turned into final values
  ServerBuilderTest: magic strings were turned into final values
  ServerBuilderTest: code improvement.
  Tests of local appium DriverService were re-designed.
  ...
@appium appium deleted a comment Oct 15, 2017
@mykola-mokhnach
Copy link
Contributor

Replaced by #814

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.

None yet

5 participants