Refactor map screen#203
Conversation
…o show count of active markers.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the map screen legend to properly count and display only the markers that are actually shown on the map. The main change involves modifying the _buildLegend method to calculate the node count by filtering contacts rather than accepting a pre-calculated count. Additionally, the marker filtering logic in _buildMarkers is updated to show only repeaters and rooms during path tracing mode.
Changes:
- Modified
_buildLegendsignature to acceptList<Contact>and settings instead of a pre-calculated node count, enabling accurate counting based on filter settings - Updated filter logic in
_buildMarkersto conditionally apply filters based on path trace mode - Added duplicate filter logic in
_buildLegendto count nodes consistently with what's displayed
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Widget _buildLegend(int nodeCount, int markerCount) { | ||
| Widget _buildLegend( | ||
| List<Contact> contactsWithLocation, | ||
| settings, |
There was a problem hiding this comment.
The settings parameter is missing a type annotation. It should be explicitly typed as AppSettings for better type safety and code clarity. This is consistent with the convention used elsewhere in the codebase where the settings object is obtained from settingsService.settings which returns an AppSettings instance (see line 119).
| ); | ||
| } | ||
|
|
||
| List<Marker> _buildMarkers(List<Contact> contacts, settings) { |
There was a problem hiding this comment.
The settings parameter is missing a type annotation. It should be explicitly typed as AppSettings for better type safety and code clarity. This is consistent with the convention used elsewhere in the codebase where the settings object is obtained from settingsService.settings which returns an AppSettings instance (see line 119).
| if (contact.type != advTypeChat && | ||
| contact.type != advTypeRepeater && | ||
| !settings.mapShowOtherNodes) { | ||
| (!settings.mapShowOtherNodes && !_isBuildingPathTrace)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
According to the PR description, path tracing mode should only show repeaters and rooms. However, this filter logic will show ALL "other" node types (including sensors) in path trace mode, not just rooms. The condition should specifically check for advTypeRoom when in path trace mode, or explicitly exclude advTypeSensor. Consider changing the logic to handle rooms separately: if in path trace mode, only allow repeaters (advTypeRepeater) and rooms (advTypeRoom) to pass through, while excluding chat nodes and sensors.
| Widget _buildLegend( | ||
| List<Contact> contactsWithLocation, | ||
| settings, | ||
| int markerCount, | ||
| ) { | ||
| int nodeCount = 0; | ||
| for (final contact in contactsWithLocation) { | ||
| // Apply node type filters | ||
| if (contact.type == advTypeRepeater && !settings.mapShowRepeaters) { | ||
| continue; | ||
| } | ||
| if (contact.type == advTypeChat && !settings.mapShowChatNodes) continue; | ||
| if (contact.type != advTypeChat && | ||
| contact.type != advTypeRepeater && | ||
| !settings.mapShowOtherNodes) { | ||
| continue; | ||
| } | ||
| nodeCount++; | ||
| } |
There was a problem hiding this comment.
The filter logic for counting nodes in _buildLegend is duplicated from _buildMarkers. This code duplication increases maintenance burden and risk of inconsistencies. Consider extracting this filtering logic into a separate helper method that both _buildMarkers and _buildLegend can call. This would ensure the same filter logic is applied consistently and only needs to be maintained in one place.
| Widget _buildLegend( | |
| List<Contact> contactsWithLocation, | |
| settings, | |
| int markerCount, | |
| ) { | |
| int nodeCount = 0; | |
| for (final contact in contactsWithLocation) { | |
| // Apply node type filters | |
| if (contact.type == advTypeRepeater && !settings.mapShowRepeaters) { | |
| continue; | |
| } | |
| if (contact.type == advTypeChat && !settings.mapShowChatNodes) continue; | |
| if (contact.type != advTypeChat && | |
| contact.type != advTypeRepeater && | |
| !settings.mapShowOtherNodes) { | |
| continue; | |
| } | |
| nodeCount++; | |
| } | |
| bool _shouldIncludeContactOnMap(Contact contact, dynamic settings) { | |
| if (contact.type == advTypeRepeater && !settings.mapShowRepeaters) { | |
| return false; | |
| } | |
| if (contact.type == advTypeChat && !settings.mapShowChatNodes) { | |
| return false; | |
| } | |
| if (contact.type != advTypeChat && | |
| contact.type != advTypeRepeater && | |
| !settings.mapShowOtherNodes) { | |
| return false; | |
| } | |
| return true; | |
| } | |
| Widget _buildLegend( | |
| List<Contact> contactsWithLocation, | |
| settings, | |
| int markerCount, | |
| ) { | |
| final int nodeCount = contactsWithLocation | |
| .where((contact) => _shouldIncludeContactOnMap(contact, settings)) | |
| .length; |
Refactor map screen legend to add filtering so count of markers shown is right.
In path tracing mode it so only shows markers for repeaters and rooms.