feat: Add locator types supported by flutter integration driver by sudharsan-selvaraj · Pull Request #2201

feat: Add locator types supported by flutter integration driver by sudharsan-selvaraj · Pull Request #2201 · appium/java-client

Conversation

Change list

Please provide briefly described change list which are you going to propose.

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)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

Please provide more details about changes if it is necessary. If there are new features you can provide code samples which show the way they
work and possible use cases. Also you can create gists with pasted java code samples or put them here using markdown.
About markdown please read Mastering markdown and Writing on GitHub

		return String.format("%s.%s: %s", AppiumBy.class.getSimpleName(), locatorName, remoteParameters.value());
		}

		public Map<String, Object> toJson() {

Choose a reason for hiding this comment

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

not sure this method is needed

Choose a reason for hiding this comment

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

Currently flutter driver supports custom commands like scrollTillVisble, waitForVisible, waitForVisible etc, which accepts raw locator info(By) as arguments. So users can directly pass the locator info by calling toJson() method instead of constructing the JsonMap manually.

Choose a reason for hiding this comment

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

I can observe that Remotable.Parameters class already contains toJson method. It's private though. Lets add it to the list in SeleniumHQ/selenium#13949 and leave a TODO there.

Also consider using Map.of() factory instead.

Choose a reason for hiding this comment

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

Could you also provide an example of any of the above calls where toJson method may be used?

Choose a reason for hiding this comment

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

One such example is using flutter: scrollTillVisible method.

Without toJson()

driver.executeScript("flutter: scrollTillVisible", Map.of("finder", Map.of("using", "-flutter key", "value", "login_button")));

With toJson()

driver.executeScript("flutter: scrollTillVisible", Map.of("finder", AppiumBy.flutterKey("login_button").toJson());

Choose a reason for hiding this comment

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

It would probably make sense to create FlutterIosDriver and FlutterAndroidDriver inherited from iOSDriver and AndroidDriver, so the above extensions may be implemented as first-class methods, e.g.

flutter. scrollTillVisible(AppiumBy.flutterKey("login_button"))

Choose a reason for hiding this comment

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

We initially had a similar idea. After reviewing the implementation, we found that the logic is consistent for both iOS and Android drivers, so we decided to maintain this approach.

Choose a reason for hiding this comment

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

@sudharsan-selvaraj Agree we wanted to make it consistent but nothing is stopping us implementing the initial approach if everyone is fine. It gets easier to implement the tests.
cc: @mykola-mokhnach

Choose a reason for hiding this comment

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

@saikrishna321 @mykola-mokhnach That sounds good! Let's proceed by creating separate Flutter-specific driver classes to abstract the underlying commands. I'll prepare a patch with the changes and tag for the review.

@mykola-mokhnach @SrinivasanTarget I have updated the PR to just include the changes related to locators. I'll create separate PR for driver implementation along with the e2e tests.

Hi @sudharsan-selvaraj, congrats, the Appium project wants to compensate you for this (and perhaps other) contribution(s) this month! Please reply to this comment mentioning me and sharing your OpenCollective account name, so that we can initiate payment! Or let me know if you decline to receive compensation via OpenCollective. Thank you!

Hey @jlipps , I would like to express my gratitude to the Appium team for the recognition and I'm pleased to be a part of the project. For the fund transfers, please use the OpenCollective account associated with https://opencollective.com/appium-device-farm. If you need any additional information, please let me know.

Does this support annotation (eg @AndroidFindBy & @iOSXCUITFindBy) to create common element ?

Conversation

Change list

Types of changes

Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels