PHASE3_CODE_QUALITY_SUMMARY.md

@@ -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.

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<Vec2d> = 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<Vec2d> = 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<Vec2d> = 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<Vec2d> = 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(
+                    for key in &requested {
-                        cx,
+                        self.cache.insert_loading(*key, TileLoadState::LoadingLocal);
-                        mbtiles_path,
+                    }
-                        cache_dir,
+                    let Some(pool) = self.tile_thread_pool.as_ref() else {
-                        requested,
+                        continue;
-                        style_epoch,
+                    };
-                        generation,
+                    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(
+                    self.cache.insert_loading(tile_key, TileLoadState::LoadingLocal);
-                        cx,
+                    let Some(pool) = self.tile_thread_pool.as_ref() else {
-                        tile_key,
+                        continue;
-                        cache_path,
+                    };
-                        style_epoch,
+                    let sender = self.tile_worker_rx.sender();
-                        generation,
+                    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<TileKey>,
-        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);
     }