Implement dynamic weather text width to prevent overflow

[lowrt]

這是什麼類型的 PR？

選擇所有符合的項目

• 重構
• 新功能
• 錯誤修復
• 最佳化
• 技術文件更新

描述

相關 issue

• 相關問題 #
• closes #

QA 指南、截圖、錄像

請將這行替換成：如何測試您的 PR 的步驟，已測試的裝置註釋，以及任何相關的 UI 更改圖片。

UI 無障礙清單

如果您的 PR 包含 UI 更改，請使用此清單：

• 變數名稱實現語意化命名？
• 測試通過 AA 顏色對比？

[claude]

Claude finished @lowrt's task —— View job

---

PR Review: Fix forecast

Todo List:

• Check for repository setup instructions (CLAUDE.md)
• Review android/app/build.gradle changes
• Review lib/app/home/_widgets/forecast_card.dart changes
• Analyze code for Flutter best practices
• Check for potential bugs and issues
• Provide comprehensive feedback

---

Summary

This PR fixes the forecast card display by changing pagination from 4 items per page to 6 items per page and implements dynamic width calculation for weather text to prevent overflow issues. The changes look good overall with some minor considerations.

Changes Overview

1. Build Version Bump (android/app/build.gradle)

• ✅ Version code incremented from 300103103 to 300103104
• Standard version bump for release tracking

2. Forecast Card Improvements (lib/app/home/_widgets/forecast_card.dart)

Key Changes:

• Changed pagination from 4 items/page to 6 items/page
• Added dynamic width calculation (maxWeatherWidth) based on actual text content
• Added caching mechanism (_lastForecast) to avoid recalculating width on every rebuild
• Improved memory management by cleaning up _measuredHeights for non-current pages
• Changed weather text from TextOverflow.ellipsis to TextOverflow.visible
• Removed Flexible wrapper from weather text (now uses fixed calculated width)
• Removed spacing: 2 from one Row widget

Code Quality Assessment

✅ Strengths

1. Performance Optimization: The caching mechanism (_lastForecast != data) prevents unnecessary TextPainter calculations on every rebuild
2. Dynamic Layout: Calculating maxWeatherWidth based on actual content ensures consistent alignment across all items
3. Memory Management: Cleanup of _measuredHeights for non-visible pages is good practice
4. Consistent Updates: All index calculations correctly updated from * 4 to * 6 throughout the code

⚠️ Potential Issues & Improvements

1. Memory Leak Risk - PageController (Minor)

Location: lib/app/home/_widgets/forecast_card.dart:18

The PageController is properly disposed, but there's a potential edge case:

final PageController _pageController = PageController();

Issue: If the widget is rebuilt with different forecast data while maintaining the same page position, the controller might be on a page that no longer exists.

Suggestion: Consider resetting the page controller when forecast data changes:

if (_lastForecast != data) {
  _lastForecast = data;
  _currentPage = 0;
  if (_pageController.hasClients) {
    _pageController.jumpToPage(0);
  }
  // ... rest of width calculation
}

2. TextPainter Not Disposed (Minor Performance Issue)

Location: lib/app/home/_widgets/forecast_card.dart:50-60

Issue: TextPainter objects are created in a loop but never disposed, which can lead to minor memory leaks over time.

Suggestion:

for (final text in allWeatherTexts) {
  final tp = TextPainter(
    text: TextSpan(
      text: text,
      style: context.theme.textTheme.bodySmall?.copyWith(
        fontWeight: FontWeight.w500,
        fontSize: 12,
      ),
    ),
    maxLines: 1,
    textDirection: TextDirection.ltr,
  )..layout(minWidth: 0, maxWidth: double.infinity);

  longestWidth = max(longestWidth, tp.width);
  tp.dispose(); // Add this line
}

3. Potential Text Overflow (Low Risk)

Location: lib/app/home/_widgets/forecast_card.dart:321

Changed from: overflow: TextOverflow.ellipsis → overflow: TextOverflow.visible

Issue: While the width is pre-calculated, if there's ever a mismatch between the TextPainter calculation and the actual rendering (e.g., different font rendering on different devices), text could overflow the bounds.

