From fe865a174382a42a01ecaec215797b0992d11aa7 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 14:34:49 +0100
Subject: [PATCH 01/21] Rewrite wxDataViewCtrl unit test without using CppUnit
 macros

Use CATCH API directly, this simplifies the code and makes adding new
tests simpler.

No real changes yet.
---
 tests/controls/dataviewctrltest.cpp | 124 +++++++++++++---------------
 1 file changed, 57 insertions(+), 67 deletions(-)

diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp
index 45c10cf253..d8b20740ed 100644
--- a/tests/controls/dataviewctrltest.cpp
+++ b/tests/controls/dataviewctrltest.cpp
@@ -27,30 +27,13 @@
 // test class
 // ----------------------------------------------------------------------------
 
-class DataViewCtrlTestCase : public CppUnit::TestCase
+class DataViewCtrlTestCase
 {
 public:
-    DataViewCtrlTestCase() { }
-
-    virtual void setUp() wxOVERRIDE;
-    virtual void tearDown() wxOVERRIDE;
-
-private:
-    CPPUNIT_TEST_SUITE( DataViewCtrlTestCase );
-        CPPUNIT_TEST( DeleteSelected );
-        CPPUNIT_TEST( DeleteNotSelected );
-        CPPUNIT_TEST( GetSelectionForMulti );
-        CPPUNIT_TEST( GetSelectionForSingle );
-    CPPUNIT_TEST_SUITE_END();
-
-    // Create wxDataViewTreeCtrl with the given style.
-    void Create(long style);
-
-    void DeleteSelected();
-    void DeleteNotSelected();
-    void GetSelectionForMulti();
-    void GetSelectionForSingle();
+    explicit DataViewCtrlTestCase(long style);
+    ~DataViewCtrlTestCase();
 
+protected:
     void TestSelectionFor0and1();
 
     // the dataview control itself
@@ -65,17 +48,29 @@ private:
     wxDECLARE_NO_COPY_CLASS(DataViewCtrlTestCase);
 };
 
-// register in the unnamed registry so that these tests are run by default
-CPPUNIT_TEST_SUITE_REGISTRATION( DataViewCtrlTestCase );
+class SingleSelectDataViewCtrlTestCase : public DataViewCtrlTestCase
+{
+public:
+    SingleSelectDataViewCtrlTestCase()
+        : DataViewCtrlTestCase(wxDV_SINGLE)
+    {
+    }
+};
 
-// also include in its own registry so that these tests can be run alone
-CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( DataViewCtrlTestCase, "DataViewCtrlTestCase" );
+class MultiSelectDataViewCtrlTestCase : public DataViewCtrlTestCase
+{
+public:
+    MultiSelectDataViewCtrlTestCase()
+        : DataViewCtrlTestCase(wxDV_MULTIPLE)
+    {
+    }
+};
 
 // ----------------------------------------------------------------------------
 // test initialization
 // ----------------------------------------------------------------------------
 
-void DataViewCtrlTestCase::Create(long style)
+DataViewCtrlTestCase::DataViewCtrlTestCase(long style)
 {
     m_dvc = new wxDataViewTreeCtrl(wxTheApp->GetTopWindow(),
                                    wxID_ANY,
@@ -94,27 +89,18 @@ void DataViewCtrlTestCase::Create(long style)
     m_dvc->Update();
 }
 
-void DataViewCtrlTestCase::setUp()
-{
-    Create(wxDV_MULTIPLE);
-}
-
-void DataViewCtrlTestCase::tearDown()
+DataViewCtrlTestCase::~DataViewCtrlTestCase()
 {
     delete m_dvc;
-    m_dvc = NULL;
-
-    m_root =
-    m_child1 =
-    m_child2 =
-    m_grandchild = wxDataViewItem();
 }
 
 // ----------------------------------------------------------------------------
 // the tests themselves
 // ----------------------------------------------------------------------------
 
-void DataViewCtrlTestCase::DeleteSelected()
+TEST_CASE_METHOD(MultiSelectDataViewCtrlTestCase,
+                 "wxDVC::DeleteSelected",
+                 "[wxDataViewCtrl][delete]")
 {
     wxDataViewItemArray sel;
     sel.push_back(m_child1);
@@ -128,14 +114,18 @@ void DataViewCtrlTestCase::DeleteSelected()
     m_dvc->GetSelections(sel);
 
     // m_child1 and its children should be removed from the selection now
-    CPPUNIT_ASSERT_EQUAL( 1, sel.size() );
-    CPPUNIT_ASSERT( sel[0] == m_child2 );
+    REQUIRE( sel.size() == 1 );
+    CHECK( sel[0] == m_child2 );
 }
 
-void DataViewCtrlTestCase::DeleteNotSelected()
+TEST_CASE_METHOD(MultiSelectDataViewCtrlTestCase,
+                 "wxDVC::DeleteNotSelected",
+                 "[wxDataViewCtrl][delete]")
 {
     // TODO not working on OS X as expected
-#ifndef __WXOSX__
+#ifdef __WXOSX__
+    WARN("Disabled under MacOS because this test currently fails");
+#else
     wxDataViewItemArray sel;
     sel.push_back(m_child1);
     sel.push_back(m_grandchild);
@@ -147,9 +137,9 @@ void DataViewCtrlTestCase::DeleteNotSelected()
     m_dvc->GetSelections(sel);
 
     // m_child1 and its children should be unaffected
-    CPPUNIT_ASSERT_EQUAL( 2, sel.size() );
-    CPPUNIT_ASSERT( sel[0] == m_child1 );
-    CPPUNIT_ASSERT( sel[1] == m_grandchild );
+    REQUIRE( sel.size() == 2 );
+    CHECK( sel[0] == m_child1 );
+    CHECK( sel[1] == m_grandchild );
 #endif
 }
 
@@ -158,43 +148,43 @@ void DataViewCtrlTestCase::TestSelectionFor0and1()
     wxDataViewItemArray selections;
 
     // Initially there is no selection.
-    CPPUNIT_ASSERT_EQUAL( 0, m_dvc->GetSelectedItemsCount() );
-    CPPUNIT_ASSERT( !m_dvc->HasSelection() );
-    CPPUNIT_ASSERT( !m_dvc->GetSelection().IsOk() );
+    CHECK( m_dvc->GetSelectedItemsCount() == 0 );
+    CHECK( !m_dvc->HasSelection() );
+    CHECK( !m_dvc->GetSelection().IsOk() );
 
-    CPPUNIT_ASSERT( !m_dvc->GetSelections(selections) );
-    CPPUNIT_ASSERT( selections.empty() );
+    CHECK( !m_dvc->GetSelections(selections) );
+    CHECK( selections.empty() );
 
     // Select one item.
     m_dvc->Select(m_child1);
-    CPPUNIT_ASSERT_EQUAL( 1, m_dvc->GetSelectedItemsCount() );
-    CPPUNIT_ASSERT( m_dvc->HasSelection() );
-    CPPUNIT_ASSERT( m_dvc->GetSelection().IsOk() );
-    CPPUNIT_ASSERT_EQUAL( 1, m_dvc->GetSelections(selections) );
-    CPPUNIT_ASSERT( selections[0] == m_child1 );
+    CHECK( m_dvc->GetSelectedItemsCount() == 1 );
+    CHECK( m_dvc->HasSelection() );
+    CHECK( m_dvc->GetSelection().IsOk() );
+    REQUIRE( m_dvc->GetSelections(selections) == 1 );
+    CHECK( selections[0] == m_child1 );
 }
 
-void DataViewCtrlTestCase::GetSelectionForMulti()
+TEST_CASE_METHOD(MultiSelectDataViewCtrlTestCase,
+                 "wxDVC::GetSelectionForMulti",
+                 "[wxDataViewCtrl][select]")
 {
     wxDataViewItemArray selections;
 
     TestSelectionFor0and1();
 
-    // Also test with more than one selected item.
     m_dvc->Select(m_child2);
 
-    CPPUNIT_ASSERT_EQUAL( 2, m_dvc->GetSelectedItemsCount() );
-    CPPUNIT_ASSERT( m_dvc->HasSelection() );
-    CPPUNIT_ASSERT( !m_dvc->GetSelection().IsOk() );
-    CPPUNIT_ASSERT_EQUAL( 2, m_dvc->GetSelections(selections) );
-    CPPUNIT_ASSERT( selections[1] == m_child2 );
+    CHECK( m_dvc->GetSelectedItemsCount() == 2 );
+    CHECK( m_dvc->HasSelection() );
+    CHECK( !m_dvc->GetSelection().IsOk() );
+    REQUIRE( m_dvc->GetSelections(selections) == 2 );
+    CHECK( selections[1] == m_child2 );
 }
 
-void DataViewCtrlTestCase::GetSelectionForSingle()
+TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
+                 "wxDVC::SingleSelection",
+                 "[wxDataViewCtrl][selection]")
 {
-    delete m_dvc;
-    Create(0);
-
     TestSelectionFor0and1();
 }
 

From 05ae63e40a0bae7fa34e3be8edf0d3ed5092584a Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 14:54:53 +0100
Subject: [PATCH 02/21] Use standard naming convention for RowToTreeNodeJob
 members

Prefix them with "m_" to indicate that they're member variables.

No real changes.
---
 src/generic/datavgen.cpp | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index 2e1d1887e1..1ad0120fb3 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3468,31 +3468,28 @@ int wxDataViewMainWindow::GetLineHeight( unsigned int row ) const
 class RowToTreeNodeJob: public DoJob
 {
 public:
-    RowToTreeNodeJob( unsigned int row_ , int current_, wxDataViewTreeNode * node )
+    RowToTreeNodeJob(unsigned int row, int current, wxDataViewTreeNode *parent)
+        : m_row(row), m_current(current), m_parent(parent), m_ret(NULL)
     {
-        this->row = row_;
-        this->current = current_;
-        ret = NULL;
-        parent = node;
     }
 
     virtual int operator() ( wxDataViewTreeNode * node ) wxOVERRIDE
     {
-        current ++;
-        if( current == static_cast<int>(row))
+        m_current ++;
+        if( m_current == static_cast<int>(m_row))
         {
-            ret = node;
+            m_ret = node;
             return DoJob::DONE;
         }
 
-        if( node->GetSubTreeCount() + current < static_cast<int>(row) )
+        if( node->GetSubTreeCount() + m_current < static_cast<int>(m_row) )
         {
-            current += node->GetSubTreeCount();
+            m_current += node->GetSubTreeCount();
             return  DoJob::SKIP_SUBTREE;
         }
         else
         {
-            parent = node;
+            m_parent = node;
 
             // If the current node has only leaf children, we can find the
             // desired node directly. This can speed up finding the node
@@ -3500,8 +3497,8 @@ public:
             if ( node->HasChildren() &&
                  (int)node->GetChildNodes().size() == node->GetSubTreeCount() )
             {
-                const int index = static_cast<int>(row) - current - 1;
-                ret = node->GetChildNodes()[index];
+                const int index = static_cast<int>(m_row) - m_current - 1;
+                m_ret = node->GetChildNodes()[index];
                 return DoJob::DONE;
             }
 
@@ -3510,13 +3507,13 @@ public:
     }
 
     wxDataViewTreeNode * GetResult() const
-        { return ret; }
+        { return m_ret; }
 
 private:
-    unsigned int row;
-    int current;
-    wxDataViewTreeNode * ret;
-    wxDataViewTreeNode * parent;
+    unsigned int m_row;
+    int m_current;
+    wxDataViewTreeNode* m_parent;
+    wxDataViewTreeNode* m_ret;
 };
 
 wxDataViewTreeNode * wxDataViewMainWindow::GetTreeNodeByRow(unsigned int row) const

From 3d4a47a6bbb1535c35319ac0f45c95b5a5057b59 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 14:56:36 +0100
Subject: [PATCH 03/21] Change RowToTreeNodeJob::m_row type to int and make it
 const

It doesn't make much sense to use an "unsigned int" variable only to
cast it to int everywhere where it's used. Just make it "int" from the
get go and have a single cast to int in the caller.

Also make m_row const as it never changes.
---
 src/generic/datavgen.cpp | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index 1ad0120fb3..5965dae3ec 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3468,7 +3468,7 @@ int wxDataViewMainWindow::GetLineHeight( unsigned int row ) const
 class RowToTreeNodeJob: public DoJob
 {
 public:
-    RowToTreeNodeJob(unsigned int row, int current, wxDataViewTreeNode *parent)
+    RowToTreeNodeJob(int row, int current, wxDataViewTreeNode *parent)
         : m_row(row), m_current(current), m_parent(parent), m_ret(NULL)
     {
     }
@@ -3476,13 +3476,13 @@ public:
     virtual int operator() ( wxDataViewTreeNode * node ) wxOVERRIDE
     {
         m_current ++;
-        if( m_current == static_cast<int>(m_row))
+        if( m_current == m_row)
         {
             m_ret = node;
             return DoJob::DONE;
         }
 
-        if( node->GetSubTreeCount() + m_current < static_cast<int>(m_row) )
+        if( node->GetSubTreeCount() + m_current < m_row )
         {
             m_current += node->GetSubTreeCount();
             return  DoJob::SKIP_SUBTREE;
@@ -3497,7 +3497,7 @@ public:
             if ( node->HasChildren() &&
                  (int)node->GetChildNodes().size() == node->GetSubTreeCount() )
             {
-                const int index = static_cast<int>(m_row) - m_current - 1;
+                const int index = m_row - m_current - 1;
                 m_ret = node->GetChildNodes()[index];
                 return DoJob::DONE;
             }
@@ -3510,7 +3510,7 @@ public:
         { return m_ret; }
 
 private:
-    unsigned int m_row;
+    const int m_row;
     int m_current;
     wxDataViewTreeNode* m_parent;
     wxDataViewTreeNode* m_ret;
@@ -3523,7 +3523,7 @@ wxDataViewTreeNode * wxDataViewMainWindow::GetTreeNodeByRow(unsigned int row) co
     if ( row == (unsigned)-1 )
         return NULL;
 
-    RowToTreeNodeJob job( row , -2, m_root );
+    RowToTreeNodeJob job( static_cast<int>(row) , -2, m_root );
     Walker( m_root , job );
     return job.GetResult();
 }

From 739ce6055213d271b38c11b75c99afe4bf462c9d Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 14:58:23 +0100
Subject: [PATCH 04/21] Remove the unused RowToTreeNodeJob::m_parent variable

This variable was assigned to but never used.

Also remove the corresponding ctor argument which was only used to
initialize this unused variable.
---
 src/generic/datavgen.cpp | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index 5965dae3ec..c83ce64821 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3468,8 +3468,8 @@ int wxDataViewMainWindow::GetLineHeight( unsigned int row ) const
 class RowToTreeNodeJob: public DoJob
 {
 public:
-    RowToTreeNodeJob(int row, int current, wxDataViewTreeNode *parent)
-        : m_row(row), m_current(current), m_parent(parent), m_ret(NULL)
+    RowToTreeNodeJob(int row, int current)
+        : m_row(row), m_current(current), m_ret(NULL)
     {
     }
 
@@ -3489,8 +3489,6 @@ public:
         }
         else
         {
-            m_parent = node;
-
             // If the current node has only leaf children, we can find the
             // desired node directly. This can speed up finding the node
             // in some cases, and will have a very good effect for list views.
@@ -3512,7 +3510,6 @@ public:
 private:
     const int m_row;
     int m_current;
-    wxDataViewTreeNode* m_parent;
     wxDataViewTreeNode* m_ret;
 };
 
@@ -3523,7 +3520,7 @@ wxDataViewTreeNode * wxDataViewMainWindow::GetTreeNodeByRow(unsigned int row) co
     if ( row == (unsigned)-1 )
         return NULL;
 
-    RowToTreeNodeJob job( static_cast<int>(row) , -2, m_root );
+    RowToTreeNodeJob job( static_cast<int>(row) , -2 );
     Walker( m_root , job );
     return job.GetResult();
 }

From 685f9ff57d87fdfdb3b9d84ed63f1ee37ad39d04 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:03:16 +0100
Subject: [PATCH 05/21] Fix bug in GetRowByItem() in generic wxDataViewCtrl

If the item was not found at all, which can happen if all its parents
are not expanded, this function still returned a valid but completely
wrong row index.

This affected many functions which could call it for the items which
were not necessarily visible, i.e. all of them except for the event
handlers as events can only affect the visible items, including but not
limited to SetCurrentItem(), all the selection-related functions, all
the expansion-related functions, EnsureVisible(), HitTest() and
GetItemRect().
---
 src/generic/datavgen.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index c83ce64821..42e55ebb2e 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3920,7 +3920,9 @@ int wxDataViewMainWindow::GetRowByItem(const wxDataViewItem & item) const
         // the parent chain was created by adding the deepest parent first.
         // so if we want to start at the root node, we have to iterate backwards through the vector
         ItemToRowJob job( item, parentChain.rbegin() );
-        Walker( m_root, job );
+        if ( !Walker( m_root, job ) )
+            return -1;
+
         return job.GetResult();
     }
 }

From 31c49caab51f086cf55a2b59fb2c03c1f5f8c25c Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:10:23 +0100
Subject: [PATCH 06/21] Add unit test for wxDataViewCtrl::IsExpanded()

Check that it returns correct results, both for the currently visible
and hidden items.
---
 tests/controls/dataviewctrltest.cpp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp
index d8b20740ed..338d00539c 100644
--- a/tests/controls/dataviewctrltest.cpp
+++ b/tests/controls/dataviewctrltest.cpp
@@ -84,7 +84,7 @@ DataViewCtrlTestCase::DataViewCtrlTestCase(long style)
       m_child2 = m_dvc->AppendItem(m_root, "child2");
 
     m_dvc->SetSize(400, 200);
-    m_dvc->ExpandAncestors(m_root);
+    m_dvc->Expand(m_root);
     m_dvc->Refresh();
     m_dvc->Update();
 }
@@ -188,4 +188,14 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
     TestSelectionFor0and1();
 }
 
+TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
+                 "wxDVC::IsExpanded",
+                 "[wxDataViewCtrl][expand]")
+{
+    CHECK( m_dvc->IsExpanded(m_root) );
+    CHECK( !m_dvc->IsExpanded(m_child1) );
+    CHECK( !m_dvc->IsExpanded(m_grandchild) );
+    CHECK( !m_dvc->IsExpanded(m_child2) );
+}
+
 #endif //wxUSE_DATAVIEWCTRL

From ada5de3d0dcd2aa8a638ced4010b9b3a60aeff30 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:12:08 +0100
Subject: [PATCH 07/21] Fix wxDataViewCtrl::GetItemRect() for collapsed items

Calling GetItemRect() for an item which was not currently visible
because its parent was collapsed resulted in silently returning the
value for a wrong value before the recent fix to GetRowByItem() and in
a crash after it because GetTreeNodeByRow() returned null when passed
invalid row index.

Fix this by explicitly checking whether the item is shown and just
returning an empty rectangle instead.

