From 201f11e65638365e7ef996bf43386581ff4bd075 Mon Sep 17 00:00:00 2001
From: Igor Kushnir <igorkuo@gmail.com>
Date: Tue, 24 May 2022 14:47:15 +0300
Subject: [PATCH] Set current article at most once in loadFinished()

When the current article is set and the user expands or collapses
optional parts (e.g. via the Ctrl+* shortcut),
ArticleView::setCurrentArticle() is called twice from
ArticleView::loadFinished(). Furthermore, the window scroll position is
restored before the second jump. This is wasteful. Move the
higher-priority setCurrentArticle() call up and, if it succeeds, skip
the other call and the scrolling.

I have measured the time spent running the affected code fragment on my
GNU/Linux system before and at this commit. When the loaded articles are
not very large, the performance gain of this commit is only about 1 ms.
However, when one of the displayed articles was huge (the
"United States" English Wikipedia article), the time went from 120 ms to
5 ms.
---
 articleview.cc | 84 ++++++++++++++++++++++++++------------------------
 articleview.hh |  3 +-
 2 files changed, 46 insertions(+), 41 deletions(-)

diff --git a/articleview.cc b/articleview.cc
index 8de8efff2..781e700e4 100644
--- a/articleview.cc
+++ b/articleview.cc
@@ -544,60 +544,63 @@ void ArticleView::loadFinished( bool )
     qApp->sendEvent( ui.definition, &ev );
   }
 
-  QVariant userDataVariant = ui.definition->history()->currentItem().userData();
+  // Expand collapsed article if only one loaded
+  ui.definition->page()->mainFrame()->evaluateJavaScript( "gdCheckArticlesNumber();" );
 
-  if ( userDataVariant.type() == QVariant::Map )
+  bool jumpedToCurrentArticle = false;
+  // Jump to current article after page reloading
+  if( !articleToJump.isEmpty() )
   {
-    QMap< QString, QVariant > userData = userDataVariant.toMap();
+    jumpedToCurrentArticle = setCurrentArticle( articleToJump, true );
+    articleToJump.clear();
+  }
 
-    QString currentArticle = userData.value( "currentArticle" ).toString();
+  if( !jumpedToCurrentArticle )
+  {
+    QVariant userDataVariant = ui.definition->history()->currentItem().userData();
 
-    if ( currentArticle.size() )
+    if ( userDataVariant.type() == QVariant::Map )
     {
-      // There's an active article saved, so set it to be active.
-      setCurrentArticle( currentArticle );
-    }
+      QMap< QString, QVariant > userData = userDataVariant.toMap();
+
+      QString currentArticle = userData.value( "currentArticle" ).toString();
 
-    double sx = 0, sy = 0;
+      if ( currentArticle.size() )
+      {
+        // There's an active article saved, so set it to be active.
+        setCurrentArticle( currentArticle );
+      }
 
-    if ( userData.value( "sx" ).type() == QVariant::Double )
-      sx = userData.value( "sx" ).toDouble();
+      double sx = 0, sy = 0;
 
-    if ( userData.value( "sy" ).type() == QVariant::Double )
-      sy = userData.value( "sy" ).toDouble();
+      if ( userData.value( "sx" ).type() == QVariant::Double )
+        sx = userData.value( "sx" ).toDouble();
 
-    if ( sx != 0 || sy != 0 )
-    {
-      // Restore scroll position
-      ui.definition->page()->mainFrame()->evaluateJavaScript(
-          QString( "window.scroll( %1, %2 );" ).arg( sx ).arg( sy ) );
+      if ( userData.value( "sy" ).type() == QVariant::Double )
+        sy = userData.value( "sy" ).toDouble();
+
+      if ( sx != 0 || sy != 0 )
+      {
+        // Restore scroll position
+        ui.definition->page()->mainFrame()->evaluateJavaScript(
+            QString( "window.scroll( %1, %2 );" ).arg( sx ).arg( sy ) );
+      }
     }
-  }
-  else
-  {
-    QString const scrollTo = Qt4x5::Url::queryItemValue( url, "scrollto" );
-    if( isScrollTo( scrollTo ) )
+    else
     {
-      // There is no active article saved in history, but we have it as a parameter.
-      // setCurrentArticle will save it and scroll there.
-      setCurrentArticle( scrollTo, true );
+      QString const scrollTo = Qt4x5::Url::queryItemValue( url, "scrollto" );
+      if( isScrollTo( scrollTo ) )
+      {
+        // There is no active article saved in history, but we have it as a parameter.
+        // setCurrentArticle will save it and scroll there.
+        setCurrentArticle( scrollTo, true );
+      }
     }
   }
 
-
   ui.definition->unsetCursor();
   //QApplication::restoreOverrideCursor();
 
-  // Expand collapsed article if only one loaded
-  ui.definition->page()->mainFrame()->evaluateJavaScript( QString( "gdCheckArticlesNumber();" ) );
-
-  // Jump to current article after page reloading
-  if( !articleToJump.isEmpty() )
-  {
-    setCurrentArticle( articleToJump, true );
-    articleToJump.clear();
-  }
-
 #if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
   if( !Qt4x5::Url::queryItemValue( url, "gdanchor" ).isEmpty() )
   {
@@ -729,13 +732,13 @@ void ArticleView::jumpToDictionary( QString const & id, bool force )
   }
 }
 
-void ArticleView::setCurrentArticle( QString const & id, bool moveToIt )
+bool ArticleView::setCurrentArticle( QString const & id, bool moveToIt )
 {
   if ( !isScrollTo( id ) )
-    return; // Incorrect id
+    return false; // Incorrect id
 
   if ( !ui.definition->isVisible() )
-    return; // No action on background page, scrollIntoView there don't work
+    return false; // No action on background page, scrollIntoView there don't work
 
   QString const dictionaryId = dictionaryIdFromScrollTo( id );
   if ( getArticlesList().contains( dictionaryId ) )
@@ -751,6 +754,7 @@ void ArticleView::setCurrentArticle( QString const & id, bool moveToIt )
     ui.definition->page()->mainFrame()->evaluateJavaScript(
       QString( "gdMakeArticleActive( '%1' );" ).arg( dictionaryId ) );
   }
+  return true;
 }
 
 void ArticleView::selectCurrentArticle()
diff --git a/articleview.hh b/articleview.hh
index ebc9dbaf9..8512d821d 100644
--- a/articleview.hh
+++ b/articleview.hh
@@ -319,7 +319,8 @@ private:
 
   /// Sets the current article by executing a javascript code.
   /// If moveToIt is true, it moves the focus to it as well.
-  void setCurrentArticle( QString const &, bool moveToIt = false );
+  /// Returns true in case of success, false otherwise.
+  bool setCurrentArticle( QString const &, bool moveToIt = false );
 
   /// Checks if the given article in form of "gdfrom-xxx" is inside a "website"
   /// frame.