From 3307f44ce20a71273614f9b30dafb08822e557a1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bla=C5=BE=20Hrastnik?= <blaz@mxxn.io>
Date: Fri, 10 Dec 2021 19:23:34 +0900
Subject: [PATCH] ui: popup: Don't allow scrolling past the end of content

---
 helix-term/src/compositor.rs  |  5 +++--
 helix-term/src/ui/markdown.rs |  5 -----
 helix-term/src/ui/popup.rs    | 19 ++++++++++++++++---
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/helix-term/src/compositor.rs b/helix-term/src/compositor.rs
index 37e679737..30554ebb3 100644
--- a/helix-term/src/compositor.rs
+++ b/helix-term/src/compositor.rs
@@ -55,9 +55,10 @@ pub trait Component: Any + AnyComponent {
 
     /// May be used by the parent component to compute the child area.
     /// viewport is the maximum allowed area, and the child should stay within those bounds.
+    ///
+    /// The returned size might be larger than the viewport if the child is too big to fit.
+    /// In this case the parent can use the values to calculate scroll.
     fn required_size(&mut self, _viewport: (u16, u16)) -> Option<(u16, u16)> {
-        // TODO: for scrolling, the scroll wrapper should place a size + offset on the Context
-        // that way render can use it
         None
     }
 
diff --git a/helix-term/src/ui/markdown.rs b/helix-term/src/ui/markdown.rs
index ca8303dd9..46657fb9a 100644
--- a/helix-term/src/ui/markdown.rs
+++ b/helix-term/src/ui/markdown.rs
@@ -241,11 +241,6 @@ impl Component for Markdown {
             } else if content_width > text_width {
                 text_width = content_width;
             }
-
-            if height >= viewport.1 {
-                height = viewport.1;
-                break;
-            }
         }
 
         Some((text_width + padding, height))
diff --git a/helix-term/src/ui/popup.rs b/helix-term/src/ui/popup.rs
index a5310b231..c55f030f9 100644
--- a/helix-term/src/ui/popup.rs
+++ b/helix-term/src/ui/popup.rs
@@ -15,6 +15,7 @@ pub struct Popup<T: Component> {
     contents: T,
     position: Option<Position>,
     size: (u16, u16),
+    child_size: (u16, u16),
     scroll: usize,
     id: &'static str,
 }
@@ -25,6 +26,7 @@ impl<T: Component> Popup<T> {
             contents,
             position: None,
             size: (0, 0),
+            child_size: (0, 0),
             scroll: 0,
             id,
         }
@@ -70,6 +72,9 @@ impl<T: Component> Popup<T> {
     pub fn scroll(&mut self, offset: usize, direction: bool) {
         if direction {
             self.scroll += offset;
+
+            let max_offset = self.child_size.1.saturating_sub(self.size.1);
+            self.scroll = (self.scroll + offset).min(max_offset as usize);
         } else {
             self.scroll = self.scroll.saturating_sub(offset);
         }
@@ -117,13 +122,21 @@ impl<T: Component> Component for Popup<T> {
         // tab/enter/ctrl-k or whatever will confirm the selection/ ctrl-n/ctrl-p for scroll.
     }
 
-    fn required_size(&mut self, _viewport: (u16, u16)) -> Option<(u16, u16)> {
+    fn required_size(&mut self, viewport: (u16, u16)) -> Option<(u16, u16)> {
+        let max_width = 120.min(viewport.0);
+        let max_height = 26.min(viewport.1.saturating_sub(2)); // add some spacing in the viewport
+
         let (width, height) = self
             .contents
-            .required_size((120, 26)) // max width, max height
+            .required_size((max_width, max_height))
             .expect("Component needs required_size implemented in order to be embedded in a popup");
 
-        self.size = (width, height);
+        self.child_size = (width, height);
+        self.size = (width.min(max_width), height.min(max_height));
+
+        // re-clamp scroll offset
+        let max_offset = self.child_size.1.saturating_sub(self.size.1);
+        self.scroll = self.scroll.min(max_offset as usize);
 
         Some(self.size)
     }