diff --git a/assets/changelog.json b/assets/changelog.json index 9209a0c..4b9b9be 100644 --- a/assets/changelog.json +++ b/assets/changelog.json @@ -1,4 +1,10 @@ { + "2.8.1": { + "content": [ + "• Fixed bug where the \"existing tags\" profile would incorrectly add default FOV ranges during submission", + "• Added drag handles so profiles can be reordered to customize dropdown order when submitting" + ] + }, "2.8.0": { "content": [ "• Update dependencies and build chain tools; no code changes" diff --git a/lib/app_state.dart b/lib/app_state.dart index 0e8ae09..a208d63 100644 --- a/lib/app_state.dart +++ b/lib/app_state.dart @@ -407,6 +407,10 @@ class AppState extends ChangeNotifier { _profileState.addOrUpdateProfile(p); } + void reorderProfiles(int oldIndex, int newIndex) { + _profileState.reorderProfiles(oldIndex, newIndex); + } + void deleteProfile(NodeProfile p) { _profileState.deleteProfile(p); } diff --git a/lib/models/node_profile.dart b/lib/models/node_profile.dart index cdf996a..cc8612a 100644 --- a/lib/models/node_profile.dart +++ b/lib/models/node_profile.dart @@ -269,16 +269,33 @@ class NodeProfile { /// Used as the default `` option when editing nodes /// All existing tags will flow through as additionalExistingTags static NodeProfile createExistingTagsProfile(OsmNode node) { - // Calculate FOV from existing direction ranges if applicable + // Only assign FOV if the original direction string actually contained range notation + // (e.g., "90-270" or "55-125"), not if it was just single directions (e.g., "90") double? calculatedFov; - // If node has direction/FOV pairs, check if they all have the same FOV - if (node.directionFovPairs.isNotEmpty) { - final firstFov = node.directionFovPairs.first.fovDegrees; + final raw = node.tags['direction'] ?? node.tags['camera:direction']; + if (raw != null) { + // Check if any part of the direction string contains range notation (dash with numbers) + final parts = raw.split(';'); + bool hasRangeNotation = false; - // If all directions have the same FOV, use it for the profile - if (node.directionFovPairs.every((df) => df.fovDegrees == firstFov)) { - calculatedFov = firstFov; + for (final part in parts) { + final trimmed = part.trim(); + // Look for range pattern: numbers-numbers (e.g., "90-270", "55-125") + if (trimmed.contains('-') && RegExp(r'^\d+\.?\d*-\d+\.?\d*$').hasMatch(trimmed)) { + hasRangeNotation = true; + break; + } + } + + // Only calculate FOV if the node originally had range notation + if (hasRangeNotation && node.directionFovPairs.isNotEmpty) { + final firstFov = node.directionFovPairs.first.fovDegrees; + + // If all directions have the same FOV, use it for the profile + if (node.directionFovPairs.every((df) => df.fovDegrees == firstFov)) { + calculatedFov = firstFov; + } } } @@ -290,7 +307,7 @@ class NodeProfile { requiresDirection: true, submittable: true, editable: false, - fov: calculatedFov, // Use calculated FOV from existing direction ranges + fov: calculatedFov, // Only use FOV if original had explicit range notation ); } diff --git a/lib/screens/settings/sections/node_profiles_section.dart b/lib/screens/settings/sections/node_profiles_section.dart index ed997f0..89fcdaf 100644 --- a/lib/screens/settings/sections/node_profiles_section.dart +++ b/lib/screens/settings/sections/node_profiles_section.dart @@ -34,76 +34,101 @@ class NodeProfilesSection extends StatelessWidget { ), ], ), - ...appState.profiles.map( - (p) => ListTile( - leading: Checkbox( - value: appState.isEnabled(p), - onChanged: (v) => appState.toggleProfile(p, v ?? false), - ), - title: Text(p.name), - subtitle: Text(p.builtin ? locService.t('profiles.builtIn') : locService.t('profiles.custom')), - trailing: !p.editable - ? PopupMenuButton( - itemBuilder: (context) => [ - PopupMenuItem( - value: 'view', - child: Row( - children: [ - const Icon(Icons.visibility), - const SizedBox(width: 8), - Text(locService.t('profiles.view')), - ], - ), + ReorderableListView.builder( + shrinkWrap: true, + physics: const NeverScrollableScrollPhysics(), + itemCount: appState.profiles.length, + onReorder: (oldIndex, newIndex) { + appState.reorderProfiles(oldIndex, newIndex); + }, + itemBuilder: (context, index) { + final p = appState.profiles[index]; + return ListTile( + key: ValueKey(p.id), + leading: Row( + mainAxisSize: MainAxisSize.min, + children: [ + // Drag handle + ReorderableDragStartListener( + index: index, + child: const Icon( + Icons.drag_handle, + color: Colors.grey, ), - ], - onSelected: (value) { - if (value == 'view') { - Navigator.push( - context, - MaterialPageRoute( - builder: (_) => ProfileEditor(profile: p), + ), + const SizedBox(width: 8), + // Checkbox + Checkbox( + value: appState.isEnabled(p), + onChanged: (v) => appState.toggleProfile(p, v ?? false), + ), + ], + ), + title: Text(p.name), + subtitle: Text(p.builtin ? locService.t('profiles.builtIn') : locService.t('profiles.custom')), + trailing: !p.editable + ? PopupMenuButton( + itemBuilder: (context) => [ + PopupMenuItem( + value: 'view', + child: Row( + children: [ + const Icon(Icons.visibility), + const SizedBox(width: 8), + Text(locService.t('profiles.view')), + ], ), - ); - } - }, - ) - : PopupMenuButton( - itemBuilder: (context) => [ - PopupMenuItem( - value: 'edit', - child: Row( - children: [ - const Icon(Icons.edit), - const SizedBox(width: 8), - Text(locService.t('actions.edit')), - ], ), - ), - PopupMenuItem( - value: 'delete', - child: Row( - children: [ - const Icon(Icons.delete, color: Colors.red), - const SizedBox(width: 8), - Text(locService.t('profiles.deleteProfile'), style: const TextStyle(color: Colors.red)), - ], - ), - ), - ], - onSelected: (value) { - if (value == 'edit') { - Navigator.push( - context, - MaterialPageRoute( - builder: (_) => ProfileEditor(profile: p), + ], + onSelected: (value) { + if (value == 'view') { + Navigator.push( + context, + MaterialPageRoute( + builder: (_) => ProfileEditor(profile: p), + ), + ); + } + }, + ) + : PopupMenuButton( + itemBuilder: (context) => [ + PopupMenuItem( + value: 'edit', + child: Row( + children: [ + const Icon(Icons.edit), + const SizedBox(width: 8), + Text(locService.t('actions.edit')), + ], ), - ); - } else if (value == 'delete') { - _showDeleteProfileDialog(context, p); - } - }, - ), - ), + ), + PopupMenuItem( + value: 'delete', + child: Row( + children: [ + const Icon(Icons.delete, color: Colors.red), + const SizedBox(width: 8), + Text(locService.t('profiles.deleteProfile'), style: const TextStyle(color: Colors.red)), + ], + ), + ), + ], + onSelected: (value) { + if (value == 'edit') { + Navigator.push( + context, + MaterialPageRoute( + builder: (_) => ProfileEditor(profile: p), + ), + ); + } else if (value == 'delete') { + _showDeleteProfileDialog(context, p); + } + }, + ), + ); + }, ), ], ); diff --git a/lib/state/profile_state.dart b/lib/state/profile_state.dart index 41493ab..117b659 100644 --- a/lib/state/profile_state.dart +++ b/lib/state/profile_state.dart @@ -6,9 +6,11 @@ import '../services/profile_service.dart'; class ProfileState extends ChangeNotifier { static const String _enabledPrefsKey = 'enabled_profiles'; + static const String _profileOrderPrefsKey = 'profile_order'; final List _profiles = []; final Set _enabled = {}; + List _customOrder = []; // List of profile IDs in user's preferred order // Callback for when a profile is deleted (used to clear stale sessions) void Function(NodeProfile)? _onProfileDeleted; @@ -18,10 +20,10 @@ class ProfileState extends ChangeNotifier { } // Getters - List get profiles => List.unmodifiable(_profiles); + List get profiles => List.unmodifiable(_getOrderedProfiles()); bool isEnabled(NodeProfile p) => _enabled.contains(p); List get enabledProfiles => - _profiles.where(isEnabled).toList(growable: false); + _getOrderedProfiles().where(isEnabled).toList(growable: false); // Initialize profiles from built-in and custom sources Future init({bool addDefaults = false}) async { @@ -34,7 +36,7 @@ class ProfileState extends ChangeNotifier { await ProfileService().save(_profiles); } - // Load enabled profile IDs from prefs + // Load enabled profile IDs and custom order from prefs final prefs = await SharedPreferences.getInstance(); final enabledIds = prefs.getStringList(_enabledPrefsKey); if (enabledIds != null && enabledIds.isNotEmpty) { @@ -44,6 +46,9 @@ class ProfileState extends ChangeNotifier { // By default, all are enabled _enabled.addAll(_profiles); } + + // Load custom order + _customOrder = prefs.getStringList(_profileOrderPrefsKey) ?? []; } void toggleProfile(NodeProfile p, bool e) { @@ -92,6 +97,45 @@ class ProfileState extends ChangeNotifier { notifyListeners(); } + // Reorder profiles (for drag-and-drop in settings) + void reorderProfiles(int oldIndex, int newIndex) { + final orderedProfiles = _getOrderedProfiles(); + if (oldIndex < newIndex) { + newIndex -= 1; + } + final item = orderedProfiles.removeAt(oldIndex); + orderedProfiles.insert(newIndex, item); + + // Update custom order with new sequence + _customOrder = orderedProfiles.map((p) => p.id).toList(); + _saveCustomOrder(); + notifyListeners(); + } + + // Get profiles in custom order, with unordered profiles at the end + List _getOrderedProfiles() { + if (_customOrder.isEmpty) { + return List.from(_profiles); + } + + final ordered = []; + final profilesById = {for (final p in _profiles) p.id: p}; + + // Add profiles in custom order + for (final id in _customOrder) { + final profile = profilesById[id]; + if (profile != null) { + ordered.add(profile); + profilesById.remove(id); + } + } + + // Add any remaining profiles that weren't in the custom order + ordered.addAll(profilesById.values); + + return ordered; + } + // Save enabled profile IDs to disk Future _saveEnabledProfiles() async { final prefs = await SharedPreferences.getInstance(); @@ -100,4 +144,10 @@ class ProfileState extends ChangeNotifier { _enabled.map((p) => p.id).toList(), ); } + + // Save custom order to disk + Future _saveCustomOrder() async { + final prefs = await SharedPreferences.getInstance(); + await prefs.setStringList(_profileOrderPrefsKey, _customOrder); + } } \ No newline at end of file diff --git a/pubspec.yaml b/pubspec.yaml index 4ad0990..03aa82d 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,7 +1,7 @@ name: deflockapp description: Map public surveillance infrastructure with OpenStreetMap publish_to: "none" -version: 2.8.0+49 # The thing after the + is the version code, incremented with each release +version: 2.8.1+50 # The thing after the + is the version code, incremented with each release environment: sdk: ">=3.10.3 <4.0.0" # Resolved dependency floor (Dart 3.10.3 = Flutter 3.38+) diff --git a/test/models/node_profile_test.dart b/test/models/node_profile_test.dart index 28b1fb8..ea66324 100644 --- a/test/models/node_profile_test.dart +++ b/test/models/node_profile_test.dart @@ -1,5 +1,8 @@ import 'package:flutter_test/flutter_test.dart'; +import 'package:latlong2/latlong.dart'; import 'package:deflockapp/models/node_profile.dart'; +import 'package:deflockapp/models/osm_node.dart'; +import 'package:deflockapp/state/profile_state.dart'; void main() { group('NodeProfile', () { @@ -72,5 +75,180 @@ void main() { expect(a.hashCode, equals(b.hashCode)); expect(a, isNot(equals(c))); }); + + group('createExistingTagsProfile', () { + test('should NOT assign FOV for nodes with single direction', () { + // This is the core bug fix: nodes with just "direction=90" should not get a default FOV + final node = OsmNode( + id: 123, + coord: const LatLng(37.7749, -122.4194), + tags: { + 'direction': '90', + 'man_made': 'surveillance', + 'surveillance:type': 'ALPR', + }, + ); + + final profile = NodeProfile.createExistingTagsProfile(node); + + expect(profile.fov, isNull, reason: 'Single direction nodes should not get default FOV'); + expect(profile.name, equals('')); + expect(profile.tags, isEmpty, reason: 'Existing tags profile should have empty tags'); + }); + + test('should assign FOV for nodes with range notation', () { + final node = OsmNode( + id: 123, + coord: const LatLng(37.7749, -122.4194), + tags: { + 'direction': '55-125', // Range notation = explicit FOV + 'man_made': 'surveillance', + 'surveillance:type': 'ALPR', + }, + ); + + final profile = NodeProfile.createExistingTagsProfile(node); + + expect(profile.fov, isNotNull, reason: 'Range notation should preserve FOV'); + expect(profile.fov, equals(70.0), reason: 'Range 55-125 should calculate to 70 degree FOV'); + }); + + test('should assign FOV for nodes with multiple consistent ranges', () { + final node = OsmNode( + id: 123, + coord: const LatLng(37.7749, -122.4194), + tags: { + 'direction': '55-125;235-305', // Two ranges with same FOV + 'man_made': 'surveillance', + 'surveillance:type': 'ALPR', + }, + ); + + final profile = NodeProfile.createExistingTagsProfile(node); + + expect(profile.fov, equals(70.0), reason: 'Multiple consistent ranges should preserve FOV'); + }); + + test('should NOT assign FOV for mixed single directions and ranges', () { + final node = OsmNode( + id: 123, + coord: const LatLng(37.7749, -122.4194), + tags: { + 'direction': '90;180-360', // Mix of single direction and range + 'man_made': 'surveillance', + 'surveillance:type': 'ALPR', + }, + ); + + final profile = NodeProfile.createExistingTagsProfile(node); + + expect(profile.fov, isNull, reason: 'Mixed notation should not assign FOV'); + }); + + test('should NOT assign FOV for multiple single directions', () { + final node = OsmNode( + id: 123, + coord: const LatLng(37.7749, -122.4194), + tags: { + 'direction': '90;180;270', // Multiple single directions + 'man_made': 'surveillance', + 'surveillance:type': 'ALPR', + }, + ); + + final profile = NodeProfile.createExistingTagsProfile(node); + + expect(profile.fov, isNull, reason: 'Multiple single directions should not get default FOV'); + }); + + test('should handle camera:direction tag', () { + final node = OsmNode( + id: 123, + coord: const LatLng(37.7749, -122.4194), + tags: { + 'camera:direction': '180', // Using camera:direction instead of direction + 'man_made': 'surveillance', + 'surveillance:type': 'camera', + }, + ); + + final profile = NodeProfile.createExistingTagsProfile(node); + + expect(profile.fov, isNull, reason: 'Single camera:direction should not get default FOV'); + }); + + test('should fix the specific bug: direction=90 should not become direction=55-125', () { + // This tests the exact bug scenario mentioned in the issue + final node = OsmNode( + id: 123, + coord: const LatLng(37.7749, -122.4194), + tags: { + 'direction': '90', // Single direction, should stay as single direction + 'man_made': 'surveillance', + 'surveillance:type': 'ALPR', + }, + ); + + final profile = NodeProfile.createExistingTagsProfile(node); + + // Key fix: profile should NOT have an FOV, so upload won't convert to range notation + expect(profile.fov, isNull, reason: 'direction=90 should not get converted to direction=55-125'); + + // Verify the node does have directionFovPairs (for rendering), but profile ignores them + expect(node.directionFovPairs, hasLength(1)); + expect(node.directionFovPairs.first.centerDegrees, equals(90.0)); + expect(node.directionFovPairs.first.fovDegrees, equals(70.0)); // Default FOV for rendering + }); + }); + + group('ProfileState reordering', () { + test('should reorder profiles correctly', () { + final profileState = ProfileState(); + + // Add some test profiles + final profileA = NodeProfile(id: 'a', name: 'Profile A', tags: const {}); + final profileB = NodeProfile(id: 'b', name: 'Profile B', tags: const {}); + final profileC = NodeProfile(id: 'c', name: 'Profile C', tags: const {}); + + profileState.addOrUpdateProfile(profileA); + profileState.addOrUpdateProfile(profileB); + profileState.addOrUpdateProfile(profileC); + + // Initial order should be A, B, C + expect(profileState.profiles.map((p) => p.id), equals(['a', 'b', 'c'])); + + // Move profile at index 0 (A) to index 2 (should become B, C, A) + profileState.reorderProfiles(0, 2); + expect(profileState.profiles.map((p) => p.id), equals(['b', 'c', 'a'])); + + // Move profile at index 2 (A) to index 1 (should become B, A, C) + profileState.reorderProfiles(2, 1); + expect(profileState.profiles.map((p) => p.id), equals(['b', 'a', 'c'])); + }); + + test('should maintain enabled status after reordering', () { + final profileState = ProfileState(); + + final profileA = NodeProfile(id: 'a', name: 'Profile A', tags: const {}); + final profileB = NodeProfile(id: 'b', name: 'Profile B', tags: const {}); + final profileC = NodeProfile(id: 'c', name: 'Profile C', tags: const {}); + + profileState.addOrUpdateProfile(profileA); + profileState.addOrUpdateProfile(profileB); + profileState.addOrUpdateProfile(profileC); + + // Disable profile B + profileState.toggleProfile(profileB, false); + expect(profileState.isEnabled(profileB), isFalse); + + // Reorder profiles + profileState.reorderProfiles(0, 2); + + // Profile B should still be disabled after reordering + expect(profileState.isEnabled(profileB), isFalse); + expect(profileState.isEnabled(profileA), isTrue); + expect(profileState.isEnabled(profileC), isTrue); + }); + }); }); }