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

Fix 186 #187

Merged
merged 4 commits into from
Feb 14, 2013
Merged

Fix 186 #187

merged 4 commits into from
Feb 14, 2013

Conversation

bootstraponline
Copy link
Member

Fixed version of #186.

$ git remote add upstream git@github.com:appium/appium.git
$ git fetch upstream
$ git checkout -b fix
$ git cherry-pick cfbded8
$ open -a Xcode app/ios.js 
$ git add .
$ git commit
$ git cherry-pick 689ae95
$ git cherry-pick c5652f5
$ git cherry-pick e6a9c50
$ git rebase -i HEAD~2

pick 5fa17ab Add support for setValueImmediate
f 54144df Fix semicolon

Gaige B Paulsen and others added 3 commits February 14, 2013 10:41
This new mobile: command restores the ability to manually set the value
of all objects, including UITextField using the direct setValue() JS in
UIAutomation.

By default, POST /value will generate keypress events in a UITextView,
as is in line with the webdriver specification.  With this additon, you
can now use the 'old' appium method of setting directly with setValue
by using:
   mobile:setvalue  [{ element: elementID, value: value }]
@jlipps
Copy link
Member

jlipps commented Feb 14, 2013

@gaige I love this! Thanks for doing the research on the socket issue and the mobile: setValue looks great too. And thanks for the git help @bootstraponline. @gaige, can you do a quick code cleanup for me so I can merge:

  • spaces surrounding "=" and other operators e.g. in assignments
  • no spaces before first parameter of a function call

(or maybe it's more straightforward for @bootstraponline to do this since it's his PR--if he doesn't have time soon, I'll merge and we can do style fixes later).

@bootstraponline
Copy link
Member Author

@jlipps Would you mind creating a style guide page on the wiki? I'm happy to use whatever style. It'd be helpful if the style was documented.

@bootstraponline
Copy link
Member Author

I think I fixed everything. Please leave inline comments for whatever lines I missed.

if (self.currentSocket) {
self.debug("Socket closed forcibly due to exit");
self.currentSocket.end();
// self.currentSocket=null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be uncommented or removed?

Copy link
Member

Choose a reason for hiding this comment

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

@gaige, any thoughts about that comment? also, indentation seems weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, was offline at a meeting. The commented line should be removed.

@jlipps
Copy link
Member

jlipps commented Feb 14, 2013

@bootstraponline
Copy link
Member Author

style guide added

Thanks.

exports.setValueImmediate = function(req, res) {
var element = req.body.element
, value = req.body.value;
if (checkMissingParams(res, {element:element,value:value})) {
Copy link
Member

Choose a reason for hiding this comment

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

space after comma

@jlipps
Copy link
Member

jlipps commented Feb 14, 2013

line-commented on specific issues remaining

@bootstraponline
Copy link
Member Author

I think !== should be added to the style guide. I fixed the remaining issues other than the commented self.currentSocket=null;

if (self.currentSocket) {
self.debug("Socket closed forcibly due to exit");
self.currentSocket.end();
// self.currentSocket=null;
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like the old discussion went away. @gaige Is this commented out for a reason?

Copy link
Member

Choose a reason for hiding this comment

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

i'm fine leaving it commented for now, let's just fix the style and make the comment at the same level as the code, so:

self.currentSocket.end()
// self.currentSocket = null;

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@jlipps
Copy link
Member

jlipps commented Feb 14, 2013

I think !== should be added to the style guide.

added, thanks.

@bootstraponline
Copy link
Member Author

I think it's ready to merge.

self.debug("Socket closed forcibly due to exit");
self.currentSocket.end();
// self.currentSocket = null; // TODO: Why is this commented?
self.currentSocket.destroy(); // close this
Copy link
Member

Choose a reason for hiding this comment

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

these two lines look like they need to be indented 4 spaces

@jlipps
Copy link
Member

jlipps commented Feb 14, 2013

added one more note

@bootstraponline
Copy link
Member Author

I miss gofmt. I fixed the latest issue.

sebv pushed a commit to sebv/appium that referenced this pull request May 11, 2014
dpgraham pushed a commit to dpgraham/appium that referenced this pull request Oct 1, 2018
dpgraham pushed a commit to dpgraham/appium that referenced this pull request Oct 1, 2018
* Make the build more stable

* Adjust language handling

* Adjust language handling
dpgraham pushed a commit to dpgraham/appium that referenced this pull request Nov 7, 2018
dpgraham pushed a commit to dpgraham/appium that referenced this pull request Nov 7, 2018
* Make the build more stable

* Adjust language handling

* Adjust language handling
boneskull pushed a commit to boneskull/appium that referenced this pull request May 17, 2021
* Add urls caching logic

* Tune date verification

* Fix dictionary access

* tune try/catch scope

* simplify the condition
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

4 participants