Also document this behaviour and add a unit test for it.
---
 interface/wx/dataview.h             |  9 +++++++--
 src/generic/datavgen.cpp            |  8 +++++++-
 tests/controls/dataviewctrltest.cpp | 20 ++++++++++++++++++++
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/interface/wx/dataview.h b/interface/wx/dataview.h
index 84c7479df8..1a8ab039ae 100644
--- a/interface/wx/dataview.h
+++ b/interface/wx/dataview.h
@@ -1446,8 +1446,13 @@ public:
     int GetIndent() const;
 
     /**
-        Returns item rectangle. Coordinates of the rectangle are specified in
-        wxDataViewCtrl client area coordinates.
+        Returns item rectangle.
+
+        If item is not currently visible because its parent is collapsed,
+        return an empty rectangle.
+
+        Coordinates of the rectangle are specified in wxDataViewCtrl client
+        area coordinates.
 
         @param item
             A valid item.
diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index 42e55ebb2e..d5a2fffd75 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3809,10 +3809,16 @@ wxRect wxDataViewMainWindow::GetItemRect( const wxDataViewItem & item,
         xpos = 0;
     }
 
+    const int row = GetRowByItem(item);
+    if ( row == -1 )
+    {
+        // This means the row is currently not visible at all.
+        return wxRect();
+    }
+
     // we have to take an expander column into account and compute its indentation
     // to get the correct x position where the actual text is
     int indent = 0;
-    int row = GetRowByItem(item);
     if (!IsList() &&
             (column == 0 || GetExpanderColumnOrFirstOne(GetOwner()) == column) )
     {
diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp
index 338d00539c..6d0d728890 100644
--- a/tests/controls/dataviewctrltest.cpp
+++ b/tests/controls/dataviewctrltest.cpp
@@ -198,4 +198,24 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
     CHECK( !m_dvc->IsExpanded(m_child2) );
 }
 
+TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
+                 "wxDVC::GetItemRect",
+                 "[wxDataViewCtrl][item]")
+{
+#ifdef __WXGTK__
+    WARN("Disabled under GTK because GetItemRect() is not implemented");
+#else
+    const wxRect rect1 = m_dvc->GetItemRect(m_child1);
+    const wxRect rect2 = m_dvc->GetItemRect(m_child2);
+
+    CHECK( rect1.x == rect2.x );
+    CHECK( rect1.width == rect2.width );
+    CHECK( rect1.height == rect2.height );
+    CHECK( rect1.y < rect2.y );
+
+    const wxRect rectNotShown = m_dvc->GetItemRect(m_grandchild);
+    CHECK( rectNotShown == wxRect() );
+#endif
+}
+
 #endif //wxUSE_DATAVIEWCTRL

From ed204211810ac347082cb0ad0704acdbb18ee7b3 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:16:31 +0100
Subject: [PATCH 08/21] Clarify RowToTreeNodeJob in generic wxDataViewCtrl code

Get rid of hardcoded, without any explanation, "-2" value passed to this
class ctor and instead initialize its m_current member to -1 and explain
why do we do it and increment it after processing the current item, not
before, in operator().

No changes in behaviour.
---
 src/generic/datavgen.cpp | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index d5a2fffd75..f2db03c189 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3468,14 +3468,16 @@ int wxDataViewMainWindow::GetLineHeight( unsigned int row ) const
 class RowToTreeNodeJob: public DoJob
 {
 public:
-    RowToTreeNodeJob(int row, int current)
-        : m_row(row), m_current(current), m_ret(NULL)
+    // Note that we initialize m_current to -1 because the first node passed to
+    // our operator() will be the root node, which doesn't appear in the window
+    // and so doesn't count as a real row.
+    explicit RowToTreeNodeJob(int row)
+        : m_row(row), m_current(-1), m_ret(NULL)
     {
     }
 
     virtual int operator() ( wxDataViewTreeNode * node ) wxOVERRIDE
     {
-        m_current ++;
         if( m_current == m_row)
         {
             m_ret = node;
@@ -3484,7 +3486,7 @@ public:
 
         if( node->GetSubTreeCount() + m_current < m_row )
         {
-            m_current += node->GetSubTreeCount();
+            m_current += node->GetSubTreeCount() + 1;
             return  DoJob::SKIP_SUBTREE;
         }
         else
@@ -3500,6 +3502,8 @@ public:
                 return DoJob::DONE;
             }
 
+            m_current++;
+
             return DoJob::CONTINUE;
         }
     }
@@ -3520,7 +3524,7 @@ wxDataViewTreeNode * wxDataViewMainWindow::GetTreeNodeByRow(unsigned int row) co
     if ( row == (unsigned)-1 )
         return NULL;
 
-    RowToTreeNodeJob job( static_cast<int>(row) , -2 );
+    RowToTreeNodeJob job(static_cast<int>(row));
     Walker( m_root , job );
     return job.GetResult();
 }

From 1f0aad61e2597f849abb22809680a3741aa481ce Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:20:11 +0100
Subject: [PATCH 09/21] Use standard naming convention for ItemToRowJob members

Use "m_" prefix for all of them.

No real changes.
---
 src/generic/datavgen.cpp | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index f2db03c189..1d2f65d00f 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3860,18 +3860,16 @@ int wxDataViewMainWindow::RecalculateCount() const
 class ItemToRowJob : public DoJob
 {
 public:
-    ItemToRowJob(const wxDataViewItem& item_, wxVector<wxDataViewItem>::reverse_iterator iter)
-        : m_iter(iter),
-        item(item_)
+    ItemToRowJob(const wxDataViewItem& item, wxVector<wxDataViewItem>::reverse_iterator iter)
+        : m_item(item), m_iter(iter), m_ret(-1)
     {
-        ret = -1;
     }
 
     // Maybe binary search will help to speed up this process
     virtual int operator() ( wxDataViewTreeNode * node) wxOVERRIDE
     {
-        ret ++;
-        if( node->GetItem() == item )
+        m_ret ++;
+        if( node->GetItem() == m_item )
         {
             return DoJob::DONE;
         }
@@ -3883,7 +3881,7 @@ public:
         }
         else
         {
-            ret += node->GetSubTreeCount();
+            m_ret += node->GetSubTreeCount();
             return DoJob::SKIP_SUBTREE;
         }
 
@@ -3891,12 +3889,12 @@ public:
 
     // the row number is begin from zero
     int GetResult() const
-        { return ret -1; }
+        { return m_ret -1; }
 
 private:
+    wxDataViewItem m_item;
     wxVector<wxDataViewItem>::reverse_iterator m_iter;
-    wxDataViewItem item;
-    int ret;
+    int m_ret;
 
 };
 

From 4b8fed3ad62692aa04f3ccdce868f617ccf6f33a Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:21:09 +0100
Subject: [PATCH 10/21] Make ItemToRowJob::m_item const

No real changes, just indicate that this member variable doesn't change.
---
 src/generic/datavgen.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index 1d2f65d00f..994a70171e 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3892,7 +3892,7 @@ public:
         { return m_ret -1; }
 
 private:
-    wxDataViewItem m_item;
+    const wxDataViewItem m_item;
     wxVector<wxDataViewItem>::reverse_iterator m_iter;
     int m_ret;
 

From c3779f2e5dcb6c57762d9347cce3c8e3cd372e19 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:29:24 +0100
Subject: [PATCH 11/21] Clarify ItemToRowJob in generic wxDataViewCtrl code

Rename its m_ret field to a more clear and more consistent with
RowToTreeNodeJob::m_current name and also make m_current, unlike m_ret,
0-based from the beginning instead of having to subtract 1 from it in
GetResult().

There should be no changes in the class behaviour.
---
 src/generic/datavgen.cpp | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index 994a70171e..d82c2f051f 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3860,41 +3860,49 @@ int wxDataViewMainWindow::RecalculateCount() const
 class ItemToRowJob : public DoJob
 {
 public:
+    // As with RowToTreeNodeJob above, we initialize m_current to -1 because
+    // the first node passed to our operator() is the root node which is not
+    // visible on screen and so we should return 0 for its first child node and
+    // not for the root itself.
     ItemToRowJob(const wxDataViewItem& item, wxVector<wxDataViewItem>::reverse_iterator iter)
-        : m_item(item), m_iter(iter), m_ret(-1)
+        : m_item(item), m_iter(iter), m_current(-1)
     {
     }
 
     // Maybe binary search will help to speed up this process
     virtual int operator() ( wxDataViewTreeNode * node) wxOVERRIDE
     {
-        m_ret ++;
         if( node->GetItem() == m_item )
         {
             return DoJob::DONE;
         }
 
+        // Is this node the next (grand)parent of the item we're looking for?
         if( node->GetItem() == *m_iter )
         {
+            // Search for the next (grand)parent now and skip this item itself.
             ++m_iter;
+            ++m_current;
             return DoJob::CONTINUE;
         }
         else
         {
-            m_ret += node->GetSubTreeCount();
+            // Skip this node and all its currently visible children.
+            m_current += node->GetSubTreeCount() + 1;
             return DoJob::SKIP_SUBTREE;
         }
 
     }
 
-    // the row number is begin from zero
     int GetResult() const
-        { return m_ret -1; }
+        { return m_current; }
 
 private:
     const wxDataViewItem m_item;
     wxVector<wxDataViewItem>::reverse_iterator m_iter;
-    int m_ret;
+
+    // The row corresponding to the last node seen in our operator().
+    int m_current;
 
 };
 

From 9b7757c44db95514cdfdf96de731410bb31750b8 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:49:12 +0100
Subject: [PATCH 12/21] Ensure that wxDataViewMainWindow has correct size in
 the test

Replace a redundant (because the same size was already specified in the
ctor) SetSize() call with a Layout() call which resizes
wxDataViewMainWindow to fit the parent control size when using the
generic implementation.

This is important for any tests dealing with the control geometry, i.e.
calling GetItemRect() or HitTest().
---
 tests/controls/dataviewctrltest.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp
index 6d0d728890..720fdce938 100644
--- a/tests/controls/dataviewctrltest.cpp
+++ b/tests/controls/dataviewctrltest.cpp
@@ -83,7 +83,7 @@ DataViewCtrlTestCase::DataViewCtrlTestCase(long style)
         m_grandchild = m_dvc->AppendItem(m_child1, "grandchild");
       m_child2 = m_dvc->AppendItem(m_root, "child2");
 
-    m_dvc->SetSize(400, 200);
+    m_dvc->Layout();
     m_dvc->Expand(m_root);
     m_dvc->Refresh();
     m_dvc->Update();

From 9460038b3ff1aa3767924b6ef44be4e0da7f9b39 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 17:50:55 +0100
Subject: [PATCH 13/21] Return empty rectangle from GetItemRect() if item is
 not visible

This was already the case if the item was not visible because its parent
was not expanded, but now make it also true for the items which are not
visible due to the current scrollbar position.

Add unit tests checking for this and also verifying that GetItemRect()
returns the coordinates in the physical window coordinates.
---
 interface/wx/dataview.h             |  5 +++--
 src/generic/datavgen.cpp            | 15 +++++++++++++--
 tests/controls/dataviewctrltest.cpp | 27 +++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/interface/wx/dataview.h b/interface/wx/dataview.h
index 1a8ab039ae..85bc40acdb 100644
--- a/interface/wx/dataview.h
+++ b/interface/wx/dataview.h
@@ -1448,8 +1448,9 @@ public:
     /**
         Returns item rectangle.
 
-        If item is not currently visible because its parent is collapsed,
-        return an empty rectangle.
+        If item is not currently visible, either because its parent is
+        collapsed or it is outside of the visible part of the control due to
+        the current vertical scrollbar position, return an empty rectangle.
 
         Coordinates of the rectangle are specified in wxDataViewCtrl client
         area coordinates.
diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp
index d82c2f051f..41c3b7fbed 100644
--- a/src/generic/datavgen.cpp
+++ b/src/generic/datavgen.cpp
@@ -3839,6 +3839,14 @@ wxRect wxDataViewMainWindow::GetItemRect( const wxDataViewItem & item,
     GetOwner()->CalcScrolledPosition(  itemRect.x,  itemRect.y,
                                       &itemRect.x, &itemRect.y );
 
+    // Check if the rectangle is completely outside of the currently visible
+    // area and, if so, return an empty rectangle to indicate that the item is
+    // not visible.
+    if ( itemRect.GetBottom() < 0 || itemRect.GetTop() > GetClientSize().y )
+    {
+        return wxRect();
+    }
+
     return itemRect;
 }
 
@@ -5853,8 +5861,11 @@ wxRect wxDataViewCtrl::GetItemRect( const wxDataViewItem & item,
     // Convert position from the main window coordinates to the control coordinates.
     // (They can be different due to the presence of the header.).
     wxRect r = m_clientArea->GetItemRect(item, column);
-    const wxPoint ctrlPos = ScreenToClient(m_clientArea->ClientToScreen(r.GetPosition()));
-    r.SetPosition(ctrlPos);
+    if ( r.width || r.height )
+    {
+        const wxPoint ctrlPos = ScreenToClient(m_clientArea->ClientToScreen(r.GetPosition()));
+        r.SetPosition(ctrlPos);
+    }
     return r;
 }
 
diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp
index 720fdce938..f7fcaca67e 100644
--- a/tests/controls/dataviewctrltest.cpp
+++ b/tests/controls/dataviewctrltest.cpp
@@ -208,6 +208,9 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
     const wxRect rect1 = m_dvc->GetItemRect(m_child1);
     const wxRect rect2 = m_dvc->GetItemRect(m_child2);
 
+    CHECK( rect1 != wxRect() );
+    CHECK( rect2 != wxRect() );
+
     CHECK( rect1.x == rect2.x );
     CHECK( rect1.width == rect2.width );
     CHECK( rect1.height == rect2.height );
@@ -215,6 +218,30 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
 
     const wxRect rectNotShown = m_dvc->GetItemRect(m_grandchild);
     CHECK( rectNotShown == wxRect() );
+
+    // Append enough items to make the window scrollable.
+    for ( int i = 3; i < 100; ++i )
+        m_dvc->AppendItem(m_root, wxString::Format("child%d", i));
+
+    const wxDataViewItem last = m_dvc->AppendItem(m_root, "last");
+
+    // This should scroll the window to bring this item into view.
+    m_dvc->EnsureVisible(last);
+
+    // Check that this was indeed the case.
+    const wxDataViewItem top = m_dvc->GetTopItem();
+    CHECK( top != m_root );
+
+    // Verify that the coordinates are returned in physical coordinates of the
+    // window and not the logical coordinates affected by scrolling.
+    const wxRect rectScrolled = m_dvc->GetItemRect(top);
+    CHECK( rectScrolled.GetBottom() > 0 );
+    CHECK( rectScrolled.GetTop() <= m_dvc->GetClientSize().y );
+
+    // Also check that the root item is not currently visible (because it's
+    // scrolled off).
+    const wxRect rectRoot = m_dvc->GetItemRect(m_root);
+    CHECK( rectRoot == wxRect() );
 #endif
 }
 

From 9d2ee59138b298721f02ff90710d4354baef51ca Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 18:02:33 +0100
Subject: [PATCH 14/21] Update comments in tests/asserthelper.h

No real changes, just avoid mentioning CPPUNIT_ASSERT_EQUAL() in the
comments for the operator<<() overloads as it is used with CATCH too.

Also don't duplicate the same comment 4 times unnecessarily.
---
 tests/asserthelper.h | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/tests/asserthelper.h b/tests/asserthelper.h
index ce8c9c6a29..ce1d1fba2a 100644
--- a/tests/asserthelper.h
+++ b/tests/asserthelper.h
@@ -29,16 +29,11 @@ namespace wxTestPrivate
     std::ostream& operator<<(std::ostream& os, const ColourChannel& cc);
 } // wxTestPrivate namespace
 
-// this operator is needed to use CPPUNIT_ASSERT_EQUAL with wxColour objects
+// Operators used to show the values of the corresponding types when comparing
+// them in the unit tests fails.
 std::ostream& operator<<(std::ostream& os, const wxColour& c);
-
-// this operator is needed to use CPPUNIT_ASSERT_EQUAL with wxSize objects
 std::ostream& operator<<(std::ostream& os, const wxSize& s);
-
-// this operator is needed to use CPPUNIT_ASSERT_EQUAL with wxFont objects
 std::ostream& operator<<(std::ostream& os, const wxFont& f);
-
-// this operator is needed to use CPPUNIT_ASSERT_EQUAL with wxPoint objects
 std::ostream& operator<<(std::ostream& os, const wxPoint& p);
 
 #endif

From 12f8ab20f998cc70e1de6fb00b801401daf65848 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 18:05:25 +0100
Subject: [PATCH 15/21] Move operator<<(std::ostream&, wxRect) overload to a
 header

This will allow reusing it in other tests too.

Also make the output slightly more readable by formatting the rectangle
as "{x,y w*h}" instead of "{x,y,w,h}".

No real changes.
---
 tests/asserthelper.cpp  |  8 ++++++++
 tests/asserthelper.h    |  1 +
 tests/geometry/rect.cpp | 13 +------------
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/tests/asserthelper.cpp b/tests/asserthelper.cpp
index 8f9f36d452..7b25cec2d6 100644
--- a/tests/asserthelper.cpp
+++ b/tests/asserthelper.cpp
@@ -67,3 +67,11 @@ std::ostream& operator<<(std::ostream& os, const wxPoint& p)
 
     return os;
 }
+
+std::ostream& operator<<(std::ostream& os, const wxRect& r)
+{
+    os << "{"
+        << r.x << ", " << r.y << "  " << r.width << "*" << r.height
+       << "}";
+    return os;
+}
diff --git a/tests/asserthelper.h b/tests/asserthelper.h
index ce1d1fba2a..57772453bb 100644
--- a/tests/asserthelper.h
+++ b/tests/asserthelper.h
@@ -35,5 +35,6 @@ std::ostream& operator<<(std::ostream& os, const wxColour& c);
 std::ostream& operator<<(std::ostream& os, const wxSize& s);
 std::ostream& operator<<(std::ostream& os, const wxFont& f);
 std::ostream& operator<<(std::ostream& os, const wxPoint& p);
+std::ostream& operator<<(std::ostream& os, const wxRect& r);
 
 #endif
diff --git a/tests/geometry/rect.cpp b/tests/geometry/rect.cpp
index 07634a3ce6..94e67c9574 100644
--- a/tests/geometry/rect.cpp
+++ b/tests/geometry/rect.cpp
@@ -22,18 +22,7 @@
 
 #include "wx/iosfwrap.h"
 
-// ----------------------------------------------------------------------------
-// helper functions
-// ----------------------------------------------------------------------------
-
-// this operator is needed to use CPPUNIT_ASSERT_EQUAL with wxRects
-std::ostream& operator<<(std::ostream& os, const wxRect& r)
-{
-    os << "{"
-        << r.x << ", " << r.y << ", " << r.width << ", " << r.height
-       << "}";
-    return os;
-}
+#include "asserthelper.h"
 
 // ----------------------------------------------------------------------------
 // test class

From 94a2595f5b53cca321c28447c8857308a965d165 Mon Sep 17 00:00:00 2001
From: Umberto Carletti <umberto.carletti@gmail.com>
Date: Mon, 22 Oct 2018 10:13:34 +0200
Subject: [PATCH 16/21] Add GTK implementation of wxDataViewCtrl::GetItemRect()

This function returns a wxRect of the given wxDataViewItem. If no column
is provided, the width will be the sum of all the visible columns widths.

See https://github.com/wxWidgets/wxWidgets/pull/990
---
 src/gtk/dataview.cpp | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/gtk/dataview.cpp b/src/gtk/dataview.cpp
index 8e1a6503a3..c5662aa26e 100644
--- a/src/gtk/dataview.cpp
+++ b/src/gtk/dataview.cpp
@@ -5289,10 +5289,36 @@ void wxDataViewCtrl::HitTest(const wxPoint& point,
 }
 
 wxRect
-wxDataViewCtrl::GetItemRect(const wxDataViewItem& WXUNUSED(item),
-                            const wxDataViewColumn *WXUNUSED(column)) const
+wxDataViewCtrl::GetItemRect(const wxDataViewItem& item,
+                            const wxDataViewColumn *column) const
 {
-    return wxRect();
+    if ( !item )
+        return wxRect();
+
+    GtkTreeViewColumn *gcolumn = NULL ;
+    if (column)
+        gcolumn = GTK_TREE_VIEW_COLUMN(column->GetGtkHandle());
+
+    GtkTreeIter iter;
+    iter.user_data = item.GetID();
+    wxGtkTreePath path(m_internal->get_path( &iter ));
+
+    GdkRectangle item_rect;
+    gtk_tree_view_get_background_area(GTK_TREE_VIEW(m_treeview), path, gcolumn, &item_rect);
+    // If column is NULL we compute the combined width of all the columns
+    if ( !column )
+    {
+        unsigned int cols = GetColumnCount();
+        int width = 0;
+        for (unsigned int i = 0; i < cols; ++i)
+        {
+            wxDataViewColumn * col = GetColumn(i);
+            if ( !col->IsHidden() )
+                width += col->GetWidth();
+        }
+        item_rect.width = width;
+    }
+    return wxRectFromGDKRect(&item_rect);
 }
 
 bool wxDataViewCtrl::SetRowHeight(int rowHeight)

From fad50e74b7737708233cf6fcc2f374d69c79b923 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 18:25:16 +0100
Subject: [PATCH 17/21] Fix wxDataViewCtrl::GetItemRect() in wxGTK and enable
 its test

A couple of fixes compared to the previous commit:

- Use the correct gtk_tree_view_get_cell_area() rather than
  gtk_tree_view_get_background_area() which doesn't work correctly
  for the items which are not shown because their parent is collapsed.
- Translate logical coordinates to physical ones using
  gtk_tree_view_convert_bin_window_to_widget_coords().

With these fixes, the unit tests for this function pass and can now be
enabled under wxGTK as well.

See https://github.com/wxWidgets/wxWidgets/pull/990
---
 src/gtk/dataview.cpp                | 37 ++++++++++++++++++++++++++++-
 tests/controls/dataviewctrltest.cpp | 19 +++++++++++----
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/src/gtk/dataview.cpp b/src/gtk/dataview.cpp
index c5662aa26e..d2c1abf352 100644
--- a/src/gtk/dataview.cpp
+++ b/src/gtk/dataview.cpp
@@ -5304,7 +5304,15 @@ wxDataViewCtrl::GetItemRect(const wxDataViewItem& item,
     wxGtkTreePath path(m_internal->get_path( &iter ));
 
     GdkRectangle item_rect;
-    gtk_tree_view_get_background_area(GTK_TREE_VIEW(m_treeview), path, gcolumn, &item_rect);
+    gtk_tree_view_get_cell_area(GTK_TREE_VIEW(m_treeview), path, gcolumn, &item_rect);
+
+    // GTK returns rectangles with the position and height, but not width, for
+    // some reason, set to 0 if the item is not currently shown, so an explicit
+    // check is needed as this rectangle is not quite the empty rectangle we're
+    // supposed to return in this case.
+    if ( item_rect.height == 0 )
+        return wxRect();
+
     // If column is NULL we compute the combined width of all the columns
     if ( !column )
     {
@@ -5318,6 +5326,33 @@ wxDataViewCtrl::GetItemRect(const wxDataViewItem& item,
         }
         item_rect.width = width;
     }
+
+    // We need to convert logical coordinates to physical ones, i.e. the
+    // rectangle of the topmost item should start at ~0, even if it's a 100th
+    // item shown on top only because the window is scrolled.
+#if GTK_CHECK_VERSION(2, 12, 0)
+    if ( wx_is_at_least_gtk2(12) )
+    {
+        gtk_tree_view_convert_bin_window_to_widget_coords
+        (
+            GTK_TREE_VIEW(m_treeview),
+            item_rect.x, item_rect.y,
+            &item_rect.x, &item_rect.y
+        );
+
+        if ( item_rect.y > GetClientSize().y ||
+                item_rect.y + item_rect.height < 0 )
+        {
+            // If it turns out that the item is not visible at all, indicate it
+            // by returning an empty rectangle for it.
+            return wxRect();
+        }
+    }
+    //else: There doesn't seem to be anything reasonable to do here, so we'll
+    //      just return wrong values with the very old GTK+ versions if the
+    //      window is scrolled.
+#endif // GTK+ 2.12+
+
     return wxRectFromGDKRect(&item_rect);
 }
 
diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp
index f7fcaca67e..0f99eecfbe 100644
--- a/tests/controls/dataviewctrltest.cpp
+++ b/tests/controls/dataviewctrltest.cpp
@@ -22,6 +22,7 @@
 #include "wx/dataview.h"
 
 #include "testableframe.h"
+#include "asserthelper.h"
 
 // ----------------------------------------------------------------------------
 // test class
@@ -203,8 +204,10 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
                  "[wxDataViewCtrl][item]")
 {
 #ifdef __WXGTK__
-    WARN("Disabled under GTK because GetItemRect() is not implemented");
-#else
+    // We need to let the native control have some events to lay itself out.
+    wxYield();
+#endif // __WXGTK__
+
     const wxRect rect1 = m_dvc->GetItemRect(m_child1);
     const wxRect rect2 = m_dvc->GetItemRect(m_child2);
 
@@ -214,7 +217,11 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
     CHECK( rect1.x == rect2.x );
     CHECK( rect1.width == rect2.width );
     CHECK( rect1.height == rect2.height );
-    CHECK( rect1.y < rect2.y );
+
+    {
+        INFO("First child: " << rect1 << ", second one: " << rect2);
+        CHECK( rect1.y < rect2.y );
+    }
 
     const wxRect rectNotShown = m_dvc->GetItemRect(m_grandchild);
     CHECK( rectNotShown == wxRect() );
@@ -228,6 +235,11 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
     // This should scroll the window to bring this item into view.
     m_dvc->EnsureVisible(last);
 
+#ifdef __WXGTK__
+    // And again to let it scroll the correct items into view.
+    wxYield();
+#endif
+
     // Check that this was indeed the case.
     const wxDataViewItem top = m_dvc->GetTopItem();
     CHECK( top != m_root );
@@ -242,7 +254,6 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
     // scrolled off).
     const wxRect rectRoot = m_dvc->GetItemRect(m_root);
     CHECK( rectRoot == wxRect() );
-#endif
 }
 
 #endif //wxUSE_DATAVIEWCTRL

From bf97715972fb05adf2df10c27571dd1e1b62f1c3 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 18:35:07 +0100
Subject: [PATCH 18/21] Support NULL column in wxDataViewCtrl::GetItemRect() in
 wxOSX

Emulate support for non-specified column by using the first and, if
necessary, last columns in this case.
---
 interface/wx/dataview.h  |  4 +---
 src/osx/dataview_osx.cpp | 22 ++++++++++++++++++----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/interface/wx/dataview.h b/interface/wx/dataview.h
index 85bc40acdb..a001760b05 100644
--- a/interface/wx/dataview.h
+++ b/interface/wx/dataview.h
@@ -1462,9 +1462,7 @@ public:
             intersection of the item with the specified column. If @NULL, the
             rectangle spans all the columns.
 
-        @note This method is currently not implemented at all in wxGTK and only
-              implemented for non-@NULL @a col argument in wxOSX. It is fully
-              implemented in the generic version of the control.
+        @note This method is currently not implemented in wxGTK.
     */
     virtual wxRect GetItemRect(const wxDataViewItem& item,
                                const wxDataViewColumn* col = NULL) const;
diff --git a/src/osx/dataview_osx.cpp b/src/osx/dataview_osx.cpp
index f49a360dc6..57a1cf3970 100644
--- a/src/osx/dataview_osx.cpp
+++ b/src/osx/dataview_osx.cpp
@@ -547,10 +547,24 @@ wxDataViewColumn *wxDataViewCtrl::GetCurrentColumn() const
 
 wxRect wxDataViewCtrl::GetItemRect(wxDataViewItem const& item, wxDataViewColumn const* columnPtr) const
 {
-  if (item.IsOk() && (columnPtr != NULL))
-    return GetDataViewPeer()->GetRectangle(item,columnPtr);
-  else
-    return wxRect();
+  if ( !item.IsOk() )
+      return wxRect();
+
+  wxRect rect = GetDataViewPeer()->GetRectangle(item, columnPtr ? columnPtr : GetColumn(0));
+
+  if ( !columnPtr )
+  {
+      const unsigned columnCount = GetColumnCount();
+      if ( columnCount != 1 )
+      {
+          // Extend the rectangle to the rightmost part of the last column.
+          const wxRect rectLastCol = GetDataViewPeer()->GetRectangle(item, GetColumn(columnCount - 1));
+          rect.SetRight(rectLastCol.GetRight());
+      }
+      //else: We already have the rectangle we need.
+  }
+
+  return rect;
 }
 
 int wxDataViewCtrl::GetSelectedItemsCount() const

From 1256695d46bc683cb3fc1311809823c191886a9e Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Sun, 4 Nov 2018 18:37:54 +0100
Subject: [PATCH 19/21] Document that wxDataViewCtrl::GetItemRect() now works
 in wxGTK

Remove the note about it being non-implemented from the docs and mention
this in the change log.
---
 docs/changes.txt        | 1 +
 interface/wx/dataview.h | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/docs/changes.txt b/docs/changes.txt
index 25d36877e0..e5ff0d7536 100644
--- a/docs/changes.txt
+++ b/docs/changes.txt
@@ -141,6 +141,7 @@ wxGTK:
 - Fix the build with glib < 2.32 (e.g. CentOS 6).
 - Fix field widths in wxStatusBar showing a size grip.
 - Fill column value in wxEVT_DATAVIEW_ITEM_ACTIVATED events.
+- Implement wxDataViewCtrl::GetItemRect() (MrMeesek).
 
 wxMSW:
 
diff --git a/interface/wx/dataview.h b/interface/wx/dataview.h
index a001760b05..06a662722d 100644
--- a/interface/wx/dataview.h
+++ b/interface/wx/dataview.h
@@ -1461,8 +1461,6 @@ public:
             If non-@NULL, the rectangle returned corresponds to the
             intersection of the item with the specified column. If @NULL, the
             rectangle spans all the columns.
-
-        @note This method is currently not implemented in wxGTK.
     */
     virtual wxRect GetItemRect(const wxDataViewItem& item,
                                const wxDataViewColumn* col = NULL) const;

From 767639276f45c3c9e68b18b2b50278394c27ae94 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Tue, 6 Nov 2018 03:57:35 +0100
Subject: [PATCH 20/21] Disable failing wxDataViewCtrl::IsExpanded() test under
 Mac

For some unfathomable reason IsExpanded() returns wrong value for one of
the items. This should be fixed, but for now just leave a warning in the
test but don't fail it.

Also document this bug to at least spare people some surprises.
---
 interface/wx/dataview.h             | 3 +++
 tests/controls/dataviewctrltest.cpp | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/interface/wx/dataview.h b/interface/wx/dataview.h
index 06a662722d..00a8069f06 100644
--- a/interface/wx/dataview.h
+++ b/interface/wx/dataview.h
@@ -1558,6 +1558,9 @@ public:
 
     /**
         Return @true if the item is expanded.
+
+        @note When using the native macOS version this method has a bug which
+              may result in returning @true even for items without children.
     */
     virtual bool IsExpanded(const wxDataViewItem& item) const;
 
diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp
index 0f99eecfbe..83e712a8f3 100644
--- a/tests/controls/dataviewctrltest.cpp
+++ b/tests/controls/dataviewctrltest.cpp
@@ -195,7 +195,13 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase,
 {
     CHECK( m_dvc->IsExpanded(m_root) );
     CHECK( !m_dvc->IsExpanded(m_child1) );
+    // No idea why, but the native NSOutlineView isItemExpanded: method returns
+    // true for this item for some reason.
+#ifdef __WXOSX__
+    WARN("Disabled under MacOS: IsExpanded() returns true for grand child");
+#else
     CHECK( !m_dvc->IsExpanded(m_grandchild) );
+#endif
     CHECK( !m_dvc->IsExpanded(m_child2) );
 }
 

