[WIP] Add a way to select Android User to let Flipper working on devices with Work Profile #4606
Conversation
|
Hi @sbatezat! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
1 similar comment
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Hi there, |
| const activeDevice: BaseDevice = useSelector(getActiveDevice); | ||
| const [users, setUsers] = useState([] as {id: string; name: string}[]); | ||
|
|
||
| activeDevice |
There was a problem hiding this comment.
Best run shell commands in an effect, with the current setup the command is fired on every render
There was a problem hiding this comment.
As said on comments, I don't know React. Please feel free to edit that part, as I don't have knowledge about these "useEffect" things. Just a warn: when you say "the command is fired on every render", it may be fine. Best would be to run this command each time selected device change (and user is opening settings sheet) or you could have bad users ids shown.
| }); | ||
| setUsers(users || []); | ||
| }) | ||
| .catch((error) => console.error(error)); |
There was a problem hiding this comment.
Could you add some more details to the error message? Also let's start with .warn, as this is typically a local setup issue
There was a problem hiding this comment.
This command should not fail. If this command fails there is chances that every shell command will. If I have a look on Flipper source code where executeShell is called, errors are not even managed. For example, on plugins/public/cpi/index.tsx
const chipname = await executeShell('getprop ro.chipname');
There is no try/catch.
The only reason why I've got a catch it's because I don't know how to await in a React context so linters are compelling me to consume the promise in the good old way (.then/.catch) with a compelled catch block (as per linters rules).
With these informations in mind, what do you think about changes I should work on this? Just a warn instead of an error?
|
|
||
| const logTag = 'AndroidCertificateProvider'; | ||
|
|
||
| export default class AndroidCertificateProvider extends CertificateProvider { | ||
| name = 'AndroidCertificateProvider'; | ||
| medium = 'FS_ACCESS' as const; | ||
|
|
||
| constructor(private adb: Client) { | ||
| constructor(private flipperServer: FlipperServerImpl, private adb: Client) { |
There was a problem hiding this comment.
could we pass in less here than the entire flipperServer? For example just the settings object?
There was a problem hiding this comment.
I can do that yes 👍
What's your though about the more suitable thing to pass in?
You are suggesting the settings object, but the minimal one would be the userId. Indeed, the only reason why flipperServer is pass in is to read "this.flipperServer.config.settings.androidUserId"
Pull Request Test Coverage Report for Build 4477312510
💛 - Coveralls |
|
Hi @mweststrate , thanks for reviewing my PR 👍 I've answered with some questions, could you please double check them?
|
|
Hi @mweststrate, could you please have a look on my questions above? |
|
Hi @mweststrate, just a quick reminder on this topic :) |
|
Sorry, been travelling and still catching up, hopefully next week.
…On Thu, Apr 13, 2023 at 9:18 AM Sébastien Batézat ***@***.***> wrote:
Hi @mweststrate <https://github.com/mweststrate>, just a quick reminder
on this topic :)
Could you please answer my questions above?
—
Reply to this email directly, view it on GitHub
<#4606 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4NBAVLTQ4TQ2O43VAMH3XA6SDRANCNFSM6AAAAAAV476Z4E>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@sbatezat Great effort! Adding work profile support is an excellent feature. @mweststrate Could you please revisit for a review when you get the chance? Thanks. |
|
I've juste resolved conflicts and merged every changes you made |
|
Again some conflicts resolved to merge latest updates of the main branch. |
|
One year later.... |
|
@passy any help on this? |
|
I would love it if this change got merged! |
|
I'd still like to see this merged. @antonk52 (since I see you've contributed multiple times in the past month), do you think you can help us move this along? Not sure who would be the best person to review at this time. |
Summary
Add a way to select Android User to let Flipper working on devices with Work Profile.
Fix #953 & #4605
Changelog
Add a setting on settings sheet to select the Android User to use.
Useful to select the work profile on devices with two users (personal / work profile)
Comments
I don't know React. It works but help would be appreciate to improve code quality/tests. Please feel free to fix things that are not well written before merging this PR.
Known limitations