Skip to content
This repository has been archived by the owner on Jul 24, 2023. It is now read-only.

Added branding exception to the ProviderPickerActivity #94

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Added branding exception to the ProviderPickerActivity #94

merged 1 commit into from
Sep 29, 2017

Conversation

dxslly
Copy link
Collaborator

@dxslly dxslly commented Sep 29, 2017

Google's provider implementation lives inside the Google Play Services APK which has a different application label and icon than user's expect. To avoid confusion, a special case is made to provide the correct branding. Ideally this should be solved at the protocol level (e.g. optional metadata specified in the manifest that can override these defaults for trusted providers).

// expect. To avoid confusion, a special case is made to provide the correct
// branding. Ideally this should be solved at the protocol level (e.g. optional
// metadata specified in the manifest that can override these defaults for
// trusted providers).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that we should be able to define this from the provider manifest.
I don't really like the fact that we introduce provider-specific images and text within this repository.
I understand that you need this right now and I'm ok to merge it, but we should plan to replace it as soon as possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also okay with this as a temporary measure, but agree that this should be handled properly in subsequent releases.

Copy link
Collaborator Author

@dxslly dxslly Sep 29, 2017

Choose a reason for hiding this comment

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

Agreed, this should be only be allowed temporarily for the Droidcon release. I have filed #96 to track its removal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If everyone is okay with this temporary fix, would you mind marking the PR as approved? Thanks!

Copy link
Collaborator

@nerdyverde nerdyverde left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@iainmcgin iainmcgin left a comment

Choose a reason for hiding this comment

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

I'll start tracking spec revisions to push for prior to DroidCon, this is clearly one we need to resolve sooner rather than later.

@dxslly dxslly merged commit 37faf5b into openid:master Sep 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants