-
Notifications
You must be signed in to change notification settings - Fork 253
fix(dashboard): display dashboard images configured via external URL link #249
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
base: develop/1.9.0
Are you sure you want to change the base?
Changes from 4 commits
48790cb
c7a0292
1eb4637
23eb34e
6e4f884
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,8 +85,8 @@ abstract class Utils { | |
| onError: onError, | ||
| ); | ||
| } else { | ||
| final newImageUrl = _removeTbImagePrefix(imageUrl); | ||
| if (_isImageResourceUrl(newImageUrl)) { | ||
| final resolvedImageUrl = _removeTbImagePrefix(imageUrl); | ||
| if (_isImageResourceUrl(resolvedImageUrl)) { | ||
| final jwtToken = tbClient.getJwtToken(); | ||
| if (jwtToken == null) { | ||
| return _onErrorImage( | ||
|
|
@@ -98,7 +98,7 @@ abstract class Utils { | |
| onError: onError, | ||
| ); | ||
| } | ||
| final parts = newImageUrl.split('/'); | ||
| final parts = resolvedImageUrl.split('/'); | ||
| final key = parts[parts.length - 1]; | ||
| parts[parts.length - 1] = Uri.encodeComponent(key); | ||
| final encodedUrl = parts.join('/'); | ||
|
|
@@ -115,29 +115,31 @@ abstract class Utils { | |
| semanticLabel: semanticLabel, | ||
| onError: onError, | ||
| ); | ||
| } else if (_isBase64DataImageUrl(imageUrl)) { | ||
| } else if (_isBase64DataImageUrl(resolvedImageUrl)) { | ||
| return _imageFromBase64( | ||
| context, | ||
| imageUrl, | ||
| color: color, | ||
| width: width, | ||
| height: height, | ||
| semanticLabel: semanticLabel, | ||
| onError: onError, | ||
| ); | ||
| } else if (_isValidUrl(imageUrl)) { | ||
| return _networkImage( | ||
| context, | ||
| imageUrl, | ||
| resolvedImageUrl, | ||
| color: color, | ||
| width: width, | ||
| height: height, | ||
| semanticLabel: semanticLabel, | ||
| onError: onError, | ||
| ); | ||
| } else { | ||
| return _onErrorImage( | ||
| final imageLink = _resolveNetworkImageLink(resolvedImageUrl); | ||
| if (imageLink == null) { | ||
| return _onErrorImage( | ||
| context, | ||
| color: color, | ||
| width: width, | ||
| height: height, | ||
| semanticLabel: semanticLabel, | ||
| onError: onError, | ||
| ); | ||
| } | ||
| return _networkImage( | ||
| context, | ||
| imageLink, | ||
| color: color, | ||
| width: width, | ||
| height: height, | ||
|
|
@@ -287,15 +289,31 @@ abstract class Utils { | |
| } | ||
|
|
||
| static String _removeTbImagePrefix(String url) { | ||
| return url.replaceFirst(_tbImagePrefix, ''); | ||
| return url.startsWith(_tbImagePrefix) | ||
| ? url.substring(_tbImagePrefix.length) | ||
| : url; | ||
| } | ||
|
|
||
| static bool _isImageResourceUrl(String url) { | ||
| return _imagesUrlRegexp.hasMatch(url); | ||
| } | ||
|
|
||
| static bool _isValidUrl(String url) { | ||
| return Uri.tryParse(url) != null; | ||
| /// HTTP(S) links are fetched as they are. Links relative to the platform, | ||
| /// such as an image public link, are resolved against the active endpoint. | ||
| /// Anything else has no meaningful target and is rendered as a missing | ||
| /// image. | ||
| static String? _resolveNetworkImageLink(String url) { | ||
| final uri = Uri.tryParse(url); | ||
| if (uri == null) { | ||
| return null; | ||
| } | ||
| if (uri.scheme == 'http' || uri.scheme == 'https') { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only nit I have on the new helper: the comment above says "Absolute links are fetched as they are", but this gate passes only For the record I did go looking for a hole in this gate and didn't find one worth acting on:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 1eb4637 — reworded to "HTTP(S) links are fetched as they are" so the scheme gate reads as intentional. |
||
| return url; | ||
| } | ||
| if (!uri.hasScheme && url.startsWith('/')) { | ||
| return getIt<IEndpointService>().getCachedEndpoint() + url; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| static double degreesToRadians(double degrees) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One imprecision left in the reworded sentence, and I am recording it rather than asking for anything — I said last round that a single imprecise clause was not worth a third pass at this comment, and that still applies. Take it only if you touch the file again.
"Links relative to the platform" is a shade broader than the gate below it.
!uri.hasScheme && url.startsWith('/')accepts only root-relative links, soapi/images/public/{key}without the leading slash falls through tonull, as doimages/logo.png,./logo.pngand../logo.png— checked on Dart 3.29.0. That is the correct behaviour, sincegetPublicLink()andgetLink()both return rooted paths and the platform never emits the other form, but "root-relative" would tell the next reader exactly where the boundary sits.I also had a second wording point — that the closing sentence describes what the caller does with
nullrather than what this function returns — and dropped it after checking: the helper has exactly one call site (utils.dart:129), so describing the end-to-end behaviour is more useful here than documenting thenullcontract, and changing it would not be an improvement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6e4f884 — reworded to "Root-relative links" so the sentence matches the leading-slash gate.