lightbox: Add "share" button in bottom app bar #1145

shivanshsharma13 · 2024-12-12T19:20:56Z

Add a share button to the lightbox that allows users to share image URLs. The button appears in the bottom app bar with a share icon and tooltip. Test coverage includes verifying the share button's UI elements (icon and tooltip)

Screen recording of the change

share_button.mp4

Fixes: #43

rajveermalviya · 2024-12-12T20:43:54Z

Thanks for working on this @shivanshsharma13.
CI is failing currently because l10n generated code doesn't match, can you please fix that.

rajveermalviya

Thanks, excited to have this feature! Added some initial comments.

Also I see that there's currently a conflict, could you please rebase to main and resolve the conflicts. If you've any questions regarding that, you can refer to https://zulip.readthedocs.io/en/latest/git/index.html or ask in #git help channel in CZO.

rajveermalviya · 2024-12-12T19:32:09Z

assets/l10n/app_en.arb

+   "lightboxShareImageTooltip": "Share Image",
+    "@lightboxShareImageTooltip": {
+      "description": "Tooltip in lightbox for the Share Image action."
+    },


nit: dedent

Suggested change

"lightboxShareImageTooltip": "Share Image",

"@lightboxShareImageTooltip": {

"description": "Tooltip in lightbox for the Share Image action."

},

"lightboxShareImageTooltip": "Share Image",

"@lightboxShareImageTooltip": {

"description": "Tooltip in lightbox for the Share Image action."

},

rajveermalviya · 2024-12-12T19:34:19Z

lib/widgets/lightbox.dart

+        _ShareButton(url: widget.src),
        // TODO(#43): Share image


nit: Remove todo comment

rajveermalviya · 2024-12-12T19:40:54Z

assets/l10n/app_en.arb

@@ -338,6 +338,10 @@
  "@errorDialogTitle": {
    "description": "Generic title for error dialog."
  },
+  "errorShareFailed": "Error Sharing the Image",


Suggested change

"errorShareFailed": "Error Sharing the Image",

"errorShareFailed": "Failed to share the image",

rajveermalviya · 2024-12-12T19:41:11Z

assets/l10n/app_en.arb

@@ -346,6 +350,10 @@
  "@lightboxCopyLinkTooltip": {
    "description": "Tooltip in lightbox for the copy link action."
  },
+   "lightboxShareImageTooltip": "Share Image",


Suggested change

"lightboxShareImageTooltip": "Share Image",

"lightboxShareImageTooltip": "Share image",

rajveermalviya · 2024-12-12T19:45:22Z

lib/widgets/lightbox.dart

+  return XFile.fromData(bytes,
+      name: url.pathSegments.last,
+      mimeType: response.headers['content-type']);


nit: 2-space indentation, here and more case below.

Suggested change

return XFile.fromData(bytes,

name: url.pathSegments.last,

mimeType: response.headers['content-type']);

return XFile.fromData(bytes,

name: url.pathSegments.last,

mimeType: response.headers['content-type']);

rajveermalviya · 2024-12-12T19:56:02Z

lib/widgets/lightbox.dart

+              ...userAgentHeader()
+            };
+            final xFile = await _downloadImage(url, headers);
+            await Share.shareXFiles([xFile]);


This function should be wrapped and called using ZulipBinding to make it possible to write tests against.
Could be ZulipBinding.shareXFiles.

rajveermalviya · 2024-12-12T20:00:49Z

lib/widgets/lightbox.dart

@@ -1,7 +1,9 @@
 import 'package:flutter/material.dart';
 import 'package:flutter/scheduler.dart';
 import 'package:flutter/services.dart';
+import 'package:http/http.dart' as httpClient;


Suggested change

import 'package:http/http.dart' as httpClient;

import 'package:http/http.dart' as http;

rajveermalviya · 2024-12-12T20:04:19Z

test/widgets/lightbox_test.dart

@@ -275,6 +275,29 @@ void main() {
      debugNetworkImageHttpClientProvider = null;
    });

+    testWidgets('share button shows correct icon and downloads image', (tester) async {


This'll need more tests, namely:

Tapping on share button opens the share sheet (check if shareXFiles function was called).

Share failed and the error dialog was shown.

rajveermalviya · 2024-12-12T20:34:55Z

lib/widgets/lightbox.dart

+              ...userAgentHeader()
+            };
+            final xFile = await _downloadImage(url, headers);
+            await Share.shareXFiles([xFile]);


This function returns a ShareResult and it provides some important information we should check for, see ShareButton implementation in lib/widgets/action_sheet.dart.

rajveermalviya · 2024-12-12T20:35:48Z

lib/widgets/lightbox.dart

+                ...authHeader(email: store.account.email, apiKey: store.account.apiKey),
+              ...userAgentHeader()
+            };
+            final xFile = await _downloadImage(url, headers);


I see that it will download the canonical (larger) image again, first time it will be downloaded by RealmContentNetworkImage, and second time when pressing the share button here. Ideally, it'd be better to download the image only once, but I guess the http cache should handle that? Can you verify if cache is being used here.

shivanshsharma13 · 2024-12-13T17:35:01Z

Hi, thank you for the review! I’m working on the suggested changes and will let you know here once they are ready for another review.🙂

Add a share button to the lightbox that allows users to share image URLs. The button appears in the bottom app bar with a share icon and tooltip. Test coverage includes verifying the share button's UI elements (icon and tooltip). Fixes: zulip#43

rajveermalviya

Tried this on iOS and seems to work well on it too:

rajveermalviya · 2024-12-16T16:50:23Z

lib/widgets/lightbox.dart

+  final response = await http.get(url, headers: headers);
+  final bytes = response.bodyBytes;
+  return XFile.fromData(bytes,
+    name: url.pathSegments.last,


Missed this in last review — trying to get the filename this way seems fragile.

To do this correctly we will need changes in the content parser (lib/model/content.dart) to read the filename in ImageNode, but it may be out of scope for this PR. So, for now how about not populating the name field here, and leaving a TODO comment.

shivanshsharma13 force-pushed the share_lightbox branch 2 times, most recently from 7dcb6d1 to d6bf4c8 Compare December 12, 2024 19:39

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Dec 12, 2024

rajveermalviya self-assigned this Dec 12, 2024

shivanshsharma13 force-pushed the share_lightbox branch 3 times, most recently from 23e9cb0 to b1ac761 Compare December 13, 2024 08:24

rajveermalviya reviewed Dec 13, 2024

View reviewed changes

shivanshsharma13 force-pushed the share_lightbox branch from 56935f6 to 9ac3d16 Compare December 13, 2024 16:55

shivanshsharma13 force-pushed the share_lightbox branch 3 times, most recently from d5c283a to a99f96c Compare December 13, 2024 20:25

shivanshsharma13 force-pushed the share_lightbox branch from a99f96c to a8397e9 Compare December 13, 2024 20:28

rajveermalviya reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightbox: Add "share" button in bottom app bar #1145

lightbox: Add "share" button in bottom app bar #1145

shivanshsharma13 commented Dec 12, 2024 •

edited

Loading

rajveermalviya commented Dec 12, 2024

rajveermalviya left a comment

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

rajveermalviya Dec 12, 2024

shivanshsharma13 commented Dec 13, 2024

rajveermalviya left a comment

rajveermalviya Dec 16, 2024

	"errorShareFailed": "Error Sharing the Image",
	"errorShareFailed": "Failed to share the image",

	"lightboxShareImageTooltip": "Share Image",
	"lightboxShareImageTooltip": "Share image",

	import 'package:http/http.dart' as httpClient;
	import 'package:http/http.dart' as http;

lightbox: Add "share" button in bottom app bar #1145

Are you sure you want to change the base?

lightbox: Add "share" button in bottom app bar #1145

Conversation

shivanshsharma13 commented Dec 12, 2024 • edited Loading

rajveermalviya commented Dec 12, 2024

rajveermalviya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshsharma13 commented Dec 13, 2024

rajveermalviya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshsharma13 commented Dec 12, 2024 •

edited

Loading