-
Notifications
You must be signed in to change notification settings - Fork 41
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
API for managing SpringBoard alerts manually #611
API for managing SpringBoard alerts manually #611
Conversation
lib/run_loop/device_agent/client.rb
Outdated
end | ||
|
||
# @!visibility private | ||
def dismiss_springboard_alerts_automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a long history with ruby idioms, but with most languages in this case I'd expect an API to accept a parameter instead of have the parameter implicitly dictated by the method name:
dismiss_springboard_alerts_automatically(true);
dismissSpringboardAlertsAutomatically(true);
[self dismissSpringboardAlertsAutomatically:YES];
If you wanted to force more semantic clarity on the caller, maybe something like
set_springboard_alert_dismissal(SpringboardDismissal::MANUAL)
In my mind, if I were programming and I realized I wanted to change the state of springboard automatic dismissal, I'd assume I'm going to be _set_ting a state and thus would instinctively look for a method whose name was similar to set_<something>
or otherwise more clearly implied a change of state.
If I'm not mistaken, ruby semantics would suggest at the very least that the method names, since they are changing state/have side effects, be dismiss_springboard_alerts_automatically!
and dismiss_springboard_alerts_manually!
, no?
A more clear approach in my opinion might be
enable_automatic_springboard_alert_dismissal!
disable_automatic_springboard_alert_dismissal!
Also, I think these two methods could be refactored - seems they only have a single variable differentiating their implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_
You are right, this is better - particularly at this API level.
!
Also correct.
When I implement the public API (in Calabash iOS), I want to use:
dismiss_springboard_alerts_automatically!
dismiss_springboard_alerts_manually!
for clarity - there is absolutely no ambiguity.
On the other hand, maybe it would be best to use the set_
idiom for discoverability.
I am trying to avoid using the word dismissal
in the API and in the documentation because I have a feeling (based on Gitter chats) that it confuses non-native English speakers.
Thanks.
0aba3bc
to
a988ccf
Compare
Route no longer returns a result key. Instead it returns a hash representation of the showing SpringBoard alert or an empty hash if no alert is showing
a988ccf
to
95b305f
Compare
jmoody Thanks always. Thanks for the latest korean alert patch. def dismiss_springboard_alert (button_title) Should DeviceAgent be modified? I know that only client.rb has been updated. Thanks for the quick reply. |
You will have to wait for run-loop 2.4.0 which needs to wait for Xcode 8.3. |
Motivation
Completes:
Progress on:
API
Test
Tested against changes in DeviceAgent: