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

Change SearchContext so that findElements can return a list that sub-classes WebElement #604

Closed
wants to merge 10 commits into from

Conversation

alexec
Copy link

@alexec alexec commented Jun 2, 2015

Firstly, I apologize for the commit comment. I only just read the "guidelines for contributing", and promptly failed to be able to squash the commits successfully.

This makes SearchContext generic on <E extends WebElement>. To prevent "yellow" markers in IntelliJ due to "rawtypes", I updated subclasses as well. This is API compatible for all subclasses, but allows for more flexible for implementations of WebElement and WebDriver when we might want to return decorated versions of a WebElement.

This is related to:

…turn a list of any class to sub-classes WebElement (e.g. MobileElement).
…turn a list of any class to sub-classes WebElement (e.g. MobileElement).
…turn a list of any class to sub-classes WebElement (e.g. MobileElement).

This make SearchContext generic on <E extends WebElement>. To prevent "yellow" markers in IntelliJ, subclasses needed to be updated as well. This is API compatible for all subclasses, but more flexible for implementations of WebElement and WebDriver when we might want to return decorated versions of a WebElement.
@TikhomirovSergey
Copy link
Contributor

Hi @alexec
The change that you require is already implemented and it is still waiting 🕐 for the merging or rejection by Appium team. Please look at this: appium/java-client#182

I think that your change is not sufficient because there are interfaces like FindsById, FindsByClassName and so on. I think they should be generic too...

@lqc
Copy link

lqc commented Jun 3, 2015

First of all, there is the WebDriver interface which extends SearchContext and I don't see your commit make it generic, so it uses a rawtype, which is bad.

That said, please don't make WebDriver generic as it will only cause pain for everyone. AFAIK, isn't it enough to change the signature to List<? extends WebElement> ? Then AppiumDriver can overide the signature to List<? extends MobileElement> (e.g. http://ideone.com/kuVIED).

@alexec
Copy link
Author

alexec commented Jun 3, 2015

Back to the drawing board?

I tried changing to List<? extends WebElement>, but this cause compilation issues:
screen shot 2015-06-03 at 12 48 29

Verses this one (where you'd have to update code to capture:

screen shot 2015-06-03 at 12 48 53

@TikhomirovSergey
Copy link
Contributor

@lqc I agree.
@lqc @alexec ...and the same is already implemented and proposed via the mentioned appium/java-client#182. It is much better to review it.

@alexec
Copy link
Author

alexec commented Jun 3, 2015

@TikhomirovSergey @lqc thank you for taking a look at this. I'm not actually using Appium for the work I'm doing, it's for another project that wraps WebElement in another type (sub-class). The problem is similar, we can have MyElement findElement(By by) just fine, but this is not consistent with List<WebElement> findElements(By by). This losses a lot of the benefits of being able to decorate WebDriver and WebElement, and find myself casting in various places

Naturally, I want to make the minimally invasive changes to such a key interface, and to maintain complete backwards compatibility with existing code.

This may very well not be possible!

@lqc
Copy link

lqc commented Jun 3, 2015

@alexec Can't check right now, so the compiler in my head can be mistaken, but if you make WebDriver generic then my code suddenly gets +thousands of "rawtypes" errors which I will need to fix. I can do it 2 ways: write WebDriver<WebElement> everywhere which is a PITA, or write WebDriver<?> which will yield the same compilation errors.

And of course WebElement is also a SearchContext, so it's actually WebDriver<WebElement<WebElement>> vs WebDriver<?> + compilation errors.

Out of 3 options, only changing List<WebElement> to List<? extends WebElement> seems like least work.

PS. I feel your pain - we also tried a solution which decorated some of the Selenium APIs and extended WebElement, but I was just unmanageable. Now we use composition instead of inheritance for this and it works a lot better.

@TikhomirovSergey I'm not a Appium user, so I don't care much if you add tons of generics and @SuppressWarnings({"rawtypes"}. I just don't want Selenium to force me to do so in my project ;)

@alexec
Copy link
Author

alexec commented Jun 11, 2015

@lqc. Sorry about the delay in getting back to you. I'd experimented around a bit, and in the last commit I've found a solution that seems to avoid raw types, and avoid have to change code to due to the signature change. However, it put "yellow warnings" in various places.

<W extends WebElement> List<W> findElements(By by);

It like the lesser of two evils.

@@ -19,7 +19,7 @@

import java.util.List;

public interface SearchContext {
public interface SearchContext{
Copy link
Member

Choose a reason for hiding this comment

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

?

@alexec
Copy link
Author

alexec commented Oct 3, 2015

This PR duplicates #863. I'll close.

@alexec alexec closed this Oct 3, 2015
yiming-tang-cs pushed a commit to ponder-lab/selenium that referenced this pull request Jan 2, 2020
yiming-tang-cs pushed a commit to ponder-lab/selenium that referenced this pull request Jan 2, 2020
yiming-tang-cs pushed a commit to ponder-lab/selenium that referenced this pull request Jan 2, 2020
yiming-tang-cs pushed a commit to ponder-lab/selenium that referenced this pull request Jan 2, 2020
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