Add homepage mode settings

[lowrt]

這是什麼類型的 PR？

選擇所有符合的項目

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

描述

新增佈局樣式設定

相關 issue

• 相關問題 #
• closes #

QA 指南、截圖、錄像

UI 無障礙清單

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

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

[claude]

Claude finished @lowrt's task —— View job

---

Code Review Complete ✅

I've reviewed the PR that adds homepage mode settings. Here's my comprehensive feedback:

---

Overall Assessment

This PR successfully implements a homepage mode settings feature that allows users to toggle visibility of different homepage sections (weather, realtime info, radar, forecast, and history). The implementation is generally solid, but there are several areas for improvement.

---

Code Quality & Best Practices

✅ Strengths

1. Good separation of concerns: The new HomeDisplaySection enum cleanly defines the available sections
2. Proper state management: Uses Provider/ChangeNotifier pattern consistently
3. Persistent storage: Settings are properly saved to SharedPreferences
4. Semantic naming: Variable names are clear and follow Dart conventions

⚠️ Issues & Improvements Needed

1. Default Values Logic Issue (lib/models/settings/ui.dart:56-64)

SettingsUserInterfaceModel() {
  final saved = savedList
      .map((s) => HomeDisplaySection.values.firstWhere((e) => e.name == s,))
      .whereType<HomeDisplaySection>()
      .toSet();
  homeSections = saved.isNotEmpty
      ? saved
      : {HomeDisplaySection.weather};
}

Problems:

• The default only includes HomeDisplaySection.weather, but based on the code logic, all sections should be enabled by default (as seen in home/page.dart where all sections are rendered)
• If no preference is saved, users will only see the weather section, hiding other content unexpectedly
• The firstWhere call will throw if a section name doesn't match (no orElse provided)

Recommended fix:

SettingsUserInterfaceModel() {
  final saved = savedList
      .map((s) => HomeDisplaySection.values
          .firstWhere((e) => e.name == s, orElse: () => null))
      .whereType<HomeDisplaySection>()
      .toSet();
  homeSections = saved.isNotEmpty
      ? saved
      : HomeDisplaySection.values.toSet(); // Enable all sections by default
}

2. Missing State Reload (lib/core/preference.dart:128-136)

The toggleHomeSection method directly modifies SharedPreferences but doesn't notify the UI model:

static void toggleHomeSection(String section, bool enabled) {
  final current = homeDisplaySections;
  if (enabled) {
    if (!current.contains(section)) current.add(section);
  } else {
    current.remove(section);
  }
  instance.set(PreferenceKeys.homeDisplaySections, current);
}

This method is defined but never used. The SettingsUserInterfaceModel.toggleSection method is used instead, which is correct. Consider removing this unused method to avoid confusion.

3. List Mutation Bug (lib/core/preference.dart:129)

final current = homeDisplaySections;

This gets a reference to the list from SharedPreferences, then mutates it. This could cause issues if the getter returns a cached list. It's safer to create a new list:

final current = List<String>.from(homeDisplaySections);

---

Performance Considerations

Minor Issue: Unnecessary rebuilds (lib/app/home/page.dart:264)

final model = context.watch<SettingsUserInterfaceModel>();

The entire HomePage rebuilds when any UI setting changes (theme, locale, etc.), even though it only needs the homeSections setting. Consider using:

final homeSections = context.select<SettingsUserInterfaceModel, Set<HomeDisplaySection>>(
  (model) => model.homeSections
);

This will only rebuild when the sections actually change.

Performance impact: Minor, but could be noticeable on lower-end devices when users change theme settings frequently.

---

Potential Bugs

1. No minimum section enforcement (lib/models/settings/ui.dart:68-77)

Users can disable all sections, resulting in a blank homepage with only the location button. Consider enforcing at least one section to be enabled:

void toggleSection(HomeDisplaySection section, bool enabled) {
  if (!enabled && homeSections.length == 1 && homeSections.contains(section)) {
    // Don't allow disabling the last section
    return;
  }
  
  if (enabled) {
    homeSections.add(section);
  } else {
    homeSections.remove(section);
  }

  Preference.homeDisplaySections = homeSections.map((e) => e.name).toList();
  notifyListeners();
}

2. Missing error handling for malformed preferences

If SharedPreferences contains invalid section names (e.g., from a future version or corrupted data), the firstWhere in the constructor will throw an exception.

---

Security Concerns

✅ No security issues identified

The implementation doesn't handle sensitive data or expose security vulnerabilities. User preferences are properly scoped to the app.

---

Test Coverage

❌ No tests found

The repository has no test files. For a feature like this, consider adding:

