two cross lines issue when I have a timer to call UpdateAll periodically

[asmwarrior]

We have discuss this issue in another issue(debug assert from wxWidgets when I drag a mpBitmapLayer), but let me create a new one here, because that one is for the bitmap issue.

When debugging, I found that when UpdateAll() function get called, the cross line is cleared.

Because there is a function call:

void mpWindow::UpdateAll()
{
    if (UpdateBBox())
    {
        ......
    }

    if (m_magnetize)
      m_magnet.ClearPlot(*this);

    Refresh(false);
}

In the void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event)), we don't re-create this cross line.

I relieve that if cross line is enabled, when we call UpdateAll(), it got cleared. But we have to mark is as "temporarily cleared", because in the OnPaint event, we will draw the whole window, but after that, do we need to "re-create" it?

[asmwarrior]

I add some log messages in the OnPaint or other functions, and got some log.txt file to see what is the problem.

I think the problem happens in such case:

When I call the UpdateAll, the cross line get cleared, and later in the OnPaint event handler, the window get repaint.

Now, here comes the bug, after that time, if I move the mouse, I see in some situation, I see the mpMagnet::ClearPlot get called (mainly in the mouse move event handler), and in this case, the m_Insideis true, so that:

void mpMagnet::ClearPlot(mpWindow &w)
{
    if (m_Inside)
    {
        wxClientDC dc(&w);
        dc.SetPen(*wxBLACK_PEN);
        dc.SetLogicalFunction(wxINVERT);
        dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
        dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
        m_Inside = false;
        dc.SetLogicalFunction(wxCOPY);
    }
}

In this case, the cross line get drawed by the XOR method(wxINVERT).

So, I think the solution is that whether the cross line get draw is a status, and maybe, we need a bool variable in the mpMagnet class.

I mean if it get cleared, we don't need to clear it again.

[asmwarrior]

I debugged a while by using the logs. I found that in some case, I got such sequence:

21:53:05: mpWindow::UpdateAll
21:53:05: mpMagnet::ClearPlot, m_Inside = true
21:53:05: mpMagnet::ClearPlot, m_Inside = false!!!
21:53:05: mpMagnet::Plot enter >>>>>>> 
21:53:05: mpMagnet::Plot draw COPY method!!!!!!
21:53:05: mpMagnet::Plot leave <<<<<<<<<< 
21:53:05: mpWindow::OnPaint paint on the memory DC
21:53:05: mpWindow::OnPaint Paint all****
21:53:05: mpWindow::OnPaint memory DC copy to screen

When UpdateAll get called, it just clear the cross line, but the mouse event still happens before the OnPaint event, so that when OnPaint event happens, the status of the cross line could be either shown or not. But when OnPaint happens, we have to force the status of the magnet to its initial status.

This is the patch, pay attention to the function call m_magnet.SetInitState(), I just set its value to the initial status.

diff --git a/mathplot/mathplot.cpp b/mathplot/mathplot.cpp
index 5478907..847bc1b 100644
--- a/mathplot/mathplot.cpp
+++ b/mathplot/mathplot.cpp
@@ -2844,6 +2844,8 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))
        delete m_buff_dc;
    }
    m_repainting = false;
+
+   m_magnet.SetInitState(); // the window get repaint, so set the magnet to initial status
 }

 void mpWindow::SetMPScrollbars(bool status)
@@ -4560,6 +4562,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
        dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
        m_Inside = false;
        m_rightClick = false;
+       m_IsShown = false;
    }

    if (m_domain.Contains(mousePos))
@@ -4577,6 +4580,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
                dc.SetLogicalFunction(wxCOPY);
                m_mousePosition_old = mousePos;
                m_Inside = true;
+               m_IsShown = true;
            }
        }
    }
@@ -4586,7 +4590,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)

 void mpMagnet::ClearPlot(mpWindow &w)
 {
-   if (m_Inside)
+   if (m_Inside && m_IsShown)
    {
        wxClientDC dc(&w);
        dc.SetPen(*wxBLACK_PEN);
@@ -4594,6 +4598,7 @@ void mpMagnet::ClearPlot(mpWindow &w)
        dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
        dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
        m_Inside = false;
+       m_IsShown = false;
        dc.SetLogicalFunction(wxCOPY);
    }
 }
diff --git a/mathplot/mathplot.h b/mathplot/mathplot.h
index dfa4761..3b263e7 100644
--- a/mathplot/mathplot.h
+++ b/mathplot/mathplot.h
@@ -1538,11 +1538,19 @@ class mpMagnet
        {
            m_Inside = false;
            m_rightClick = false;
+           m_IsShown = false;
        }
        ~mpMagnet()
        {
-           ;
        }
+
+       void SetInitState()
+       {
+           m_Inside = false;
+           m_rightClick = false;
+           m_IsShown = false;
+       }
+
        void UpdateBox(wxCoord left, wxCoord top, wxCoord width, wxCoord height)
        {
            m_domain = wxRect(left, top, width, height);
@@ -1562,6 +1570,7 @@ class mpMagnet
        wxPoint m_mousePosition_old;
        bool m_Inside;
        bool m_rightClick;
+       bool m_IsShown;
 };

 /** Canvas for plotting mpLayer implementations.

The above patch fixes the issue.

But I think there is still one thing remains. After the repaint event, if mouse is not moved, no cross line is shown. So, we need another way to emulate a mouse move event? Or similar things to draw the cross line again.

[GitHubLionel]

Hi, You go to fast, for the moment, I just push the last corrections. For this problem, I need to test your patch. In fact, when you paint, refresh or updateAll, you don't need to draw the cross. We just need to say to mpMagnet to not redraw the cross if the mouse move because the background has changed. I suppose that your boolean m_IsShown play that role, I will test.

[asmwarrior]

Yes, I think after we call UpdateAll(), we need to "stop/disable" the drawing of mpMagnet.

But in some cases, some events still happens before the OnPaint event, so that the there is still a chance the cross lines were drawn again.

For those reason, I think we can either "make a clean reset of mpMagnet" after the "OnPaint", which is what my patch does.

Further more, which I don't implement is that I think a better way is we can "disable the drawing feature of mpMagnet" in the UpdateAll() function(Not just clear the cross lines), and "re-enable the drawing feature of mpMagnet" after "OnPaint". This way, we just drop all the events between "UpdateAll()" and "OnPaint" for drawing or clearing the cross lines.

[GitHubLionel]

Hi

Ok, I reorganize the process. I also redraw the cross after the paint. Now the cross is still visible if we zoom, pan and select rectangle. I test with a repaint in a timer (the original sample3 with a moving car) and it work. Same time, I change some words and some functions.

[asmwarrior]

I just test the latest master ahead code, I see that, if I don't have a timer to refresh the window, I still see two crosses when I zoom the window, so this is the first bug. And the second bug is that if I start the timer, which call UpdateAll() every 3 seconds, I still see sometimes, there are two crosses in the window, this only goes back to one cross when the OnPaint() happens.

[GitHubLionel]

I can not reproduce this bug, neither with MathPlotConfig (the sinus), neither with MathPlotDemo or with sample3 (a moving car with a timer 25 ms). UpdateAll() call Refresh() who call OnPaint(), so all the scene is replotted, so no need to erase the cross before. We just need to keep in memory if the cross was drawn before to repaint it after (that is the goal of the ReInitDrawn() method). May be you can try Refresh(true); in line 3011. Note that we can use the flag m_repainting at beginning of UpdateAll() to prevent the cross drawing.

[asmwarrior]

I think I can reproduce this bug, I'm using the MathPlotConfig (the sinus). I don't have any wxTimer to refresh the window. I just keep the mouse moving, also sometimes, I scroll(zoom) the window. You can have a look at my screen cast video below.

https://github.com/GitHubLionel/wxMathPlot/assets/561818/cd9b12ee-bd2d-4ef7-8619-c9cc602a6ae9

EDIT: it looks like my Firefox can't play the attached mp4 file on the web, but I can confirm that you can just download the mp4 file to your local disk, and use some player(I'm using vlc player) to play it.

[asmwarrior]

        void UpdateBox(wxCoord left, wxCoord top, wxCoord width, wxCoord height)
        {
            m_domain = wxRect(left, top, width, height);
            m_plot_size = wxRect(left, top, width + left, height + top);
        }

When reading the code, I see the above code. Those two member variable shared the same left and top. And m_plot_size is actually not a wxSize, but a wxRect. Do you think it is redundant?

I also draw a ascii art to demonstrate:

//  +---------------------------------+
//  |  Plot_size_XY(width,height)     |
//  |  +--------------------------+   |
//  |  |     |                    |   |
//  |  |     |                    |   |
//  |  |-----|--------------------|   |
//  |  |     |MousePos            |   |
//  |  |     |                    |   |
//  |  |     |                    |   |
//  |  +--------------------------+   |
//  +---------------------------------+
//             mPWindow

[GitHubLionel]

I think I can reproduce this bug, I'm using the MathPlotConfig (the sinus). I don't have any wxTimer to refresh the window. I just keep the mouse moving, also sometimes, I scroll(zoom) the window. You can have a look at my screen cast video below.

No, for me, no problem even moving faster the mouse when zooming or moving pan faster in all directions. I only can have this problem one or two times when I disabled double buffering (and if I move mouse very faster) and so have flickering. What is strange on your video is that the cross is not always drawing when the mouse move. For me, the cross is always visible.

[GitHubLionel]

Those two member variable shared the same left and top. And m_plot_size is actually not a wxSize, but a wxRect. Do you think it is redundant?

Not redundant but the word m_plot_size is maybe not the best.

[asmwarrior]

When using the log, I can see the bug happens here:

18:01:41: mpMagnet::Plot enter >>>>>>> 
18:01:41: mpMagnet::Plot draw COPY method!!!!!!
18:01:41: mpMagnet::Plot leave <<<<<<<<<< 
18:01:41: mpWindow::UpdateAll
18:01:41: mpWindow::OnPaint paint on the memory DC
18:01:41: mpWindow::OnPaint Paint all****
18:01:41: mpWindow::OnPaint memory DC copy to screen
18:01:41: mpMagnet::UpdatePlot m_IsWasDrawn=true****
18:01:41: mpMagnet::ClearPlot, m_InDrawn = true
18:01:41: mpMagnet::ClearPlot, m_InDrawn = true
18:01:41: mpMagnet::Plot enter >>>>>>> 
18:01:41: mpMagnet::Plot draw COPY method!!!!!!
18:01:41: mpMagnet::Plot leave <<<<<<<<<< 

You see, when mpMagnet::UpdatePlot get called, the m_IsWasDrawn is true.

You got mpMagnet::ClearPlot, m_InDrawn = true called 2 times later.

18:01:43: mpMagnet::ClearPlot, m_InDrawn = true
18:01:43: mpMagnet::Plot enter >>>>>>> 
18:01:43: mpMagnet::Plot draw XOR method, m_Inside(true)
18:01:43: mpMagnet::Plot draw COPY method!!!!!!
18:01:43: mpMagnet::Plot leave <<<<<<<<<< 
18:01:43: mpWindow::OnPaint paint on the memory DC
18:01:43: mpWindow::OnPaint Paint all****
18:01:43: mpWindow::OnPaint memory DC copy to screen
18:01:43: mpMagnet::UpdatePlot m_IsWasDrawn=false!!!!
18:01:43: mpMagnet::ClearPlot, m_InDrawn = true
18:01:43: mpMagnet::Plot enter >>>>>>> 
18:01:43: mpMagnet::Plot draw COPY method!!!!!!
18:01:43: mpMagnet::Plot leave <<<<<<<<<< 

In another case, the mpMagnet::ClearPlot called once, which draw the extra cross.

I will post the patch(I used the debug log) below:

From 2d7c6a65f0150e3874d17a3a01732063f5fa0087 Mon Sep 17 00:00:00 2001
From: hidehide <hidehide@hidehide.com>
Date: Sun, 14 May 2023 22:09:11 +0800
Subject: [PATCH] add the debug code

---
 MathPlotConfig/MathPlotConfigDemo.cpp | 14 +++++++++++++-
 mathplot/mathplot.cpp                 | 20 ++++++++++++++++++--
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/MathPlotConfig/MathPlotConfigDemo.cpp b/MathPlotConfig/MathPlotConfigDemo.cpp
index 3c21a1f..66166da 100644
--- a/MathPlotConfig/MathPlotConfigDemo.cpp
+++ b/MathPlotConfig/MathPlotConfigDemo.cpp
@@ -38,6 +38,8 @@

 #include "mathplot.h"

+#include <wx/ffile.h>
+
 // Define a new application type, each program should derive a class from wxApp
 class MyApp: public wxApp
 {
@@ -94,7 +96,8 @@ enum
 // the event tables connect the wxWidgets events with the functions (event
 // handlers) which process them. It can be also done at run-time, but for the
 // simple menu events like this the static method is much simpler.
-wxBEGIN_EVENT_TABLE(MyFrame, wxFrame) EVT_MENU(Demo_Quit, MyFrame::OnQuit)
+wxBEGIN_EVENT_TABLE(MyFrame, wxFrame)
+EVT_MENU(Demo_Quit, MyFrame::OnQuit)
 EVT_MENU(Demo_About, MyFrame::OnAbout)
 wxEND_EVENT_TABLE()

@@ -177,6 +180,15 @@ MyFrame::MyFrame(const wxString &title) :
    wxSizer *sizer = new wxBoxSizer(wxVERTICAL);
    sizer->Add(m_plot, 1, wxALL | wxEXPAND, 5);
    SetSizer(sizer);
+
+   wxLog* logger = new wxLogStream(&std::cout);
+    wxLog::SetActiveTarget(logger);
+
+    FILE *logFile = fopen("log.txt", "w");
+    wxLogChain* logToFile = new wxLogChain(new wxLogStderr(logFile));
+    wxLog::SetActiveTarget(logToFile);
+
+
 }

 void MyFrame::CreatePlot(void)
diff --git a/mathplot/mathplot.cpp b/mathplot/mathplot.cpp
index 207d114..dfef747 100644
--- a/mathplot/mathplot.cpp
+++ b/mathplot/mathplot.cpp
@@ -2754,6 +2754,7 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))
    // J.L.Blanco @ Aug 2007: Added double buffer support
    if (m_enableDoubleBuffer)
    {
+       wxLogMessage("mpWindow::OnPaint paint on the memory DC");
        // Recreate Bitmap if sizes have changed
        if (m_last_lx != m_scrX || m_last_ly != m_scrY)
        {
@@ -2790,6 +2791,7 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))

    // Draw all the layers:
    m_repainting = true;
+   wxLogMessage("mpWindow::OnPaint Paint all****");

    mpFunctionType function;
    mpScaleType scale;
@@ -2809,6 +2811,7 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))
    // If doublebuffer, draw now to the window:
    if (m_enableDoubleBuffer)
    {
+       wxLogMessage("mpWindow::OnPaint memory DC copy to screen");
        dc.Blit(0, 0, m_scrX, m_scrY, trgDc, 0, 0, wxCOPY);
        m_buff_dc->SelectObject(wxNullBitmap);
        delete m_buff_dc;
@@ -3004,7 +3007,7 @@ void mpWindow::UpdateAll()
            }
        }
    }
-
+   wxLogMessage("mpWindow::UpdateAll");
    if (m_magnetize)
      m_magnet.ReInitDrawn();

@@ -4539,9 +4542,10 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
    wxClientDC dc(&w);
    dc.SetPen(*wxBLACK_PEN);
    dc.SetLogicalFunction(wxINVERT);
-
+   wxLogMessage("mpMagnet::Plot enter >>>>>>> ");
    if (m_IsDrawn)
    {
+       wxLogMessage("mpMagnet::Plot draw XOR method, m_Inside(true)");
        dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
        dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
        m_IsDrawn = false;
@@ -4558,6 +4562,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
            }
            else
            {
+               wxLogMessage("mpMagnet::Plot draw COPY method!!!!!!");
                dc.DrawLine(mousePos.x, m_plot_size.y, mousePos.x, m_plot_size.height);
                dc.DrawLine(m_plot_size.x, mousePos.y, m_plot_size.width, mousePos.y);
                dc.SetLogicalFunction(wxCOPY);
@@ -4567,6 +4572,8 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
        }
    }

+   wxLogMessage("mpMagnet::Plot leave <<<<<<<<<< ");
+
    dc.SetLogicalFunction(wxCOPY);
 }

@@ -4574,6 +4581,7 @@ void mpMagnet::ClearPlot(mpWindow &w)
 {
    if (m_IsDrawn || m_IsWasDrawn)
    {
+       wxLogMessage("mpMagnet::ClearPlot, m_InDrawn = true");
        wxClientDC dc(&w);
        dc.SetPen(*wxBLACK_PEN);
        dc.SetLogicalFunction(wxINVERT);
@@ -4583,6 +4591,10 @@ void mpMagnet::ClearPlot(mpWindow &w)
        m_IsWasDrawn = false;
        dc.SetLogicalFunction(wxCOPY);
    }
+   else
+   {
+       wxLogMessage("mpMagnet::ClearPlot, m_IsDrawn = false!!!");
+   }
 }

 void mpMagnet::UpdatePlot(mpWindow &w, const wxPoint &mousePos)
@@ -4592,8 +4604,12 @@ void mpMagnet::UpdatePlot(mpWindow &w, const wxPoint &mousePos)
        // Mouse position change when pan operation
        if (m_rightClick)
            m_mousePosition_old = mousePos;
+
+        wxLogMessage("mpMagnet::UpdatePlot m_IsWasDrawn=true****");
        ClearPlot(w);
    }
+   else
+        wxLogMessage("mpMagnet::UpdatePlot m_IsWasDrawn=false!!!!");
 }

 //-----------------------------------------------------------------------------
-- 
2.37.0.windows.1

[GitHubLionel]

Well, it is a mystery ! You can not have in same time m_IsWasDrawnand m_IsDrawn to true because when m_IsWasDrawn is set to true, in same time m_IsDrawnis set to false in the ReInitDrawn() function.

I push the code with my log function (I simply use a define to have log in console). Then I have this log :

mpMagnet::Plot enter >>>>>>> m_IsDrawn = false
mpMagnet::Plot draw COPY method!!!!!!
mpMagnet::Plot leave <<<<<<<<<<
mpWindow::UpdateAll
mpWindow::OnPaint enter >>>>> paint on the memory DC
mpWindow::OnPaint Paint all****
mpWindow::OnPaint leave <<<<< memory DC copy to screen
mpMagnet::UpdatePlot m_IsWasDrawn = true
mpMagnet::ClearPlot m_IsDrawn = false
mpMagnet::ClearPlot m_IsDrawn = true
mpMagnet::Plot enter >>>>>>> m_IsDrawn = false
mpMagnet::Plot draw COPY method!!!!!!
mpMagnet::Plot leave <<<<<<<<<<
mpMagnet::ClearPlot m_IsDrawn = true

As you can see, the first call of ClearPlot, m_IsDrawnis false.

[asmwarrior]

Sorry, I don't understand the logic of the drawing. I mean: What does the variable m_IsWasDrawn used for?

[GitHubLionel]

When we call UpdateAll (and then OnPaint), you must remember if the cross was drawn : that's the purpose of m_IsWasDrawn. So at the end of OnPaint, we call UpdatePlot to redraw the cross only if m_IsWasDrawn is true.

[asmwarrior]

I add some extra debug info, and see below:

20:53:00: mpWindow::UpdateAll
20:53:00: mpMagnet::ReInitDrawn m_IsDrawn = 1, m_IsWasDrawn = 0
20:53:00: mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 0, m_IsWasDrawn = 1
20:53:00: mpMagnet::Plot enter >>>>>>> 
20:53:00: mpMagnet::Plot draw XOR method, m_Inside(true)
20:53:00: mpMagnet::Plot draw COPY method!!!!!!
20:53:00: mpMagnet::Plot leave <<<<<<<<<< 
20:53:00: mpWindow::OnPaint paint on the memory DC
20:53:00: mpWindow::OnPaint Paint all****
20:53:00: mpWindow::OnPaint memory DC copy to screen
20:53:00: mpMagnet::UpdatePlot m_IsWasDrawn=false!!!!
20:53:00: mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 1, m_IsWasDrawn = 0
20:53:00: mpMagnet::Plot enter >>>>>>> 
20:53:00: mpMagnet::Plot draw COPY method!!!!!!
20:53:00: mpMagnet::Plot leave <<<<<<<<<< 
20:53:00: mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 1, m_IsWasDrawn = 0
20:53:00: mpMagnet::Plot enter >>>>>>> 
20:53:00: mpMagnet::Plot draw COPY method!!!!!!
20:53:00: mpMagnet::Plot leave <<<<<<<<<< 
20:53:00: mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 1, m_IsWasDrawn = 0
20:53:00: mpMagnet::Plot enter >>>>>>> 
20:53:00: mpMagnet::Plot draw COPY method!!!!!!
20:53:00: mpMagnet::Plot leave <<<<<<<<<< 
20:53:00: mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 1, m_IsWasDrawn = 0
20:53:00: mpMagnet::Plot enter >>>>>>> 
20:53:00: mpMagnet::Plot draw COPY method!!!!!!
20:53:00: mpMagnet::Plot leave <<<<<<<<<< 
20:53:00: mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 1, m_IsWasDrawn = 0
20:53:00: mpMagnet::Plot enter >>>>>>> 
20:53:00: mpMagnet::Plot draw COPY method!!!!!!
20:53:00: mpMagnet::Plot leave <<<<<<<<<< 

You can see, when the mpMagnet::ReInitDrawn is called, it input value is m_IsDrawn = 1, m_IsWasDrawn = 0, but later, there is another function call

    if (m_IsDrawn || m_IsWasDrawn)
    {
        wxLogMessage("mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = %d, m_IsWasDrawn = %d", m_IsDrawn, m_IsWasDrawn);
        wxClientDC dc(&w);
        dc.SetPen(*wxBLACK_PEN);
        dc.SetLogicalFunction(wxINVERT);
        dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
        dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
        m_IsDrawn = m_IsWasDrawn;
        m_IsWasDrawn = false;
        dc.SetLogicalFunction(wxCOPY);
    }

So, the m_IsWasDrawn is false now. And after the OnPaint, there is still a function(see log)

mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 1, m_IsWasDrawn = 0

So, the cross line is draw again on the old mouse xy, but now m_IsDrawn becomes false and m_IsWasDrawn is also false.

Which means when we need to draw another cross line, we just draw on the new mouse pos, thus the old xy is shown.

[asmwarrior]

Oh, look at this log:

20:53:00: mpWindow::OnPaint memory DC copy to screen
20:53:00: mpMagnet::UpdatePlot m_IsWasDrawn=false!!!!
20:53:00: mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = 1, m_IsWasDrawn = 0

After the redraw of the window, why do we need to XOR draw on the old saved mouse xy to remove the old cross? I think when the plot get repaint, there is no old cross on the screen, so use XOR to draw is just to draw a new cross, not removing the old cross.

EDIT:

If the m_IsWasDrawn == 1 means there are cross lines before the paint of the window, than after the paint of the window, we don't need the call the "Clear the old plot".

I see in the OnPaint event handler, in the last stage, you call:

void mpMagnet::UpdatePlot(mpWindow &w, const wxPoint &mousePos)
{
    if (m_IsWasDrawn)
    {
        // Mouse position change when pan operation
        if (m_rightClick)
            m_mousePosition_old = mousePos;

        wxLogMessage("mpMagnet::UpdatePlot m_IsWasDrawn=true****");
        ClearPlot(w);
    }
    else
        wxLogMessage("mpMagnet::UpdatePlot m_IsWasDrawn=false!!!!");
}

You see, the m_IsWasDrawn is true here, and you call ClearPlot, but in this function you have the condition:

void mpMagnet::ClearPlot(mpWindow &w)
{
    if (m_IsDrawn || m_IsWasDrawn)
    {
        wxLogMessage("mpMagnet::ClearPlot draw on old mouse xy, m_IsDrawn = %d, m_IsWasDrawn = %d", m_IsDrawn, m_IsWasDrawn);
        wxClientDC dc(&w);
        dc.SetPen(*wxBLACK_PEN);
        dc.SetLogicalFunction(wxINVERT);
        dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
        dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
        m_IsDrawn = m_IsWasDrawn;
        m_IsWasDrawn = false;
        dc.SetLogicalFunction(wxCOPY);
    }
    else
    {
        wxLogMessage("mpMagnet::ClearPlot do nothing, m_IsDrawn = %d, m_IsWasDrawn = %d", m_IsDrawn, m_IsWasDrawn);
    }
}

Which just use the XOR method to draw the cross, which is wrong here!

EDIT2: I think you try to reset the status of the magnet in the UpdateAll() function, you just call ReInitDrawn there, but this does not set the status correctly, because you still need to handle the events before the actual OnPaint event.

[asmwarrior]

I have some the fix on my own branch, see here

https://github.com/asmwarrior/wxMathPlot/commits/test_cross_line

One issue is that I think UpdatePlot should not use wxClientDC, but wxPaintDC.

EDIT: since UpdatePlot is only called in OnPaint event handler, it is no need to ClearPlot the old plot. So, generally we just need to draw the new cross line.

EDIT2: this is fixed in my branch now.

[GitHubLionel]

Well, Lock/unlock/m_IsLock is same as m_IsWasDrawn and I don't see the interest to pass w and dc. Anyway, I clean the code and remove extra lines. For me, I can not see any problems when I pan/zoom/select even with fast move with the mouse (even with sample3 car with timer). Please, use this last code if you have always some troubles. Note : You may try to add this code :

    if (m_IsWasDrawn)
        return;

at the beginning of mpMagnet::Plot method (even if for me m_IsWasDrawn is always false).

[asmwarrior]

Please, use this last code if you have always some troubles.

Hi, I just used the latest code, and I don't see the two crosses issue now, good work! So, I believe the latest commit Correction mpMagnet, you just fixed this issue.

Anyway, I think it is not correct in the below situation:

In the OnPaint event handler, the

    // We redraw the cross if necessary. We pass the mouse position if we do a pan operation.
    if (m_magnetize)
        m_magnet.UpdatePlot(*this, m_mouseRClick);

And in the UpdatePlot function, the DrawCross get called.

void mpMagnet::UpdatePlot(mpWindow &w, const wxPoint &mousePos)
{
    if (m_IsWasDrawn)
    {
        // Mouse position has changed when pan operation
        if (m_rightClick)
            m_mousePosition = mousePos;
        DrawCross(w);
        m_IsDrawn = true;
        m_IsWasDrawn = false;
    }
}

But in DrawCross function call, you used the wxClientDC. For my understanding, when in OnPaint event handler, we should use wxPaintDC, no wxClientDC, at least on Windows.

[GitHubLionel]

Great !! Ok, I replace mpWindow by wxClientDC (wxPaintDC inherit from wxClientDC), so it should be good now.

[asmwarrior]

OK, thanks. Now, I think some member variables' comment of the magnet class should be added.

        bool m_IsDrawn;
        bool m_IsWasDrawn;

I think m_IsDrawn == true means there is a cross line shown on the mpPlot, and if the m_IsDraw is false, no cross lines were drawn on the plot.

m_IsWasDrawn this variable is bit tricky, it is true if mangun was shown before the OnPaint event?

I'm not sure I understand the logic, but I guess the m_IsWasDrawn is still hard to understand.

[GitHubLionel]

Probably because me English is too poor, but for me : m_IsDrawn say : is the cross is drawn ? m_IsWasDrawn say : is the cross was drawn ? It is used at the end of OnPaint event to redraw the cross if he is true.

[asmwarrior]

Probably because me English is too poor, but for me : m_IsDrawn say : is the cross is drawn ? m_IsWasDrawn say : is the cross was drawn ? It is used at the end of OnPaint event to redraw the cross if he is true.

I'm also not an English native speaker, but I think if one looked at a variable name, and does not understand its meaning, it will be difficult to contribute in the future.

So, I think we need to add comments on those variables also if those variable names are not good, they can be renamed. Code::Blocks now has a nice feature (clangd based code completion), which can just let you right click on the variable, and refactoring(rename) the variable.

[asmwarrior]

There are still many "TAB" and "Space" mixed issue in the code, also, there are many trailing spaces in the code. I'm not sure you use/accept the github pull request, I can help to clean it. Also, I think some comments should be added.

Another two things: 1, I think you can put many screen shots of your demo program in the homepage, currently, they are in the sub-folder 2, I'm still wandering why you keep two sources files in the git repo, I believe they are the same file which same contents, do they?

[GitHubLionel]

I keep two sources because the C::B code generated by wxSmith use wxObjectEventFunction for Connect and I have a warning in Eclipse with that. So I replace by wxCommandEventHandler for Eclipse. Note that it will be better to use Bind in place of Connect. For "TAB" and "Space", I use TAB=2*(Spaces length) everywhere but I can use only space if you want. Note that I don't use C::B for coding but Eclipse. I use C::B for wxSmith only to generate the IHM.

[asmwarrior]

I keep two sources because the C::B code generated by wxSmith use wxObjectEventFunction for Connect and I have a warning in Eclipse with that. So I replace by wxCommandEventHandler for Eclipse. Note that it will be better to use Bind in place of Connect.

Oh, sorry, I think you have replied about this before. I think in the git code repo, you can only keep the final source file. I mean the file use have replaced by wxCommandEventHandler. Git can select the changes you want to index/commit, not the final file changes, so this can be done easily.

For "TAB" and "Space", I use TAB=2*(Spaces length) everywhere but I can use only space if you want. Note that I don't use C::B for coding but Eclipse. I use C::B for wxSmith only to generate the IHM.

About this issue, I think I prefer using all Spaces. Because different editors are having different spaces for TAB. So, using space is better, at least from my point.

Thanks.

[GitHubLionel]

Ok, I have changed "TAB" by "SPACE". Note : wxINVERT not work on Linux, so I hide this option in MathPlot config dialog.

[asmwarrior]

I looked at your commit: Change "TAB" by "SPACE"

I see that you have change the wxButton* colourButton to wxButton *colourButton. Is that expected?

For wxSmith generated code or wxWidgets source code, I prefer the wxButton* colourButton kinds of format.

Note : wxINVERT not work on Linux, so I hide this option in MathPlot config dialog.

You mean that magnet only works under Windows system?

[GitHubLionel]

Ok I change pointer * position and I found more tab. Now I change nothing more. And yes, magnet work only on Windows. wxINVERT is a known bug of GTK (and also for Mac).

[asmwarrior]

https://github.com/wxWidgets/wxWidgets/issues/16890

I don't use Linux, but look at this issue is fixed?

[GitHubLionel]

I had seen and read this discussion but it does not provide any solution.

[asmwarrior]

I asked this question in that wx's issue, and I hope some wx dev can help.

[asmwarrior]

wxWidgets/wxWidgets#16890

I don't use Linux, but look at this issue is fixed?

You can see WX developers VZ has commented on that issue. He said on the wayland only WXPaintDC is supported.