-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add getDefaultProfile and setDefaultProfile methods to API #1858
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
base: master
Are you sure you want to change the base?
Conversation
|
Works for me, but of course I won't take anything for granted. If you're able to take a look that would be great. |
|
|
||
| } | ||
|
|
||
| private APIVpnProfile newAPIVpnProfile(VpnProfile vp) { |
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.
Should be named rather something like APIVPNProfileFromVPNProfile or even better have a static method in APIVpnProfile like constructFromVPNProfile
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.
The static method would be ideal except then there would have to be a dependency in APIVPNProfile on the implementation class VPNProfile, though. So I renamed to apiVPNProfileFromVPNProfile, in order to have the method name start with a lower case letter. (Google style guide would have us write use "Vpn" and "vpn" everywhere but we're not following that in any case). Not attached to the name.
I did notice that the two copies of APIVpnProfile.java were out of sync so I updated those. Notably, the copyright notices were different.
|
|
||
| @Override | ||
| public APIVpnProfile getDefaultProfile() throws RemoteException { | ||
| ProfileManager pm = ProfileManager.getInstance(getBaseContext()); |
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.
Missing permission check.
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.
Done
| } | ||
| break; | ||
| case ".api.SetDefaultVPN": | ||
| String defaultVpnName = intent.getStringExtra(EXTRA_NAME); |
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.
This case is quite long, better to put into into its own method.
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.
Moved the bodies of the two long cases into new methods.
| * This file is used for implementing the external API and this file like the AIDL and is exempted | ||
| * from the GPLv2. | ||
| * | ||
| */ |
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.
@schwabe Note that I updated the copyright notice in remoteExample/.../APIVpnProfile.java to match main/.../APIVpnProfile.java.
|
Gentle nudge. I have a simple VPN scheduling app that depends on this PR here: https://github.com/righteffort/ics-openvpn-scheduler |
|
Yeah. I would like to have the comments merged into the original commits instead of having an extra "PR comments" commit. Can you do that? |
Also: * add corresponding setDefaultVPN intent * add sample usage to remoteExample * bring remoteExample/.../IOpenVPNAPIService.aidl into sync with main/... * make APIVpnProfile.java in remoteExample/ and main/ identical.
Done. I just went ahead and squashed all the commits with details in the commit message -- I didn't see much value in keeping them separated. |
getDefaultProfileandsetDefaultProfileAPI methods and intents.IOpenVPNAPIService.aidland ofAPIVpnProfile.javainto sync.Fixes #1857.