1. Unit tests for SettingsUserInterfaceModel:

   • Test default section initialization
   • Test toggling sections on/off
   • Test persistence to SharedPreferences
   • Test malformed preference data handling

2. Widget tests for the settings UI:

   • Test that switches reflect current state
   • Test that toggling switches updates the model
   • Test that changes persist across app restarts

3. Integration tests:

   • Test that homepage sections appear/disappear based on settings
   • Test that settings survive app restarts

---

UI/Accessibility

From the screenshot review:

• ✅ Layout looks clean and consistent with the rest of the app
• ✅ Toggle switches are properly sized and easy to tap
• ⚠️ Color contrast testing incomplete (as noted in PR checklist)
• ⚠️ No i18n strings visible for section names in the screenshot

Recommendations:

1. Ensure all text passes WCAG AA color contrast requirements (especially the gray toggle state)
2. Test with system font scaling (accessibility settings)
3. Consider adding descriptions to each toggle for clarity

---

Code Organization

Minor suggestion: The home_display_mode.dart file only contains the enum. Consider either:

1. Moving the enum into lib/models/settings/ui.dart since it's closely related
2. Adding more home-related types/utilities to justify a separate file

---

Summary & Priority Issues

Must Fix (High Priority):

1. ⚠️ Fix default sections logic to include all sections by default
2. ⚠️ Add orElse to firstWhere to prevent crashes with invalid data
3. ⚠️ Remove unused Preference.toggleHomeSection method or document why it exists

Should Fix (Medium Priority):
4. ⚠️ Prevent users from disabling all homepage sections
5. ⚠️ Fix list mutation in Preference.toggleHomeSection if keeping the method
6. ⚠️ Optimize rebuild behavior using context.select

Nice to Have (Low Priority):
7. Add unit tests for the new functionality
8. Complete color contrast accessibility testing
9. Consider consolidating the enum location

---

The feature works as intended and the code is generally well-structured. The main concerns are around edge cases (empty sections, invalid data) and default behavior. I recommend addressing at least the high-priority issues before merging.

Great work on the semantic naming and consistent patterns throughout! 🎉

---

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

Code Review Complete ✅

I've reviewed the PR that adds homepage mode settings. Great work on iterating based on previous feedback! I can see you've already addressed several issues from the earlier review. Here's my analysis:

---

Overall Assessment

This PR successfully implements a homepage mode settings feature that allows users to toggle visibility of different homepage sections (realtime info, radar, forecast, and history). The implementation is clean and follows Flutter/Dart best practices well. I noticed you've already fixed several issues that were mentioned in the previous review (removed weather section, removed unused toggle method, fixed .cast() issue).

---

Critical Issues 🔴

1. Empty Sections State (lib/models/settings/ui.dart:56-63)

Issue: When no sections are saved (first-time user), homeSections is initialized as an empty set, causing the homepage to be completely blank except for the location button and weather header.

SettingsUserInterfaceModel() {
  final saved = savedList
      .map((s) => HomeDisplaySection.values
      .cast<HomeDisplaySection?>()
      .firstWhere((e) => e?.name == s, orElse: () => null))
      .whereType<HomeDisplaySection>()
      .toSet();
  homeSections = saved;  // ← Will be empty on first launch
}

Impact: First-time users will see a nearly empty homepage, which is confusing and looks broken.

Recommended fix:

SettingsUserInterfaceModel() {
  final saved = savedList
      .map((s) => HomeDisplaySection.values
      .cast<HomeDisplaySection?>()
      .firstWhere((e) => e?.name == s, orElse: () => null))
      .whereType<HomeDisplaySection>()
      .toSet();
  // Default to all sections enabled if no preference is saved
  homeSections = saved.isEmpty 
      ? HomeDisplaySection.values.toSet() 
      : saved;
}

---

Major Issues ⚠️

2. No Minimum Section Enforcement (lib/models/settings/ui.dart:68-76)

Issue: Users can disable all sections, resulting in a blank homepage with only the weather header and location button visible. This creates a poor UX where users might think the app is broken.

Current code:

void toggleSection(HomeDisplaySection section, bool enabled) {
  if (enabled) {
    homeSections.add(section);
  } else {
    homeSections.remove(section);
  }
  Preference.homeDisplaySections = homeSections.map((e) => e.name).toList();
  notifyListeners();
}

Recommended fix (enforce at least one section):

void toggleSection(HomeDisplaySection section, bool enabled) {
  // Prevent disabling the last remaining section
  if (!enabled && homeSections.length == 1 && homeSections.contains(section)) {
    return; // Or show a toast/snackbar explaining why
  }
  
  if (enabled) {
    homeSections.add(section);
  } else {
    homeSections.remove(section);
  }
  Preference.homeDisplaySections = homeSections.map((e) => e.name).toList();
  notifyListeners();
}

