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

Install apk from the app - REQUEST_INSTALL_PACKAGES #1432

Merged
merged 6 commits into from
Sep 26, 2023

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Sep 26, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

The app is requesting REQUEST_INSTALL_PACKAGES permission, but some code was missing to handle this properly.

Also when the mime type is not know, try to guess it from the file extension.

I have also provided the link to the demo video (https://github.com/vector-im/element-x-android/assets/3940906/19d47d51-998a-4d82-a8a8-44a0f38317fe) to the PlayStore form.

First commit is a small cleanup.

Motivation and context

Fix rejection from PlayStore.

Screenshots / GIFs

Before After
InstallApk InstallApk_after

Tests

  • Send an APK to a timeline and try to open it from the app.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@bmarty bmarty requested a review from a team as a code owner September 26, 2023 12:12
@bmarty bmarty requested review from ganfra and removed request for a team September 26, 2023 12:12
@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Sep 26, 2023
@bmarty bmarty changed the title Install apk from the app Install apk from the app - REQUEST_INSTALL_PACKAGES Sep 26, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/qrtPvF

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (d8fbf21) 58.06% compared to head (0d00e8d) 58.02%.
Report is 7 commits behind head on develop.

❗ Current head 0d00e8d differs from pull request most recent head 3eb1123. Consider uploading reports for the commit 3eb1123 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1432      +/-   ##
===========================================
- Coverage    58.06%   58.02%   -0.05%     
===========================================
  Files         1129     1129              
  Lines        29880    29913      +33     
  Branches      6067     6079      +12     
===========================================
+ Hits         17351    17358       +7     
- Misses        9931     9955      +24     
- Partials      2598     2600       +2     
Files Coverage Δ
...ent/android/features/location/api/StaticMapView.kt 50.98% <100.00%> (ø)
...n/impl/screens/waitlistscreen/WaitListPresenter.kt 88.23% <100.00%> (ø)
...messages/impl/media/viewer/MediaViewerPresenter.kt 87.80% <100.00%> (ø)
...res/rageshake/impl/bugreport/BugReportPresenter.kt 91.17% <100.00%> (ø)
...raries/designsystem/swipe/SwipeableActionsState.kt 26.31% <100.00%> (ø)
.../libraries/designsystem/theme/components/Slider.kt 51.85% <100.00%> (ø)
...ibraries/matrix/ui/components/SelectedUsersList.kt 71.42% <100.00%> (ø)
...oid/libraries/matrix/impl/media/RustMediaLoader.kt 0.00% <0.00%> (ø)
...ures/messages/impl/media/viewer/MediaViewerView.kt 65.81% <66.66%> (+0.39%) ⬆️
...ement/android/libraries/core/mimetype/MimeTypes.kt 0.00% <0.00%> (ø)
... and 2 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ganfra ganfra left a comment

Choose a reason for hiding this comment

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

Just one remark otherwise LGTM

.setFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION)
.setDataAndType(localMedia.toShareableUri(), localMedia.info.mimeType)
withContext(coroutineDispatchers.main) {
activityContext!!.startActivity(openMediaIntent)
Copy link
Member

Choose a reason for hiding this comment

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

Please clean this part?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not change this, it was at line 106 in the previous version of the file. But I will add a commit to fix another potential issue.

@bmarty bmarty force-pushed the feature/bma/installApk branch from 0d00e8d to 3eb1123 Compare September 26, 2023 15:48
@bmarty bmarty enabled auto-merge September 26, 2023 15:49
@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

android:tint="?attr/colorControlNormal">
<path
android:fillColor="@android:color/white"
android:pathData="M160,880Q127,880 103.5,856.5Q80,833 80,800L80,160Q80,127 103.5,103.5Q127,80 160,80L480,80L720,320L720,490L640,490L640,360L440,360L440,160L160,160Q160,160 160,160Q160,160 160,160L160,800Q160,800 160,800Q160,800 160,800L600,800L600,880L160,880ZM160,800L160,490L160,490L160,360L160,160L160,160Q160,160 160,160Q160,160 160,160L160,800Q160,800 160,800Q160,800 160,800L160,800ZM200,760Q204,711 230,670Q256,629 298,605L260,537Q260,536 264,522Q269,520 273.5,520Q278,520 280,525L319,595Q339,587 359,582.5Q379,578 400,578Q421,578 441,582.5Q461,587 481,595L520,525Q520,525 535,521Q540,523 541,528Q542,533 540,537L502,605Q544,629 570,670Q596,711 600,760L200,760ZM310,700Q318,700 324,694Q330,688 330,680Q330,672 324,666Q318,660 310,660Q302,660 296,666Q290,672 290,680Q290,688 296,694Q302,700 310,700ZM490,700Q498,700 504,694Q510,688 510,680Q510,672 504,666Q498,660 490,660Q482,660 476,666Q470,672 470,680Q470,688 476,694Q482,700 490,700ZM800,880L640,720L696,663L760,726L760,560L840,560L840,726L904,663L960,720L800,880Z"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • ⚠️ Very long vector path (1005 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.

@bmarty bmarty merged commit f607b55 into develop Sep 26, 2023
@bmarty bmarty deleted the feature/bma/installApk branch September 26, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants