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

lightbox: Add download button to bottom app bar #1144

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chimnayajith
Copy link
Contributor

@chimnayajith chimnayajith commented Dec 12, 2024

Pull Request

Description

This merge request introduces a new Download Image button to the bottom app bar, enabling users to download images.

When tapped, the button initiates a network request to download the image and saves it to the device's storage using the DownloadManager API which is implemented using pigeon.

Related Issues

Screenshots

Before After
before after

Additional Notes

  • The network request for downloading the image handles success, failure, and timeouts.
  • A test has been implemented to verify the download button functionality, ensuring the correct behavior is triggered and the SnackBar is displayed appropriately.

@chimnayajith chimnayajith force-pushed the main branch 2 times, most recently from ab03207 to fb8bab9 Compare December 12, 2024 17:40
@chimnayajith chimnayajith changed the title Add download button to lightbox to bottom app bar lightbox: add download button to lightbox to bottom app bar Dec 12, 2024
@chimnayajith chimnayajith changed the title lightbox: add download button to lightbox to bottom app bar lightbox: add download button to bottom app bar Dec 12, 2024
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Comments below from a quick skim. Please also update your commit message to follow our Git style. The part after lightbox: should start with a capital; also add an empty line and then Fixes: with the issue fixed in the commit.

@chimnayajith
Copy link
Contributor Author

@chrisbobbe I've made the changes as mentioned, Please take a look when you have time, Thank you.

@chimnayajith chimnayajith changed the title lightbox: add download button to bottom app bar lightbox: Add download button to bottom app bar Dec 14, 2024
@gnprice
Copy link
Member

gnprice commented Dec 23, 2024

See also the chat thread I started for sorting out the available APIs here:
https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/Android.20APIs.20for.20downloading.20an.20image/near/2010826

I'd like to hear there much the same information I asked for at #1144 (comment) above — that'd help us compare this API to alternatives and work out what solution we want.

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Dec 23, 2024
@chimnayajith chimnayajith force-pushed the main branch 2 times, most recently from 3ce76de to ad1c17f Compare January 2, 2025 06:33
@chimnayajith
Copy link
Contributor Author

chimnayajith commented Jan 2, 2025

@gnprice I've worked on the reviews you've given,

  • Implemented the same with pigeon ( still not sure if its the right way ).
  • Reduced the comments to just the needed ones.

Please take a look when you have time. Thank you

@chimnayajith chimnayajith requested a review from gnprice January 2, 2025 07:52
@gnprice
Copy link
Member

gnprice commented Jan 8, 2025

Thanks!

On a high-level scan, this structure looks good. The use of a new file pigeon/media_scanner.dart and the MediaScannerHost class look like the right structure for implementing a call to the Android side.

The main thing this will need is tests. In this version, the test case checks that a snack bar gets shown, but not any part of the actual functionality of the button. There's significant logic in that onPressed callback, and we'll want tests for that logic.

To do that, the main ingredient you'll need is to call the host scanFile method via a method you'll add to ZulipBinding, rather than calling it directly. See the advice in this section of our README:
https://github.com/zulip/zulip-flutter/?tab=readme-ov-file#writing-tests

For examples using Pigeon, see our notifications tests. For a simpler example though not using Pigeon, try looking at ZulipBinding.pickFiles, and the place where that's called, and the test in test/widgets/compose_box_test.dart that tests the place where pickFiles is called.

You might also need to add a ZulipBinding method for getExternalStorageDirectory, which should be a fairly simple binding method to add.

@chimnayajith chimnayajith force-pushed the main branch 5 times, most recently from 9be35a5 to 08b6583 Compare January 29, 2025 14:00
@chimnayajith
Copy link
Contributor Author

Modified the download implementation to use the DownloadManager API instead of the File API and MediaScannerConnection.scanFile() from the last review. Also wrote tests to verify the functionality of the download using TestZulipBinding.

@gnprice , could you please review it?

@gnprice
Copy link
Member

gnprice commented Feb 6, 2025

Thanks @chimnayajith for all your work on this!

I like this idea of using DownloadManager — I think that's likely to be the right strategy for us to use. The structure of this code looks broadly good, though I haven't reviewed it in detail.

This issue is for the M6 Post-launch milestone, though, and I've determined we need to start focusing more strictly on launch issues in order to get the app launched to our users on schedule. And there's still significant work ahead to get this completed, partly because there's always an extra level of care and validation required when dealing with platform APIs.

Given that, let's put this PR on hold for now. We can plan for you to pick this issue up from here when we're ready to resume work on post-launch issues.

@gnprice gnprice marked this pull request as draft February 6, 2025 02:30
@gnprice gnprice added this to the M6: Post-launch milestone Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lightbox: Add "download" button in bottom app bar
3 participants