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

Simplify some sharing utilities #8064

Closed
wants to merge 1 commit into from

Conversation

TacoTheDank
Copy link
Member

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • These are handled internally by the methods I implemented in the PR.
    • At least I'm pretty sure.

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AudricV
Copy link
Member

AudricV commented Mar 18, 2022

Sorry, but I will probably close this PR.

Let me explain why: you are reverting my share sheet improvements on EMUI devices, which allow these devices to get the preview title (and soon the thumbnail of the content) of the content shared by using Android system chooser instead of EMUI chooser. See #5187 (the PR which introduced the changes) for the difference.

I know that's something we didn't have to do if Huawei (and also other OEMs) didn't change this, but as you can see, improvements when using the Android share sheet are pretty big in terms of information shown to the user.

Hope you can understand.

@opusforlife2
Copy link
Collaborator

@TiA4f8R Is the Huawei-specific code marked as such? Or do you have a comment somewhere which explains why that code shouldn't be touched?

@triallax triallax added the codequality Improvements to the codebase to improve the code quality label Mar 18, 2022
@TacoTheDank
Copy link
Member Author

image

Curses, foiled again! Damn you Android fragmentation!

@TacoTheDank
Copy link
Member Author

TacoTheDank commented Mar 19, 2022

But yeah I understand, I didn't even know that was intentional (I guess I should've looked at the git blame though).

Perhaps some code commenting is in order (as @opusforlife2 noted)?

(Also @TiA4f8R did you install the debug build and see if this PR actually did screw up the share sheet (seeing as you have an EMUI device)? I'd check, just in case. If you already did, then alrighty. 👍)

@AudricV
Copy link
Member

AudricV commented Mar 19, 2022

@TacoTheDank I did and the Android share sheet is still shown when sharing a content (for an unknown reason).

But the EMUI one is back when using the Open in browser command or when sharing a download (which was not the case for the last operation, even before my PR (the Android one was shown, unless Huawei did more changes in the Intent class with a security patch) (it tries to generate an hwCHOOSER intent first and fallbacks to the Android one if the first one is null).

@cketti
Copy link

cketti commented Mar 19, 2022

What is special about EMUI? Does Intent.createChooser() create an intent that is different from a manually created Intent.ACTION_CHOOSER intent?

@AudricV
Copy link
Member

AudricV commented Mar 20, 2022

What is special about EMUI? Does Intent.createChooser() create an intent that is different from a manually created Intent.ACTION_CHOOSER intent?

Yes, see my comment above:

it tries to generate an hwCHOOSER intent first and fallbacks to the Android one if the first one is null

(where the Android one is Intent.ACTION_CHOOSER)

@litetex
Copy link
Member

litetex commented Mar 21, 2022

Anyway I think we can close this or? (@TiA4f8R you should maybe create a new PR where you add a few comments/javadoc to the above lines)

@litetex litetex marked this pull request as draft March 22, 2022 18:02
@TacoTheDank
Copy link
Member Author

I'll close it.

@TacoTheDank TacoTheDank deleted the createChooser branch March 23, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants