Skip to content

Commit 387e7d5

Browse files
vinodsingh-dev-gsocvinodsingh-dev-gsoc
committed
avatar: Show placeholder on image load error.
Previously, if an avatar image URL was invalid or failed to load, the UI would show a broken image icon. This commit improves the user experience by displaying a consistent `_AvatarPlaceholder` widget in all error cases, including network errors, invalid URLs that resolve to null, and null user objects. Fixes: zulip#1558.
1 parent 80f5a85 commit 387e7d5

File tree

4 files changed

+129
-20
lines changed

4 files changed

+129
-20
lines changed

‎lib/widgets/user.dart‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,8 @@ class AvatarImage extends StatelessWidget {
6767
final user = store.getUser(userId);
6868

6969
if (user == null) { // TODO(log)
70-
return const SizedBox.shrink();
70+
return _AvatarPlaceholder(size: size);
7171
}
72-
7372
if (replaceIfMuted && store.isUserMuted(userId)) {
7473
return _AvatarPlaceholder(size: size);
7574
}
@@ -80,7 +79,7 @@ class AvatarImage extends StatelessWidget {
8079
};
8180

8281
if (resolvedUrl == null) {
83-
return const SizedBox.shrink();
82+
return _AvatarPlaceholder(size: size);
8483
}
8584

8685
final avatarUrl = AvatarUrl.fromUserData(resolvedUrl: resolvedUrl);
@@ -90,6 +89,7 @@ class AvatarImage extends StatelessWidget {
9089
avatarUrl.get(physicalSize),
9190
filterQuality: FilterQuality.medium,
9291
fit: BoxFit.cover,
92+
errorBuilder: (_, _, _) => _AvatarPlaceholder(size: size),
9393
);
9494
}
9595
}

‎test/test_images.dart‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import 'dart:async';
22
import 'dart:io';
3-
3+
import 'dart:typed_data';
44
import 'package:flutter/widgets.dart';
55
import 'package:flutter_test/flutter_test.dart';
66

@@ -12,12 +12,18 @@ import 'package:flutter_test/flutter_test.dart';
1212
/// before the end of the test.
1313
// TODO(upstream) simplify callers by using addTearDown: https://github.com/flutter/flutter/issues/123189
1414
// See also: https://github.com/flutter/flutter/issues/121917
15-
FakeImageHttpClient prepareBoringImageHttpClient() {
15+
FakeImageHttpClient prepareBoringImageHttpClient({bool success = true}) {
1616
final httpClient = FakeImageHttpClient();
1717
debugNetworkImageHttpClientProvider = () => httpClient;
18-
httpClient.request.response
19-
..statusCode = HttpStatus.ok
20-
..content = kSolidBlueAvatar;
18+
if (success) {
19+
httpClient.request.response
20+
..statusCode = HttpStatus.ok
21+
..content = kSolidBlueAvatar;
22+
} else {
23+
httpClient.request.response
24+
..statusCode = HttpStatus.notFound
25+
..content = Uint8List(0);
26+
}
2127
return httpClient;
2228
}
2329

‎test/widgets/new_dm_sheet_test.dart‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -253,17 +253,31 @@ void main() {
253253
});
254254