From caa270a1af94cd4576afbd049dd2060ba7827d10 Mon Sep 17 00:00:00 2001
From: Vadim Zeitlin <vadim@wxwidgets.org>
Date: Tue, 6 Nov 2018 04:17:27 +0100
Subject: [PATCH 21/21] Bring Mac wxDataViewCtrl::GetItemRect() in sync with
 other ports

Return empty rectangle if the item is not currently visible, for
whatever reason, and use physical coordinates for the rectangle origin.

This makes the unit test for GetItemRect() pass under macOS too.
---
 src/osx/cocoa/dataview.mm | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/osx/cocoa/dataview.mm b/src/osx/cocoa/dataview.mm
index d7f6fc362b..ce7d333e0d 100644
--- a/src/osx/cocoa/dataview.mm
+++ b/src/osx/cocoa/dataview.mm
@@ -2295,8 +2295,33 @@ wxDataViewItem wxCocoaDataViewControl::GetTopItem() const
 
 wxRect wxCocoaDataViewControl::GetRectangle(const wxDataViewItem& item, const wxDataViewColumn *columnPtr)
 {
-    return wxFromNSRect([m_osxView superview],[m_OutlineView frameOfCellAtColumn:GetColumnPosition(columnPtr)
+    NSView* const parent = [m_osxView superview];
+
+    wxRect r = wxFromNSRect(parent, [m_OutlineView frameOfCellAtColumn:GetColumnPosition(columnPtr)
             row:[m_OutlineView rowForItem:[m_DataSource getDataViewItemFromBuffer:item]]]);
+
+    // For hidden items, i.e. items not shown because their parent is
+    // collapsed, the native method returns rectangles with negative width, but
+    // we're supposed to just return an empty rectangle in this case. To be on
+    // the safe side, also check for the height as well, even if it seems to be
+    // always 0 in this case.
+    if ( r.width < 0 || r.height < 0 )
+        return wxRect();
+
+    // Also adjust the vertical coordinates to use physical window coordinates
+    // instead of the logical ones returned by frameOfCellAtColumn:row:
+    NSScrollView* const scrollView = [m_OutlineView enclosingScrollView];
+    const wxRect
+        visible = wxFromNSRect(parent, scrollView.contentView.visibleRect);
+
+    // We are also supposed to return empty rectangle if the item is not
+    // visible because it is scrolled out of view.
+    if ( r.GetBottom() < visible.GetTop() || r.GetTop() > visible.GetBottom() )
+        return wxRect();
+
+    r.y -= visible.y;
+
+    return r;
 }
 
 bool wxCocoaDataViewControl::IsExpanded(const wxDataViewItem& item) const