Alternative approach: You could also choose to allow all sections to be disabled if that's the intended behavior, but I'd recommend adding a placeholder message in page.dart when all sections are disabled to guide users.

3. Performance: Unnecessary Rebuilds (lib/app/home/page.dart:264)

Issue: The entire HomePage rebuilds whenever ANY UI setting changes (theme, locale, etc.), even though it only needs the homeSections setting.

final model = context.watch<SettingsUserInterfaceModel>();

Impact: Every time a user changes their theme or locale, the entire homepage rebuilds, including potentially expensive operations like map rendering.

Recommended fix:

final homeSections = context.select<SettingsUserInterfaceModel, Set<HomeDisplaySection>>(
  (model) => model.homeSections
);

// Then update the conditionals:
if (homeSections.contains(HomeDisplaySection.realtime))
  ..._buildRealtimeInfo(),
// etc.

Or create a helper method in the model:

// In SettingsUserInterfaceModel
bool isEnabledStatic(Set<HomeDisplaySection> sections, HomeDisplaySection section) {
  return sections.contains(section);
}

---

Minor Issues & Suggestions 💡

4. Code Organization

The home_display_mode.dart file contains only a 5-line enum. Consider:

• Option A: Move the enum into lib/models/settings/ui.dart since it's tightly coupled with the settings model
• Option B: Keep it separate but document why (perhaps you plan to add more home-related types later)

I'd lean toward Option A for simplicity unless you have plans to extend this file.

5. Preference Key Naming Consistency

In lib/core/preference.dart:29, you've defined homeDisplayMode but it's not currently used:

static const homeDisplayMode = 'pref:ui:homeDisplayMode';

If this is for future use, that's fine. Otherwise, consider removing unused keys to keep the codebase clean.

6. UI Polish: Section Ordering

The sections appear in a specific order in the settings UI (realtime, radar, forecast, history), but the enum definition is slightly different (realtime, radar, forecast, history). While they match now, consider adding a comment noting that the enum order should match the UI display order for consistency.

---

What's Working Well ✨

1. Excellent iteration: You've already fixed several issues from previous feedback:

   • Removed the unused weather section (commit aeaed45)
   • Fixed the homeSections == 1 issue (commit 9291217)
   • Removed the unused Preference.toggleHomeSection method
   • Added proper null handling with .cast() and orElse

2. Clean architecture: The separation between UI, model, and preferences is well-structured

3. Proper state management: Using ChangeNotifier correctly with immediate persistence to SharedPreferences

4. Semantic naming: Variables and methods are clearly named and follow Dart conventions

5. Consistent patterns: The implementation follows the same patterns as other settings (theme, locale, etc.)

---

Security & Data Integrity ✅

• No security concerns identified
• User preferences are properly scoped to the app
• No sensitive data handling

---

Testing Recommendations 🧪

Since the repository has no test files, I recommend adding:

1. Unit tests for SettingsUserInterfaceModel:

   • Test default initialization (empty preferences)
   • Test loading saved sections
   • Test toggling sections on/off
   • Test handling invalid section names from preferences

2. Widget tests for the settings UI:

   • Test that switches reflect current state
   • Test toggling behavior

3. Integration tests:

   • Test homepage sections appear/disappear based on settings
   • Test settings persist across app restarts

---

Accessibility Checklist 📱

From your PR description, you've checked "語意化命名" (semantic naming) ✅ but "顏色對比" (color contrast) is unchecked. Please verify:

• Toggle switches have sufficient color contrast in both on/off states
• Text labels are readable against the background
• Test with system font scaling enabled

---

Summary & Priority Recommendations

Must Fix Before Merge 🔴:

1. Fix empty sections initialization (lib/models/settings/ui.dart:63)
   • Set default to all sections enabled for new users

Strongly Recommended ⚠️:
2. Add minimum section enforcement or empty state UI
3. Optimize rebuild behavior with context.select

Nice to Have 💡:
4. Complete accessibility color contrast testing
5. Consider consolidating the enum location
6. Add unit tests for the new functionality

---

Final Verdict

This is a well-implemented feature with good code quality. The main concerns are around edge cases (empty sections on first launch, allowing all sections to be disabled) that could create a confusing user experience. Once the critical initialization issue is fixed, this will be ready to merge.

The iterative improvements you've made based on previous feedback show excellent development practices! 🎉

---

[lowrt]

@claude review

[claude]

Claude finished @lowrt's task —— View job

---