255255
group('user selection', () {
256-
void checkUserSelected(WidgetTester tester, User user, bool expected) {
257-
final icon = tester.widget<Icon>(find.descendant(
258-
of: findUserTile(user),
259-
matching: find.byType(Icon)));
260256

257+
void checkUserSelected(WidgetTester tester, User user, bool expected) {
258+
final userTileFinder = findUserTile(user);
261259
if (expected) {
262260
check(findUserChip(user)).findsOne();
263-
check(icon).icon.equals(ZulipIcons.check_circle_checked);
261+
check(find.descendant(
262+
of: userTileFinder,
263+
matching: find.byIcon(ZulipIcons.check_circle_checked),
264+
)).findsOne();
265+
check(find.descendant(
266+
of: userTileFinder,
267+
matching: find.byIcon(ZulipIcons.check_circle_unchecked),
268+
)).findsNothing();
269+
264270
} else {
265271
check(findUserChip(user)).findsNothing();
266-
check(icon).icon.equals(ZulipIcons.check_circle_unchecked);
272+
check(find.descendant(
273+
of: userTileFinder,
274+
matching: find.byIcon(ZulipIcons.check_circle_unchecked),
275+
)).findsOne();
276+
277+
check(find.descendant(
278+
of: userTileFinder,
279+
matching: find.byIcon(ZulipIcons.check_circle_checked),
280+
)).findsNothing();
267281
}
268282
}
269283

‎test/widgets/user_test.dart‎

Lines changed: 95 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
import 'package:checks/checks.dart';
2-
import 'package:flutter/cupertino.dart';
32
import 'package:flutter/material.dart';
4-
import 'package:flutter/rendering.dart';
53
import 'package:flutter_test/flutter_test.dart';
64
import 'package:zulip/model/store.dart';
75
import 'package:zulip/widgets/content.dart';
6+
import 'package:zulip/widgets/icons.dart';
87
import 'package:zulip/widgets/store.dart';
8+
import 'package:zulip/widgets/theme.dart';
99
import 'package:zulip/widgets/user.dart';
10-
1110
import '../example_data.dart' as eg;
1211
import '../model/binding.dart';
1312
import '../model/test_store.dart';
1413
import '../stdlib_checks.dart';
1514
import '../test_images.dart';
15+
import 'test_app.dart';
1616

1717
void main() {
1818
TestZulipBinding.ensureInitialized();
@@ -28,9 +28,17 @@ void main() {
2828
await store.addUser(user);
2929

3030
prepareBoringImageHttpClient();
31-
await tester.pumpWidget(GlobalStoreWidget(
32-
child: PerAccountStoreWidget(accountId: eg.selfAccount.id,
33-
child: AvatarImage(userId: user.userId, size: size ?? 30))));
31+
await tester.pumpWidget(
32+
MaterialApp(
33+
theme: ThemeData(extensions: [
34+
DesignVariables.light,
35+
]),
36+
home: GlobalStoreWidget(
37+
child: PerAccountStoreWidget(accountId: eg.selfAccount.id,
38+
child: AvatarImage(userId: user.userId, size: size ?? 30)),
39+
),
40+
),
41+
);
3442
await tester.pump();
3543
await tester.pump();
3644
tester.widget(find.byType(AvatarImage));
@@ -78,5 +86,86 @@ void main() {
7886
check(await actualUrl(tester, avatarUrl)).isNull();
7987
debugNetworkImageHttpClientProvider = null;
8088
});
89+
90+
testWidgets('shows placeholder when image URL gives error', (WidgetTester tester) async {
91+
addTearDown(testBinding.reset);
92+
prepareBoringImageHttpClient(success: false);
93+
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
94+
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
95+
final badUser = eg.user(avatarUrl: 'https://zulip.com/avatarinvalid.png');
96+
await store.addUser(badUser);
97+
await tester.pumpWidget(
98+
MaterialApp(
99+
theme: ThemeData(extensions: [
100+
DesignVariables.light,
101+
]),
102+
home: TestZulipApp(
103+
accountId: eg.selfAccount.id,
104+
child: AvatarImage(userId: badUser.userId, size: 30))));
105+
await tester.pumpAndSettle();
106+
check(
107+
find.descendant(
108+
of: find.byType(AvatarImage),
109+
matching: find.byIcon(ZulipIcons.person),
110+
).evaluate().length
111+
).equals(1);
112+
debugNetworkImageHttpClientProvider = null;
113+
});
114+
115+
testWidgets('shows placeholder when user avatarUrl is null', (WidgetTester tester) async {
116+
addTearDown(testBinding.reset);
117+
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
118+
final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
119+
120+
final userWithNoUrl = eg.user(avatarUrl: null);
121+
await store.addUser(userWithNoUrl);
122+
123+
await tester.pumpWidget(
124+
MaterialApp(
125+
theme: ThemeData(extensions: [
126+
DesignVariables.light,
127+
]),
128+
home: TestZulipApp(
129+
accountId: eg.selfAccount.id,
130+
child: AvatarImage(userId: userWithNoUrl.userId, size: 30),
131+
),
132+
),
133+
);
134+
await tester.pumpAndSettle();
135+
136+
check(
137+
find.descendant(
138+
of: find.byType(AvatarImage),
139+
matching: find.byIcon(ZulipIcons.person),
140+
).evaluate().length
141+
).equals(1);
142+
});
143+
144+
testWidgets('shows placeholder when user is not found', (WidgetTester tester) async {
145+
addTearDown(testBinding.reset);
146+
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
147+
148+
const nonExistentUserId = 9999999;
149+
150+
await tester.pumpWidget(
151+
MaterialApp(
152+
theme: ThemeData(extensions: [
153+
DesignVariables.light,
154+
]),
155+
home: TestZulipApp(
156+
accountId: eg.selfAccount.id,
157+
child: AvatarImage(userId: nonExistentUserId, size: 30),
158+
),
159+
),
160+
);
161+
await tester.pumpAndSettle();
162+
163+
check(
164+
find.descendant(
165+
of: find.byType(AvatarImage),
166+
matching: find.byIcon(ZulipIcons.person),
167+
).evaluate().length
168+
).equals(1);
169+
});
81170
});
82171
}

0 commit comments

Comments
 (0)