Conversation
| @override | ||
| Widget build(BuildContext context, WidgetRef ref) { | ||
| final queueService = GetIt.instance<QueueService>(); | ||
| assert(item is Genre || item is Artist || (item is FinampSortable<Album> && item is FinampPlayable)); |
There was a problem hiding this comment.
I'd prefer to keep these checks in place, even if they're guaranteed with the currently implemented types.
There was a problem hiding this comment.
Makes sense. The FinampPlayable check is currently redundant, but keeping it as an explicit invariant is reasonable. I’ll restore it.
| autoScrollerVelocityScalar: 20.0, | ||
| onReorder: (oldIndex, newIndex) { | ||
| onReorderItem: (oldIndex, newIndex) { | ||
| final serviceNewIndex = oldIndex < newIndex ? newIndex + 1 : newIndex; |
There was a problem hiding this comment.
Has this been tested? I thought onReorderItem was expected to generally remove this sort of offset.
There was a problem hiding this comment.
Good catch. onReorderItem already provides the corrected insertion index, so my additional +1 adjustment is wrong. I’ll remove it from all three handlers.
| builtPath.addAll(["Items", item.imageId!, "Images", "Primary"]); | ||
| final Map<String, dynamic> queryParams = { | ||
| if (format != null) "format": format, | ||
| "format": ?format, |
There was a problem hiding this comment.
Have you run build-runner with these changes? I believe last time I saw, it couldn't handle files with this syntax.
There was a problem hiding this comment.
Not on the current branch. CI is currently failing the code-generation verification. I’ll revert this to the existing syntax.
|
|
||
| @override | ||
| Future<void> setRepeatMode(AudioServiceRepeatMode repeatMode) async { | ||
| if (repeatMode != AudioServiceRepeatMode.all && |
There was a problem hiding this comment.
I feel like pulling the errors out in these blocks just makes the logic more redundant.
There was a problem hiding this comment.
Agreed. Moving the checks out of the switch makes the logic more redundant. I’ll move them back, but keep rethrow so the original stack trace is preserved.
| _audioServiceBackgroundTaskLogger.severe("Custom action '$name' not found.", e); | ||
| } | ||
|
|
||
| if (actionFuture != null) { |
There was a problem hiding this comment.
I'm not sure actionFuture really makes this more understandable? I did notice a minor logic error in the existing code where actions like toggleRadio seem to be falling through super.customAction even though we're parsing them ourselves.
There was a problem hiding this comment.
Agreed. actionFuture doesn’t really improve this. You’re also right about the existing fall-through to super.customAction(). I’ll use explicit returns for handled actions instead.
|
I addressed the points and updated the branch. I also tested the current state successfully on a physical iPhone. Re-review would be appreciated. |
2 participants
Summary
This PR continues the analyzer cleanup after #1738 and groups the remaining follow-up work into one reviewable change set.
It includes:
RadioGroup,onReorderItem, and Navigator pop API migrationsBuildContexthandling where required by the analyzergenreselect case (discwas intended).g.dartand generated localization output instead of modifying generated filesGenerated
.g.dartfiles are intentionally not modified directly.Relation to #1738
This branch contains the follow-up analyzer cleanup and is based on the same original
redesignstate as #1738 rather than being stacked on top of that PR.Because of that, the branch by itself still contains the warnings already fixed in #1738.
After #1738 is merged, this branch can be rebased onto the updated
redesignbranch.Validation
The combined cleanup state including #1738 was tested locally:
flutter analyze: no issues foundflutter gen-l10n: succeedsconfirmAddToNextUpgenerates separategenreanddisccasesgit diff --check: cleanGenerated code is excluded from analyzer lint diagnostics rather than being edited manually.
The repository currently has no
test/directory, soflutter testis not applicable.