Code Review Complete ✅

I've reviewed the PR for the third time. Excellent progress! I can see you've addressed several critical issues from previous reviews. Here's my comprehensive analysis:

---

What's Been Fixed ✨

Great work on addressing previous feedback! I can see these improvements:

1. ✅ Performance optimization (commit b21b7ab): You've implemented context.select in lib/app/home/page.dart:264-266, which prevents unnecessary rebuilds. This is exactly what was recommended!

2. ✅ Empty sections UX (commit 19aecb4): You've added a helpful guidance prompt in lib/app/home/page.dart:303-319 when users disable all sections. This provides clear direction to users instead of leaving them with a blank page.

3. ✅ Previous iterations: The unused Preference.toggleHomeSection method was removed, and the weather section handling was cleaned up.

---

Critical Issue Remaining 🔴

Empty Initialization on First Launch (lib/models/settings/ui.dart:56-63)

Problem: When a user launches the app for the first time (no saved preferences), homeSections is initialized to an empty set, causing the homepage to show only the guidance prompt with no sections enabled by default.

SettingsUserInterfaceModel() {
  final saved = savedList
      .map((s) => HomeDisplaySection.values
      .cast<HomeDisplaySection?>()
      .firstWhere((e) => e?.name == s, orElse: () => null))
      .whereType<HomeDisplaySection>()
      .toSet();
  homeSections = saved;  // ← Empty on first launch!
}

Impact: New users see an empty homepage with a guidance message instead of seeing all sections by default. This creates a poor first-time experience.

Recommended fix:

SettingsUserInterfaceModel() {
  final saved = savedList
      .map((s) => HomeDisplaySection.values
      .cast<HomeDisplaySection?>()
      .firstWhere((e) => e?.name == s, orElse: () => null))
      .whereType<HomeDisplaySection>()
      .toSet();
  // Default to all sections enabled for new users
  homeSections = saved.isEmpty 
      ? HomeDisplaySection.values.toSet() 
      : saved;
}

This way, first-time users see all homepage sections, providing a better onboarding experience. Users can then customize which sections they want to hide.

---

Code Quality Assessment ⭐

Strengths:

• ✅ Clean architecture with proper separation of concerns
• ✅ Excellent use of Provider pattern for state management
• ✅ Performance-optimized with context.select (lines 264-266)
• ✅ Good UX with guidance prompts when sections are empty
• ✅ Proper null handling with .cast() and orElse
• ✅ Semantic naming throughout
• ✅ Consistent with existing codebase patterns

Structure:

• Enum definition: Clean and simple
• State management: Well-implemented with immediate persistence
• UI integration: Properly integrated with conditional rendering
• Settings page: Simple and intuitive

---

Minor Observations 💡

1. File organization (lib/app/home/home_display_mode.dart): The enum-only file is fine. If you prefer, you could move it into lib/models/settings/ui.dart, but the current structure is acceptable.

2. Unused preference key (lib/core/preference.dart:29): There's a homeDisplayMode constant defined but not used. This appears to be leftover. Consider removing it if it's not for future use.

3. Code style: The .cast<HomeDisplaySection?>() approach works, though it's a bit verbose. It properly handles potential mismatches in saved data.

---

Testing Recommendations 🧪

Since the repository has no test files, consider adding:

1. Unit tests for SettingsUserInterfaceModel:

   • Test first-time initialization (empty preferences)
   • Test loading saved sections
   • Test toggling sections
   • Test invalid section names from preferences

2. Widget tests for settings UI:

   • Test switch states
   • Test toggle interactions

3. Integration tests:

   • Test homepage section visibility based on settings
   • Test persistence across app restarts

---

Accessibility ♿

From your PR checklist:

• ✅ Semantic naming implemented
• ⚠️ Color contrast testing still incomplete

Please verify:

• Switch widgets have sufficient contrast in both states
• Text is readable with system font scaling enabled
• All interactive elements meet WCAG AA standards

---

Security & Performance ✅

• Security: No concerns identified
• Performance: Excellent! The context.select optimization (lines 264-266) ensures the HomePage only rebuilds when homeSections changes, not on every UI setting change

---

Summary

This is a well-implemented feature that's almost ready to merge. The code quality is good, and you've addressed most of the previous feedback excellently, especially the performance optimization and empty state UX.

The one critical issue is the empty initialization for first-time users. Once that's fixed to default to all sections enabled, this PR will provide a great user experience.

Priority:

• 🔴 Must fix: Default to all sections enabled on first launch
• 💡 Optional: Complete accessibility color contrast testing
• 💡 Nice to have: Add unit tests

Great iterative work on this feature! 🎉

---