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

'const' fix and support for specifying retina devices for subliminal-test #43

Closed
wants to merge 9 commits into from

Conversation

nzhuk
Copy link
Contributor

@nzhuk nzhuk commented Jul 5, 2013

No description provided.

Nikita Zhuk added 2 commits July 5, 2013 13:44
…IM_DEVICE will include single quotes if retina device is specified ( e.g. 'iPhone (Retina 4-inch)' )
…expands to const char *

- This fixes compiler warnings: "Sending 'const char [61]' to parameter of type 'char *' discards qualifiers"
@ghost ghost assigned wearhere Jul 5, 2013
@@ -335,7 +335,7 @@ fi
# http://openradar.appspot.com/13607967
TARGETED_DEVICE_FAMILY_ARG=""
if [[ -n "$SIM_DEVICE" ]]; then
case $SIM_DEVICE in
case ${SIM_DEVICE//"'"/} in
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that a multi-word SIM_DEVICE value must be single-quoted for it to ultimately be accepted (by defaults write, in the set_simulator_device script)--but it looks like defaults write actually requires both double- and single-quotes (like `"'iPhone (Retina 4-inch)'"), which is a little weird; I feel like the expectation is that you can usually get away with simply using one pair of quotes.

So, I feel like a better fix might be to have set_simulator_device single-quote the argument to defaults write:

defaults write com.apple.iphonesimulator SimulateDevice "'$DEVICE'"

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that could be a cleaner solution. The only thing I'm a bit worried about is that with the proposed solution set_simulator_device will produce different results than what you get when you change device type from iOS Simulator app's Device menu.

When you select "iPad" from the iOS Simulator -> Device menu, SimulateDevice value in com.apple.iphonesimulator prefs plist is set to iPad. If you run

defaults write com.apple.iphonesimulator SimulateDevice "'iPad'"

then SimulateDevice will be set to 'iPad'

However, running:

defaults write com.apple.iphonesimulator SimulateDevice "'iPad (Retina)'"

strips single quotes and sets SimulateDevice to iPad (Retina)

That said, iOS Simulator seems to handle quoted SimulateDevice values just fine, so this discrepancy doesn't seem to cause actual problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah if it can handle single-quoted arguments fine then I think that's the way to go, rather than putting the burden on the user to remember when to use single-quotes. Please fixup that change into the first commit and force push to update this request, then.

@wearhere
Copy link
Contributor

wearhere commented Jul 5, 2013

Question for you on the first commit, @nzhuk. Second commit looks good. Btw I don't think that this request broke the build, it looks like Travis just hiccuped.

Nikita Zhuk added 4 commits July 13, 2013 16:53
…efaults' command. Device model names with spaces ( e.g. 'iPhone (Retina 4-inch)' ) require single-quoting.
…IM_DEVICE will include single quotes if retina device is specified ( e.g. 'iPhone (Retina 4-inch)' )
…expands to const char *

- This fixes compiler warnings: "Sending 'const char [61]' to parameter of type 'char *' discards qualifiers"
@nzhuk
Copy link
Contributor Author

nzhuk commented Jul 13, 2013

Ok, I'm a hg guy so I probably screwed up this git rebase/fixup thing in my fork, but the end result of all those commits seems to be what is needed.

@wearhere
Copy link
Contributor

Thanks for updating the request, @nzhuk. I'm a bit puzzled by the sequence of the commits though: it looks like you have two changes (adding single quotes to the device model name, and adding the const qualifier) made multiple times? I'd think you'd only have two commits when you finished, one for each of those changes. Those look to be (roughly) 2130b10, and 6ba67b0. 2130b10 contains the code that undoes e03508d--you can get rid of that by squashing 2130b10 into e03508d, while keeping 2130b10's commit message.

Nikita Zhuk added 3 commits July 16, 2013 13:41
…expands to const char *

- This fixes compiler warnings: "Sending 'const char [61]' to parameter of type 'char *' discards qualifiers"
…efaults' command. Device model names with spaces ( e.g. 'iPhone (Retina 4-inch)' ) require single-quoting.
@nzhuk
Copy link
Contributor Author

nzhuk commented Jul 16, 2013

Ok, I've created a new pull request with just those two commits ( #45 ), please ignore this one.

@nzhuk nzhuk closed this Jul 16, 2013
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.

2 participants