This repository has been archived by the owner. It is now read-only.

IWindow protocol #78

Merged
merged 6 commits into from Oct 2, 2012

Conversation

2 participants
@smidas
Contributor

smidas commented Sep 28, 2012

Adds functionality to programmatically manage the size and relative position of the browser window. The protocol is declared and implemented in the clj-webdriver.window namespace, in which contains the following functions:

(position [driver] "Returns map of X Y coordinates ex. {:x 1 :y 3} relative to the upper left corner of screen.")
(reposition [driver point-map] "Excepts map of X Y coordinates ex. {:x 1 :y 3} repositioning current window relative to screen. Returns driver.")
(size [driver] "Get size of current window. Returns a map of width and height ex. {:width 480 :height 800}")
(resize [driver dim-map] "Resize the driver window with a map of width and height ex. {:width 480 :height 800}. Returns driver.")
(maximize [driver] "Maximizes the current window to fit screen if it is not already maximized. Returns driver.")

Example usage:

(:use clj-webdriver.taxi)
(:require [clj-webdriver.window :as win])

(def driver (new-driver {:browser :firefox}))

(win/resize driver {:width 480 :height 800})
(win/position driver) => {:x 34 :y 56}
(win/size driver) => {:width 480 :height 800}

clj-webdriver.test.window namespace contains passing tests. I didn't add that namespace to the clj-webdriver.test.runner or to the bash script as I didn't know which or if both will be used moving forward.

@semperos

This comment has been minimized.

Show comment Hide comment
@semperos

semperos Sep 28, 2012

Owner

@smidas Thank you for your continued contributions. I'll take a look at this and merge.

As for running the tests, I'm not sure which way I'm going to go either :)

Owner

semperos commented Sep 28, 2012

@smidas Thank you for your continued contributions. I'll take a look at this and merge.

As for running the tests, I'm not sure which way I'm going to go either :)

@smidas

This comment has been minimized.

Show comment Hide comment
@smidas

smidas Sep 28, 2012

Contributor

Not a problem man. Glad to contribute to such a great project.

Oh, I added a new namespace because I was not sure if you wanted direct contributions into the clj-webdriver.core* namespaces or not.

Contributor

smidas commented Sep 28, 2012

Not a problem man. Glad to contribute to such a great project.

Oh, I added a new namespace because I was not sure if you wanted direct contributions into the clj-webdriver.core* namespaces or not.

@semperos

This comment has been minimized.

Show comment Hide comment
@semperos

semperos Oct 1, 2012

Owner

I've tried writing a response to this pull request a couple of times now, because it raises a lot of larger questions about window handling throughout the API (there's already some there). But since my response was turning into paragraph upon paragraph, I'll just ask you to make a couple of small edits and then I'll merge and make changes afterward:

  • Please make the reposition!, resize!, and maximize! functions return the driver being acted on, instead of nil. This doesn't mirror the Java API, but I like to replace it's copious void returns with something useful, and in this case returning the driver opens the door to better composability.
  • Please rename the functions in the IWindow protocol that have ! at the end not to include the !. I like to limit that to functions that have mutable-state semantics like swap! or reset!, that act on "legitimate" data structures (e.g., clj-webdriver.taxi/set-driver!). Since there's so much mutability throughout the API related to working with good ol' Java objects, I keep !'s to a minimum.
  • The test suite passes for Firefox, but fails for Chrome in the maximize test. I think maximizing is going to be a troublesome thing to test in a cross-browser, cross-platform fashion. I'd be satisfied if you just tested that maximizing made either the width or height greater, and leave it at that.
  • In the test suite, please rename the firefox-driver var to just driver.

Thanks again for contributing. After these edits, I'll accept your pull request. Afterward, I'll be merging your code with the clj-webdriver.window-handle namespace and making windows more of a first-class citizen in the API, like driver and element instances are, so a second thanks for making me realize window handling needed more attention. The functions in your IWindow protocol only work on the currently-active window, which is the expected behavior, but there needs to be more of a relationship between functions that are already in the API that deal with switching/choosing windows and manipulating them directly as you do here.

Owner

semperos commented Oct 1, 2012

I've tried writing a response to this pull request a couple of times now, because it raises a lot of larger questions about window handling throughout the API (there's already some there). But since my response was turning into paragraph upon paragraph, I'll just ask you to make a couple of small edits and then I'll merge and make changes afterward:

  • Please make the reposition!, resize!, and maximize! functions return the driver being acted on, instead of nil. This doesn't mirror the Java API, but I like to replace it's copious void returns with something useful, and in this case returning the driver opens the door to better composability.
  • Please rename the functions in the IWindow protocol that have ! at the end not to include the !. I like to limit that to functions that have mutable-state semantics like swap! or reset!, that act on "legitimate" data structures (e.g., clj-webdriver.taxi/set-driver!). Since there's so much mutability throughout the API related to working with good ol' Java objects, I keep !'s to a minimum.
  • The test suite passes for Firefox, but fails for Chrome in the maximize test. I think maximizing is going to be a troublesome thing to test in a cross-browser, cross-platform fashion. I'd be satisfied if you just tested that maximizing made either the width or height greater, and leave it at that.
  • In the test suite, please rename the firefox-driver var to just driver.

Thanks again for contributing. After these edits, I'll accept your pull request. Afterward, I'll be merging your code with the clj-webdriver.window-handle namespace and making windows more of a first-class citizen in the API, like driver and element instances are, so a second thanks for making me realize window handling needed more attention. The functions in your IWindow protocol only work on the currently-active window, which is the expected behavior, but there needs to be more of a relationship between functions that are already in the API that deal with switching/choosing windows and manipulating them directly as you do here.

smidas added some commits Oct 1, 2012

smidas
Changed @firefox-driver var to @driver, removed ! to match api, and m…
…aximize test now looks to see if width and height are greater.
@smidas

This comment has been minimized.

Show comment Hide comment
@smidas

smidas Oct 1, 2012

Contributor

The requested changes have been made and pushed to the window-protocol branch.

I want to extend my gratitude for your willingness to take the time to critique my pull request and explain your reasoning. This only stands to make me more conscious of the thought process that goes into a well planned API.

Contributor

smidas commented Oct 1, 2012

The requested changes have been made and pushed to the window-protocol branch.

I want to extend my gratitude for your willingness to take the time to critique my pull request and explain your reasoning. This only stands to make me more conscious of the thought process that goes into a well planned API.

semperos added a commit that referenced this pull request Oct 2, 2012

Merge pull request #78 from smidas/window-protocol
IWindow protocol, functions to manipulate system-level browser window

@semperos semperos merged commit 65d4a20 into semperos:master Oct 2, 2012

@semperos

This comment has been minimized.

Show comment Hide comment
@semperos

semperos Oct 2, 2012

Owner

Happy to share both ways, it's definitely a community effort. I'm accepting this, and I'll keep you and others posted on the Google Group as to the rest of the window-related changes I'll be making.

Owner

semperos commented Oct 2, 2012

Happy to share both ways, it's definitely a community effort. I'm accepting this, and I'll keep you and others posted on the Google Group as to the rest of the window-related changes I'll be making.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.