SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.63k stars 3.19k forks source link

TextEditor: "No Preview" mode does not hide preview panel #7462

Closed bcoles closed 3 years ago

bcoles commented 3 years ago

Under normal circumstances, selecting "No Preview" will hide the preview panel, leaving a vast open space in which to edit text.

However, if prior to selecting "No Preview" the user had used the horizontal splitter to resize the preview panel, then the preview will vanish leaving in its wake a desolate unusable wasteland.

Worse, the vertical splitting no longer works and cannot be resized. If the user wants to liberate their text editing region once more, they must first select another preview mode (anything other than "No Preview") then resize it.

Note the giant vacant area eating valuable text editing space in "No Preview" mode:

image

(I had a quick look at fixing this by auto adjusting the width in set_web_view_visible but that did not work as expected.)

trflynn89 commented 3 years ago

This is because the resize is implemented by HorizontalSplitter::mousemove_event setting the fixed_width of the child Widget to the left of the splitter, which in this case is the GUI::TextEditor. When the preview widget is removed, the fixed_width is still in effect, so the text box doesn't grow to fill the available space.

The simplest fix is to clear the text box's fixed width in set_web_view_visible:

diff --git a/Userland/Applications/TextEditor/MainWidget.cpp b/Userland/Applications/TextEditor/MainWidget.cpp
index a0171ebcc..2867ad546 100644
--- a/Userland/Applications/TextEditor/MainWidget.cpp
+++ b/Userland/Applications/TextEditor/MainWidget.cpp
@@ -705,6 +705,8 @@ void MainWidget::set_web_view_visible(bool visible)
     ensure_web_view();
     auto& web_view_container = *find_descendant_of_type_named<GUI::Widget>("web_view_container");
     web_view_container.set_visible(visible);
+    if (!visible)
+        m_editor->set_fixed_width(-1);
 }

But I don't like that fix because then any splitter with a non-constant number of visible children must remember to do something similar.

We can have GUI::Splitter itself track the number of visible children, and when that number changes, just split the total space evenly amongst its children:

diff --git a/Userland/Libraries/LibGUI/Splitter.cpp b/Userland/Libraries/LibGUI/Splitter.cpp
index 764a701fe..6b93b6e7e 100644
--- a/Userland/Libraries/LibGUI/Splitter.cpp
+++ b/Userland/Libraries/LibGUI/Splitter.cpp
@@ -180,6 +180,26 @@ void Splitter::mousemove_event(MouseEvent& event)
     invalidate_layout();
 }

+void Splitter::custom_layout()
+{
+    size_t visible_children = 0;
+
+    for_each_child_widget([&](auto& child_widget) {
+        if (child_widget.is_visible())
+            ++visible_children;
+        return IterationDecision::Continue;
+    });
+
+    if (visible_children != m_visible_children) {
+        for_each_child_widget([&](auto& child_widget) {
+            child_widget.set_fixed_width(-1);
+            return IterationDecision::Continue;
+        });
+
+        m_visible_children = visible_children;
+    }
+}
+
 void Splitter::did_layout()
 {
     if (m_first_resizee && m_second_resizee)
diff --git a/Userland/Libraries/LibGUI/Splitter.h b/Userland/Libraries/LibGUI/Splitter.h
index 66283df6e..97854669a 100644
--- a/Userland/Libraries/LibGUI/Splitter.h
+++ b/Userland/Libraries/LibGUI/Splitter.h
@@ -31,6 +31,7 @@ protected:
     virtual void mouseup_event(MouseEvent&) override;
     virtual void leave_event(Core::Event&) override;

+    virtual void custom_layout() override;
     virtual void did_layout() override;

 private:
@@ -49,6 +50,7 @@ private:
     int m_first_resizee_minimum_size { 0 };
     int m_second_resizee_minimum_size { 0 };
     Gfx::IntRect m_grabbable_rect;
+    size_t m_visible_children { 0 };
 };

 class VerticalSplitter final : public Splitter {

I don't love this solution either though, it feels kinda hacky...so I haven't opened a PR. Open to any other suggestions :)