-
-
Notifications
You must be signed in to change notification settings - Fork 466
new: custom tabs #889
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
new: custom tabs #889
Conversation
Fixes #887
Reviewer's GuideThis PR adds support for user-customizable home page tabs by introducing a new AppTab model with Hive persistence, wiring it into the settings store and UI, refactors SettingsStore property declarations for consistency, and updates the fl_lib dependency. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Remove or guard the dprint debug statements in SettingStore.homeTabs.fromObj to avoid noisy logs in production.
- Extract the AppTab parsing logic in homeTabs.fromObj into a standalone helper or utility function to improve readability and testability.
- Consider subscribing to changes in the homeTabs store within HomePage so that updating the tab order in settings takes effect immediately without a restart.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove or guard the dprint debug statements in SettingStore.homeTabs.fromObj to avoid noisy logs in production.
- Extract the AppTab parsing logic in homeTabs.fromObj into a standalone helper or utility function to improve readability and testability.
- Consider subscribing to changes in the homeTabs store within HomePage so that updating the tab order in settings takes effect immediately without a restart.
## Individual Comments
### Comment 1
<location> `lib/view/page/setting/entries/home_tabs.dart:81` </location>
<code_context>
+
+ Widget _buildTabItem(AppTab tab, int index, bool isSelected) {
+ return Card(
+ key: ValueKey('${tab.name}_$index'),
+ margin: const EdgeInsets.symmetric(horizontal: 16, vertical: 4),
+ child: ReorderableDragStartListener(
</code_context>
<issue_to_address>
Using index in ValueKey may cause unnecessary widget rebuilds.
Use a unique property like tab.name for the ValueKey to ensure widget identity remains stable when reordering.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
key: ValueKey('${tab.name}_$index'),
=======
key: ValueKey(tab.name),
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `lib/view/page/setting/entries/home_tabs.dart:83` </location>
<code_context>
+ return Card(
+ key: ValueKey('${tab.name}_$index'),
+ margin: const EdgeInsets.symmetric(horizontal: 16, vertical: 4),
+ child: ReorderableDragStartListener(
+ index: index,
+ child: ListTile(
</code_context>
<issue_to_address>
ReorderableDragStartListener is used for both selected and available tabs.
Only wrap selected tab items with ReorderableDragStartListener to prevent drag handles from appearing on non-reorderable available tabs.
</issue_to_address>
### Comment 3
<location> `lib/view/page/setting/entries/home_tabs.dart:117` </location>
<code_context>
+ }
+
+ void _removeTab(AppTab tab) {
+ if (_selectedTabs.length <= 1) {
+ context.showSnackBar(l10n.atLeastOneTab);
+ return;
+ }
</code_context>
<issue_to_address>
Preventing removal of the last tab is good, but consider disabling the delete icon.
Disabling or hiding the delete icon when only one tab remains would improve user experience by preventing unnecessary actions.
Suggested implementation:
```
void _removeTab(AppTab tab) {
```
```
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: CustomAppBar(
title: Text(l10n.homeTabs),
),
body: ListView.builder(
itemCount: _selectedTabs.length,
itemBuilder: (context, index) {
final tab = _selectedTabs[index];
final canRemove = _selectedTabs.length > 1;
return ListTile(
title: Text(tab.label),
trailing: IconButton(
icon: Icon(Icons.delete),
onPressed: canRemove ? () => _removeTab(tab) : null,
color: canRemove ? null : Theme.of(context).disabledColor,
tooltip: canRemove ? l10n.removeTab : l10n.atLeastOneTab,
),
);
},
),
```
- If you use a custom widget for tab list items, apply the same logic to the delete icon there.
- If you want to hide the icon instead of disabling it, replace the `IconButton` with `canRemove ? IconButton(...) : SizedBox.shrink()`.
- Make sure `tab.label` and `l10n.removeTab`/`l10n.atLeastOneTab` exist or adjust as needed for your localization and tab model.
</issue_to_address>
### Comment 4
<location> `lib/data/model/app/tab.g.dart:31` </location>
<code_context>
+
+ @override
+ void write(BinaryWriter writer, AppTab obj) {
+ switch (obj) {
+ case AppTab.server:
+ writer.writeByte(0);
+ case AppTab.ssh:
+ writer.writeByte(1);
+ case AppTab.file:
+ writer.writeByte(2);
+ case AppTab.snippet:
+ writer.writeByte(3);
+ }
</code_context>
<issue_to_address>
Missing break statements in switch cases may cause unintended fallthrough.
Add break statements after each case in the switch to prevent unintended fallthrough and ensure only the correct byte is written.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| Widget _buildTabItem(AppTab tab, int index, bool isSelected) { | ||
| return Card( | ||
| key: ValueKey('${tab.name}_$index'), |
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.
suggestion: Using index in ValueKey may cause unnecessary widget rebuilds.
Use a unique property like tab.name for the ValueKey to ensure widget identity remains stable when reordering.
| key: ValueKey('${tab.name}_$index'), | |
| key: ValueKey(tab.name), |
| return Card( | ||
| key: ValueKey('${tab.name}_$index'), | ||
| margin: const EdgeInsets.symmetric(horizontal: 16, vertical: 4), | ||
| child: ReorderableDragStartListener( |
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.
suggestion: ReorderableDragStartListener is used for both selected and available tabs.
Only wrap selected tab items with ReorderableDragStartListener to prevent drag handles from appearing on non-reorderable available tabs.
| if (_selectedTabs.length <= 1) { | ||
| context.showSnackBar(l10n.atLeastOneTab); |
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.
suggestion: Preventing removal of the last tab is good, but consider disabling the delete icon.
Disabling or hiding the delete icon when only one tab remains would improve user experience by preventing unnecessary actions.
Suggested implementation:
void _removeTab(AppTab tab) {
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: CustomAppBar(
title: Text(l10n.homeTabs),
),
body: ListView.builder(
itemCount: _selectedTabs.length,
itemBuilder: (context, index) {
final tab = _selectedTabs[index];
final canRemove = _selectedTabs.length > 1;
return ListTile(
title: Text(tab.label),
trailing: IconButton(
icon: Icon(Icons.delete),
onPressed: canRemove ? () => _removeTab(tab) : null,
color: canRemove ? null : Theme.of(context).disabledColor,
tooltip: canRemove ? l10n.removeTab : l10n.atLeastOneTab,
),
);
},
),
- If you use a custom widget for tab list items, apply the same logic to the delete icon there.
- If you want to hide the icon instead of disabling it, replace the
IconButtonwithcanRemove ? IconButton(...) : SizedBox.shrink(). - Make sure
tab.labelandl10n.removeTab/l10n.atLeastOneTabexist or adjust as needed for your localization and tab model.
Fixes #887
Summary by Sourcery
Enable users to customize which tabs appear on the home page and their order by introducing a persistent homeTabs setting, a new AppTab model with Hive support, and a dedicated configuration UI in the app settings.
New Features:
Enhancements:
Build:
Chores: