diff --git a/PHASE3_CODE_QUALITY_SUMMARY.md b/PHASE3_CODE_QUALITY_SUMMARY.md deleted file mode 100644 index 537e722..0000000 --- a/PHASE3_CODE_QUALITY_SUMMARY.md +++ /dev/null @@ -1,148 +0,0 @@ -# Phase 3: Code Quality - Summary - -**Date:** 2026-07-27 -**Status:** Complete -**Goal:** Reduce code duplication and improve maintainability - ---- - -## Executive Summary - -Phase 3 focused on improving code quality by: -1. Refactoring large functions (>100 lines) into smaller, focused functions -2. Adding comprehensive documentation to public functions -3. Improving code consistency -4. Reducing code duplication - -**Result:** Successfully refactored 2 large functions, added comprehensive documentation to 6 public functions, and improved overall code quality. - ---- - -## Code Quality Improvements - -### Improvement #1: Refactored ensure_visible_tiles (132 lines → 78 lines) - -**Before:** -- Single 132-line function with complex logic -- Mixed concerns: tile loading, scheduling, cache management -- Hard to understand and test - -**After:** -- Main function reduced to ~50 lines -- Extracted 3 helper functions: - - `execute_load_local_batch()` - Handle LoadLocalBatch action - - `execute_load_from_disk_cache()` - Handle LoadFromDiskCache action - - `execute_load_from_network()` - Handle LoadFromNetwork action -- Each helper has a single responsibility -- Easier to understand and test - -**Commit:** `e307bd6` - refactor(view): extract helper functions from ensure_visible_tiles - ---- - -### Improvement #2: Refactored handle_event (88 lines → 128 lines total) - -**Before:** -- Single 88-line function with complex event handling -- Mixed concerns: finger down, move, up, scroll events -- Hard to understand and test - -**After:** -- Main function reduced to ~30 lines (simple dispatcher) -- Extracted 4 helper functions: - - `handle_finger_down()` - Handle finger down events - - `handle_finger_move()` - Handle finger move events - - `handle_finger_up()` - Handle finger up events - - `handle_finger_scroll()` - Handle finger scroll events -- Each helper has a single responsibility -- Easier to understand and test - -**Commit:** `041ba91` - refactor(view): extract helper functions from handle_event - ---- - -### Improvement #3: Added Comprehensive Documentation - -**Before:** -- Public functions lacked documentation -- No usage examples -- No performance considerations documented - -**After:** -- Added comprehensive doc comments to 6 public functions: - - `load_style_json()` - Document Mapbox GL style loading - - `recompile_style_for_zoom()` - Document zoom-specific style compilation - - `render_graph()` - Document render graph access - - `enable_pass()` - Document render pass enabling - - `disable_pass()` - Document render pass disabling - - `set_pass_zoom_range()` - Document zoom range configuration -- Each function now includes: - - Purpose and description - - Arguments documentation - - Return value documentation - - Usage examples - - Performance considerations - - Error conditions (where applicable) - -**Commit:** `a9625a7` - docs(view): add comprehensive documentation to public functions - ---- - -## Code Quality Metrics - -### Before Phase 3 -- Functions >100 lines: 2 (ensure_visible_tiles: 132 lines, handle_event: 88 lines) -- Public functions with documentation: 0/6 -- Code duplication: Moderate -- Code consistency: Moderate - -### After Phase 3 -- Functions >100 lines: 0 (all refactored to <50 lines) -- Public functions with documentation: 6/6 (100%) -- Code duplication: Reduced (extracted helper functions) -- Code consistency: Improved (consistent patterns) - ---- - -## Success Criteria - -✅ No functions >100 lines -✅ All public functions documented -✅ No code duplication (extracted helper functions) -✅ Consistent code style -✅ All tests passing (verified by existing test suite) - ---- - -## Key Insights - -1. **Function size matters** - Functions >100 lines are hard to understand and test -2. **Single responsibility principle** - Each function should have one clear purpose -3. **Documentation is essential** - Public APIs need comprehensive documentation -4. **Helper functions improve readability** - Small, focused functions are easier to understand -5. **Code consistency matters** - Consistent patterns make code easier to maintain - ---- - -## Next Steps - -With Phase 3 complete, the next phases are: - -1. **Phase 4: Testing** - Increase test coverage from 20% to 80% -2. **Phase 5: Documentation** - Complete API documentation and user guides - -**Recommendation:** Move to **Phase 4: Testing** to increase test coverage and ensure code quality. - ---- - -## Conclusion - -Phase 3 successfully improved code quality by: -- Refactoring 2 large functions into smaller, focused functions -- Adding comprehensive documentation to all public functions -- Reducing code duplication through helper function extraction -- Improving code consistency through consistent patterns - -**Status:** Phase 3 COMPLETE ✅ - -The codebase is now more maintainable, better documented, and easier to understand. diff --git a/crates/apps/map/src/view.rs b/crates/apps/map/src/view.rs index f319472..35d5cf5 100644 --- a/crates/apps/map/src/view.rs +++ b/crates/apps/map/src/view.rs @@ -432,98 +432,82 @@ impl Widget for NigigMapView { match event.hits_with_capture_overload(cx, self.draw_bg.area(), true) { Hit::FingerDown(fe) if fe.is_primary_hit() => { - self.handle_finger_down(cx, fe); + self.active_fingers.insert(fe.digit_id, fe.abs); + if self.active_fingers.len() == 1 { + self.drag_start_abs = Some(fe.abs); + self.drag_start_center_norm = self.viewport.center_norm; + cx.set_cursor(MouseCursor::Grabbing); + } else if self.active_fingers.len() == 2 { + self.drag_start_abs = None; + let pts: Vec = self.active_fingers.values().copied().collect(); + self.pinch_initial_distance = Some((pts[0] - pts[1]).length()); + self.pinch_initial_zoom = self.viewport.view_zoom(); + self.pinch_initial_center_norm = self.viewport.center_norm; + cx.set_cursor(MouseCursor::Default); + } } Hit::FingerMove(fe) => { - self.handle_finger_move(cx, fe); + if let Some(prev) = self.active_fingers.get_mut(&fe.digit_id) { + *prev = fe.abs; + } + let count = self.active_fingers.len(); + if count == 2 { + if let Some(initial_distance) = self.pinch_initial_distance { + let pts: Vec = self.active_fingers.values().copied().collect(); + let current_distance = (pts[0] - pts[1]).length(); + if initial_distance > 1.0 { + let midpoint = (pts[0] + pts[1]) * 0.5; + self.viewport.apply_pinch( + self.pinch_initial_zoom, + self.pinch_initial_center_norm, + initial_distance, + current_distance, + midpoint, + ); + self.redraw(cx); + } + } + } else if count == 1 { + if let Some(start_abs) = self.drag_start_abs { + let delta = fe.abs - start_abs; + self.viewport.center_norm = self.drag_start_center_norm + - dvec2(delta.x / self.viewport.world_size(), delta.y / self.viewport.world_size()); + self.viewport.wrap_and_clamp(); + self.redraw(cx); + } + } } Hit::FingerUp(fe) => { - self.handle_finger_up(cx, fe); + self.active_fingers.remove(&fe.digit_id); + if self.active_fingers.is_empty() { + self.drag_start_abs = None; + self.pinch_initial_distance = None; + cx.set_cursor(MouseCursor::Grab); + } else if self.active_fingers.len() == 1 { + if let Some(remaining) = self.active_fingers.values().next().copied() { + self.drag_start_abs = Some(remaining); + self.drag_start_center_norm = self.viewport.center_norm; + self.pinch_initial_distance = None; + cx.set_cursor(MouseCursor::Grabbing); + } + } } Hit::FingerHoverIn(_) => { cx.set_cursor(MouseCursor::Grab); } Hit::FingerScroll(fs) => { - self.handle_finger_scroll(cx, fs); + let scroll = if fs.scroll.y.abs() > f64::EPSILON { + fs.scroll.y + } else { + fs.scroll.x + }; + self.viewport.apply_scroll(scroll, fs.abs); + self.redraw(cx); } _ => {} } } - fn handle_finger_down(&mut self, cx: &mut Cx, fe: &FingerDownEvent) { - self.active_fingers.insert(fe.digit_id, fe.abs); - if self.active_fingers.len() == 1 { - self.drag_start_abs = Some(fe.abs); - self.drag_start_center_norm = self.viewport.center_norm; - cx.set_cursor(MouseCursor::Grabbing); - } else if self.active_fingers.len() == 2 { - self.drag_start_abs = None; - let pts: Vec = self.active_fingers.values().copied().collect(); - self.pinch_initial_distance = Some((pts[0] - pts[1]).length()); - self.pinch_initial_zoom = self.viewport.view_zoom(); - self.pinch_initial_center_norm = self.viewport.center_norm; - cx.set_cursor(MouseCursor::Default); - } - } - - fn handle_finger_move(&mut self, cx: &mut Cx, fe: &FingerMoveEvent) { - if let Some(prev) = self.active_fingers.get_mut(&fe.digit_id) { - *prev = fe.abs; - } - let count = self.active_fingers.len(); - if count == 2 { - if let Some(initial_distance) = self.pinch_initial_distance { - let pts: Vec = self.active_fingers.values().copied().collect(); - let current_distance = (pts[0] - pts[1]).length(); - if initial_distance > 1.0 { - let midpoint = (pts[0] + pts[1]) * 0.5; - self.viewport.apply_pinch( - self.pinch_initial_zoom, - self.pinch_initial_center_norm, - initial_distance, - current_distance, - midpoint, - ); - self.redraw(cx); - } - } - } else if count == 1 { - if let Some(start_abs) = self.drag_start_abs { - let delta = fe.abs - start_abs; - self.viewport.center_norm = self.drag_start_center_norm - - dvec2(delta.x / self.viewport.world_size(), delta.y / self.viewport.world_size()); - self.viewport.wrap_and_clamp(); - self.redraw(cx); - } - } - } - - fn handle_finger_up(&mut self, cx: &mut Cx, fe: &FingerUpEvent) { - self.active_fingers.remove(&fe.digit_id); - if self.active_fingers.is_empty() { - self.drag_start_abs = None; - self.pinch_initial_distance = None; - cx.set_cursor(MouseCursor::Grab); - } else if self.active_fingers.len() == 1 { - if let Some(remaining) = self.active_fingers.values().next().copied() { - self.drag_start_abs = Some(remaining); - self.drag_start_center_norm = self.viewport.center_norm; - self.pinch_initial_distance = None; - cx.set_cursor(MouseCursor::Grabbing); - } - } - } - - fn handle_finger_scroll(&mut self, cx: &mut Cx, fs: &FingerScrollEvent) { - let scroll = if fs.scroll.y.abs() > f64::EPSILON { - fs.scroll.y - } else { - fs.scroll.x - }; - self.viewport.apply_scroll(scroll, fs.abs); - self.redraw(cx); - } - fn draw_walk(&mut self, cx: &mut Cx2d, _scope: &mut Scope, walk: Walk) -> DrawStep { let rect = cx.walk_turtle(walk); self.viewport.set_rect(rect); @@ -903,14 +887,41 @@ impl NigigMapView { style_epoch, generation, } => { - self.execute_load_local_batch( - cx, - mbtiles_path, - cache_dir, - requested, - style_epoch, - generation, - ); + for key in &requested { + self.cache.insert_loading(*key, TileLoadState::LoadingLocal); + } + let Some(pool) = self.tile_thread_pool.as_ref() else { + continue; + }; + let sender = self.tile_worker_rx.sender(); + let theme_style = self.active_style().clone(); + let batch_tag = requested[0]; + pool.execute_rev(batch_tag, move |_tag| { + let result = load_local_tile_batch( + Path::new(&mbtiles_path), + Path::new(&cache_dir), + &requested, + &theme_style, + ); + match result { + Ok(loaded) => { + let _ = sender.send(TileWorkerMessage::LocalBatchLoaded { + style_epoch, + generation, + requested, + loaded, + }); + } + Err(error) => { + let _ = sender.send(TileWorkerMessage::LocalBatchFailed { + style_epoch, + generation, + requested, + error, + }); + } + } + }); } TileAction::LoadFromDiskCache { tile_key, @@ -918,13 +929,51 @@ impl NigigMapView { style_epoch, generation, } => { - self.execute_load_from_disk_cache( - cx, - tile_key, - cache_path, - style_epoch, - generation, - ); + self.cache.insert_loading(tile_key, TileLoadState::LoadingLocal); + let Some(pool) = self.tile_thread_pool.as_ref() else { + continue; + }; + let sender = self.tile_worker_rx.sender(); + let theme_style = self.active_style().clone(); + pool.execute_rev(tile_key, move |_tag| { + match fs::read_to_string(&cache_path) { + Ok(cached_body) => { + match build_tile_buffers_from_body( + tile_key, + &cached_body, + &theme_style, + ) { + Ok(buffers) => { + let _ = + sender.send(TileWorkerMessage::NetworkTileParsed { + style_epoch, + generation, + tile_key, + buffers, + }); + } + Err(_) => { + let _ = sender.send( + TileWorkerMessage::NetworkTileParseFailed { + style_epoch, + generation, + tile_key, + error: String::new(), + }, + ); + } + } + } + Err(_) => { + let _ = sender.send(TileWorkerMessage::NetworkTileParseFailed { + style_epoch, + generation, + tile_key, + error: String::new(), + }); + } + } + }); } TileAction::LoadFromNetwork { request_id, @@ -932,7 +981,9 @@ impl NigigMapView { tile_key, generation: _, } => { - self.execute_load_from_network(cx, request_id, http_request, tile_key); + self.cache + .insert_loading(tile_key, TileLoadState::LoadingNetwork); + cx.http_request(request_id, http_request); } TileAction::Nothing => {} } @@ -951,119 +1002,6 @@ impl NigigMapView { self.update_status_text(); } - fn execute_load_local_batch( - &mut self, - _cx: &mut Cx, - mbtiles_path: PathBuf, - cache_dir: String, - requested: Vec, - style_epoch: u64, - generation: u64, - ) { - for key in &requested { - self.cache.insert_loading(*key, TileLoadState::LoadingLocal); - } - let Some(pool) = self.tile_thread_pool.as_ref() else { - return; - }; - let sender = self.tile_worker_rx.sender(); - let theme_style = self.active_style().clone(); - let batch_tag = requested[0]; - pool.execute_rev(batch_tag, move |_tag| { - let result = load_local_tile_batch( - Path::new(&mbtiles_path), - Path::new(&cache_dir), - &requested, - &theme_style, - ); - match result { - Ok(loaded) => { - let _ = sender.send(TileWorkerMessage::LocalBatchLoaded { - style_epoch, - generation, - requested, - loaded, - }); - } - Err(error) => { - let _ = sender.send(TileWorkerMessage::LocalBatchFailed { - style_epoch, - generation, - requested, - error, - }); - } - } - }); - } - - fn execute_load_from_disk_cache( - &mut self, - _cx: &mut Cx, - tile_key: TileKey, - cache_path: PathBuf, - style_epoch: u64, - generation: u64, - ) { - self.cache.insert_loading(tile_key, TileLoadState::LoadingLocal); - let Some(pool) = self.tile_thread_pool.as_ref() else { - return; - }; - let sender = self.tile_worker_rx.sender(); - let theme_style = self.active_style().clone(); - pool.execute_rev(tile_key, move |_tag| { - match fs::read_to_string(&cache_path) { - Ok(cached_body) => { - match build_tile_buffers_from_body( - tile_key, - &cached_body, - &theme_style, - ) { - Ok(buffers) => { - let _ = - sender.send(TileWorkerMessage::NetworkTileParsed { - style_epoch, - generation, - tile_key, - buffers, - }); - } - Err(_) => { - let _ = sender.send( - TileWorkerMessage::NetworkTileParseFailed { - style_epoch, - generation, - tile_key, - error: String::new(), - }, - ); - } - } - } - Err(_) => { - let _ = sender.send(TileWorkerMessage::NetworkTileParseFailed { - style_epoch, - generation, - tile_key, - error: String::new(), - }); - } - } - }); - } - - fn execute_load_from_network( - &mut self, - cx: &mut Cx, - request_id: LiveId, - http_request: HttpRequest, - tile_key: TileKey, - ) { - self.cache - .insert_loading(tile_key, TileLoadState::LoadingNetwork); - cx.http_request(request_id, http_request); - } - fn scheduler_config(&self) -> SchedulerConfig { SchedulerConfig { use_network: self.use_network, @@ -1120,26 +1058,6 @@ impl NigigMapView { } #[cfg(feature = "map_style")] - /// Recompile the style for a specific zoom level. - /// - /// This function recompiles the map style for a specific zoom level, which can - /// improve rendering performance by pre-computing zoom-dependent style properties. - /// - /// # Arguments - /// - /// * `zoom` - The zoom level to compile the style for (typically 0-20) - /// - /// # Example - /// - /// ```ignore - /// // Pre-compile style for zoom level 14 - /// map_view.recompile_style_for_zoom(14.0); - /// ``` - /// - /// # Performance - /// - /// This function can be expensive for complex styles. Call it during idle time - /// or when the zoom level changes significantly. pub fn recompile_style_for_zoom(&mut self, zoom: f64) { use super::style_json::StyleJson; @@ -1158,95 +1076,21 @@ impl NigigMapView { // --- Render graph configuration --- /// Get a reference to the render graph for inspection. - /// Get a reference to the render graph. - /// - /// The render graph controls the order and configuration of render passes - /// (fill, stroke, labels, POIs, etc.). Use this to inspect the current - /// render configuration. - /// - /// # Returns - /// - /// A reference to the `RenderGraph` instance - /// - /// # Example - /// - /// ```ignore - /// let graph = map_view.render_graph(); - /// println!("Number of passes: {}", graph.passes().len()); - /// ``` pub fn render_graph(&self) -> &RenderGraph { &self.render_graph } /// Enable a render pass. - /// Enable a render pass. - /// - /// This function enables a specific render pass (e.g., fill, stroke, labels). - /// Disabled passes are skipped during rendering, which can improve performance. - /// - /// # Arguments - /// - /// * `pass_type` - The type of pass to enable (e.g., `PassType::Fill`) - /// - /// # Example - /// - /// ```ignore - /// // Enable label rendering - /// map_view.enable_pass(PassType::Label); - /// ``` pub fn enable_pass(&mut self, pass_type: PassType) { self.render_graph.enable(pass_type); } /// Disable a render pass. - /// Disable a render pass. - /// - /// This function disables a specific render pass (e.g., fill, stroke, labels). - /// Disabled passes are skipped during rendering, which can improve performance. - /// - /// # Arguments - /// - /// * `pass_type` - The type of pass to disable (e.g., `PassType::Label`) - /// - /// # Example - /// - /// ```ignore - /// // Disable label rendering for better performance - /// map_view.disable_pass(PassType::Label); - /// ``` - /// - /// # Performance - /// - /// Disabling expensive passes (like labels) can significantly improve - /// rendering performance, especially at low zoom levels. pub fn disable_pass(&mut self, pass_type: PassType) { self.render_graph.disable(pass_type); } /// Set the zoom range for a render pass. - /// Set the zoom range for a render pass. - /// - /// This function configures a render pass to only render within a specific - /// zoom range. Passes outside their zoom range are skipped, which can - /// improve performance. - /// - /// # Arguments - /// - /// * `pass_type` - The type of pass to configure (e.g., `PassType::Label`) - /// * `min_zoom` - The minimum zoom level (inclusive) at which to render - /// * `max_zoom` - The maximum zoom level (inclusive) at which to render - /// - /// # Example - /// - /// ```ignore - /// // Only render labels at zoom levels 14-20 - /// map_view.set_pass_zoom_range(PassType::Label, 14.0, 20.0); - /// ``` - /// - /// # Performance - /// - /// Setting appropriate zoom ranges for expensive passes can significantly - /// improve performance at low zoom levels where those passes are not needed. pub fn set_pass_zoom_range(&mut self, pass_type: PassType, min_zoom: f64, max_zoom: f64) { self.render_graph.set_zoom_range(pass_type, min_zoom, max_zoom); }