From cda88b3c3b8ea1d05bd3e8436e9083b339ea474c Mon Sep 17 00:00:00 2001 From: Ruben Dashyan Date: Thu, 31 Jan 2019 15:20:05 +0100 Subject: [PATCH] [escher/scroll_view] Fix virtuality issues of layoutSubviews Method contentSize() made virtual and overridden by ScrollableView so that ScrollableView and TableView do not need to setSize themselves and that setSize/setFrame is not called twice over m_contentView. --- escher/include/escher/scroll_view.h | 2 +- escher/include/escher/scrollable_view.h | 2 +- escher/include/escher/table_view.h | 3 +-- escher/src/scroll_view.cpp | 8 +------- escher/src/scrollable_view.cpp | 11 +++++------ escher/src/table_view.cpp | 25 +++++++++++++------------ 6 files changed, 22 insertions(+), 29 deletions(-) diff --git a/escher/include/escher/scroll_view.h b/escher/include/escher/scroll_view.h index 5789d56f5..3a7ff7d92 100644 --- a/escher/include/escher/scroll_view.h +++ b/escher/include/escher/scroll_view.h @@ -92,7 +92,7 @@ protected: } KDRect visibleContentRect(); void layoutSubviews() override; - KDSize contentSize(); + virtual KDSize contentSize() const { return m_contentView->minimalSizeForOptimalDisplay(); } #if ESCHER_VIEW_LOGGING virtual const char * className() const override; virtual void logAttributes(std::ostream &os) const override; diff --git a/escher/include/escher/scrollable_view.h b/escher/include/escher/scrollable_view.h index 4ceeca709..ba4aaea4e 100644 --- a/escher/include/escher/scrollable_view.h +++ b/escher/include/escher/scrollable_view.h @@ -11,7 +11,7 @@ public: bool handleEvent(Ion::Events::Event event) override; void reloadScroll(bool forceRelayout = false); protected: - void layoutSubviews() override; + KDSize contentSize() const override; KDPoint m_manualScrollingOffset; }; diff --git a/escher/include/escher/table_view.h b/escher/include/escher/table_view.h index b67868cae..eb7010306 100644 --- a/escher/include/escher/table_view.h +++ b/escher/include/escher/table_view.h @@ -38,12 +38,12 @@ protected: void scrollToCell(int i, int j) const; void reloadCellAtLocation(int i, int j); HighlightCell * cellAtLocation(int i, int j); - void resizeToFitContent(); TableViewDataSource * dataSource(); int rowsScrollingOffset() const; int columnsScrollingOffset() const; int numberOfDisplayableRows() const; int numberOfDisplayableColumns() const; + void layoutSubviews() override; protected: #if ESCHER_VIEW_LOGGING const char * className() const override; @@ -54,7 +54,6 @@ protected: int numberOfSubviews() const override; View * subviewAtIndex(int index) override; - void layoutSubviews() override; /* realCellWidth enables to handle list view for which * TableViewDataSource->cellWidht = 0 */ diff --git a/escher/src/scroll_view.cpp b/escher/src/scroll_view.cpp index bd65b9f1c..52d2506b5 100644 --- a/escher/src/scroll_view.cpp +++ b/escher/src/scroll_view.cpp @@ -104,10 +104,8 @@ KDRect ScrollView::visibleContentRect() { } void ScrollView::layoutSubviews() { - // Layout contentView - // We're only re-positionning the contentView, not modifying its size. KDPoint absoluteOffset = contentOffset().opposite().translatedBy(KDPoint(m_leftMargin, m_topMargin)); - KDRect contentFrame = KDRect(absoluteOffset, m_contentView->bounds().size()); + KDRect contentFrame = KDRect(absoluteOffset, contentSize()); m_contentView->setFrame(contentFrame); KDSize content( m_contentView->bounds().width() + m_leftMargin + m_rightMargin, @@ -116,10 +114,6 @@ void ScrollView::layoutSubviews() { decorator()->layoutIndicators(content, contentOffset(), m_frame.size()); } -KDSize ScrollView::contentSize() { - return m_contentView->minimalSizeForOptimalDisplay(); -} - void ScrollView::setContentOffset(KDPoint offset, bool forceRelayout) { if (m_dataSource->setOffset(offset) || forceRelayout) { layoutSubviews(); diff --git a/escher/src/scrollable_view.cpp b/escher/src/scrollable_view.cpp index a8a30b9d2..ea122e2b0 100644 --- a/escher/src/scrollable_view.cpp +++ b/escher/src/scrollable_view.cpp @@ -50,10 +50,9 @@ void ScrollableView::reloadScroll(bool forceReLayout) { setContentOffset(m_manualScrollingOffset, forceReLayout); } -void ScrollableView::layoutSubviews() { - KDSize viewSize = contentSize(); - KDCoordinate viewWidth = max(viewSize.width(), bounds().width() - leftMargin() - rightMargin()); - KDCoordinate viewHeight = max(viewSize.height(), bounds().height() - topMargin() - bottomMargin()); - m_contentView->setSize(KDSize(viewWidth, viewHeight)); - ScrollView::layoutSubviews(); +KDSize ScrollableView::contentSize() const { + KDSize viewSize = ScrollView::contentSize(); + KDCoordinate viewWidth = max(viewSize.width(), maxContentWidthDisplayableWithoutScrolling()); + KDCoordinate viewHeight = max(viewSize.height(), maxContentHeightDisplayableWithoutScrolling()); + return KDSize(viewWidth, viewHeight); } diff --git a/escher/src/table_view.cpp b/escher/src/table_view.cpp index 6aa1784cd..9a9d2ef28 100644 --- a/escher/src/table_view.cpp +++ b/escher/src/table_view.cpp @@ -38,12 +38,20 @@ const char * TableView::className() const { #endif void TableView::layoutSubviews() { - // We only have to layout our contentView. - // We will size it here, and ScrollView::layoutSubviews will position it. - - m_contentView.resizeToFitContent(); - ScrollView::layoutSubviews(); + m_contentView.layoutSubviews(); + /* FIXME: + * On the one hand, ScrollView::layoutSubviews() + * calls setFrame(...) over m_contentView, + * which typically calls layoutSubviews() over m_contentView. + * However, if the frame happens to be unchanged, + * setFrame(...) does not call layoutSubviews. + * On the other hand, calling only m_contentView.layoutSubviews() + * does not relayout ScrollView when the offset + * or the content's size changes. + * For those reasons, we call both of them explicitly. + * Finally, this solution is not optimal at all since + * layoutSubviews is called twice over m_contentView. */ } void TableView::reloadCellAtLocation(int i, int j) { @@ -75,13 +83,6 @@ KDCoordinate TableView::ContentView::columnWidth(int i) const { return columnWidth; } -void TableView::ContentView::resizeToFitContent() { - if (!(m_tableView->bounds() == KDRectZero)) { - layoutSubviews(); - setSize(KDSize(width(), height())); - } -} - KDCoordinate TableView::ContentView::height() const { return m_dataSource->cumulatedHeightFromIndex(m_dataSource->numberOfRows())+m_verticalCellOverlap; }