Suggestion: Consider using TextOverflow.clip instead of visible for a cleaner appearance if overflow occurs, or add a safeguard:

Text(
  weather,
  style: context.theme.textTheme.bodySmall?.copyWith(
    fontWeight: FontWeight.w500,
    color: context.colors.onSurfaceVariant,
    fontSize: 12,
  ),
  maxLines: 1,
  overflow: TextOverflow.clip, // or ellipsis as fallback
),

4. Missing Null Safety Check (Very Low Risk)

Location: lib/app/home/_widgets/forecast_card.dart:44

final w = item['weather'] as String?;

This assumes item is always a Map. If the data structure changes, this could throw. Consider:

final w = (item as Map<String, dynamic>?)?['weather'] as String?;

5. Spacing Inconsistency

Location: lib/app/home/_widgets/forecast_card.dart:332

The spacing: 2 was removed from the Row containing the rain icon, but line 303 still has spacing: 4. This is intentional but worth confirming the spacing should be different.

Flutter Best Practices Assessment

✅ Good practices observed:

• Proper widget lifecycle management (dispose)
• State management with setState
• Use of const constructors where appropriate
• Error handling with try-catch
• Responsive layout with LayoutBuilder

⚠️ Minor improvements:

• Consider extracting the width calculation logic into a separate method for better readability
• The build method is quite long (231 lines); consider breaking down into smaller widget methods

Testing Recommendations

Since I cannot run the Flutter test suite without proper setup, please ensure:

1. ✅ Run flutter analyze to check for any static analysis issues
2. ✅ Run flutter test to ensure all existing tests pass
3. ✅ Test on different screen sizes to verify the dynamic width calculation works correctly
4. ✅ Test with various weather text lengths (especially long Chinese characters)
5. ✅ Test pagination with different forecast data lengths (0-24+ items)
6. ✅ Verify memory usage doesn't increase on repeated page changes
7. ✅ Test expand/collapse functionality across page boundaries

Security & Performance

• ✅ No security concerns identified
• ✅ Performance improvement from caching width calculations
• ⚠️ Minor memory improvement possible by disposing TextPainter objects

Verdict

Recommendation: ✅ Approve with minor suggestions

The PR successfully addresses the forecast display issues and introduces a smart dynamic width calculation. The code is well-structured and shows good understanding of Flutter performance considerations. The suggested improvements are minor and don't block merging.

The main change (4→6 items per page) improves UX by showing more information at once, and the dynamic width calculation is a robust solution to the text overflow problem mentioned in PR #480.

Priority fixes before merge:

• None critical

Nice-to-have improvements:

• Dispose TextPainter objects in the width calculation loop
• Consider adding page reset when forecast data changes
• Consider TextOverflow.clip instead of .visible for safety

---

[lowrt]

@claude tidy up PR title

[claude]

Claude finished @lowrt's task —— View job

---

Tidying up PR title

• Read changed files to understand what was fixed
• Analyze the forecast fix details
• Propose improved PR title
• Update PR title

---

Analysis

I've analyzed the changes in this PR. The current title "Fix forecast" is too vague. Based on the code changes, this PR does two main things:

1. Increases forecast items per page: Changes pagination from 4 items per page to 6 items per page (see line 80 and multiple related calculations)
2. Fixes weather text overflow: Implements dynamic width calculation for weather text to prevent overflow issues, replacing the fixed width of 135px with a calculated maxWeatherWidth based on the longest weather description
3. Minor version bump: Updates build version from 300103103 to 300103104

Recommended PR Title

Since I don't have permission to update PR metadata directly, here are some suggested improved titles:

Option 1 (Concise):

fix: Increase forecast items per page from 4 to 6 and fix weather text overflow

Option 2 (More detailed):

fix(forecast): Increase pagination to 6 items and implement dynamic weather text width

Option 3 (Focus on the main fix):

fix(forecast): Implement dynamic weather text width to prevent overflow

I recommend Option 2 as it clearly describes both major changes while following conventional commit format.

You can update the PR title by clicking the "Edit" button next to the title at the top of this PR page, or by running:

gh pr edit 482 --title "fix(forecast): Increase pagination to 6 items and implement dynamic weather text width"