Fix unnecessary Center PC calls in the CodeWindow

[aldelaro5]

This not only fixes a regression where toggling a breakpoint using the CodeWindow would cause a Center PC, but it also removes several redundant JumpToAddress(PC) calls.

The regression was due to an improper fix I did in pr #4218 and was undone in the Hi-dpi pr. Not only it didn't made sense to have the center pc in the repopulate function, but there was other ways to cause a center for the events that desires one.

---

This change is

[sepalani]

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 391 at r1 (raw file):

    {
      wxCommandEvent ev(wxEVT_HOST_COMMAND, IDM_UPDATE_DISASM_DIALOG);
      GetEventHandler()->ProcessEvent(ev);

If you don't need the event to be processed right now, you might prefer wxPostEvent to fire the event instead.

---

Comments from Reviewable

[sepalani]

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 389 at r1 (raw file):

    CPU::PauseAndLock(false, false);

    {

Are braces necessary here?

---

Comments from Reviewable

[sepalani]

Otherwise, LGTM.

[aldelaro5]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 389 at r1 (raw file):

Previously, sepalani wrote…

Are braces necessary here?

Honestlu, no, I thought there was a reason as it was just a call to update diasm dialog before, but this changed in the hidpi pr for unknown reasons to me.

I removed them now.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 391 at r1 (raw file):

Previously, sepalani wrote…

If you don't need the event to be processed right now, you might prefer wxPostEvent to fire the event instead.

I actually want the event to be processed right now because if I could, I wouldn't do an event at all, I really want this update to happen to avoid weird things to happen.

---

Comments from Reviewable

[sepalani]

---

Comments from Reviewable

[BhaaLseN]

I suspect those braces were for scope limiting, to avoid ev living too long there. But since that method is over like a call later, it doesn't really matter.

Can't speak much for the correctness of the rest of the code, but style-wise LGTM.

[aldelaro5]

@lioncash and @leoetlino anything else to review here? Would really like this regression fix to be merged.

@phire , @EmptyChaos ?

[EmptyChaos]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

Source/Core/DolphinWX/FrameTools.cpp, line 528 at r2 (raw file):

      CPU::EnableStepping(!CPU::IsStepping());

      wxThread::Sleep(20);

This sleep line should be removed. It's legacy code from when EnableStepping used to be broken before I rewrote the CPU module.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 613 at r2 (raw file):

    return;

  codeview->Center(PC);

I'm pretty sure the codeview->Refresh() is needed here for symbol map and breakpoint updates. You can add a defaulted boolean control parameter to skip refreshing from IDM_UPDATE_DISASM_DIALOG.

---

Comments from Reviewable

[aldelaro5]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

Source/Core/DolphinWX/FrameTools.cpp, line 528 at r2 (raw file):

Previously, EmptyChaos wrote…

This sleep line should be removed. It's legacy code from when EnableStepping used to be broken before I rewrote the CPU module.

Done.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 613 at r2 (raw file):

Previously, EmptyChaos wrote…

I'm pretty sure the codeview->Refresh() is needed here for symbol map and breakpoint updates. You can add a defaulted boolean control parameter to skip refreshing from IDM_UPDATE_DISASM_DIALOG.

Just so that I don;t go overly complex like last time where I passed an int via the event, do you have a suggestion for a simpler way to do that? I agree that it would be ideal to do since I also noticed that the onPLay refresh is needed, but it was redundant with another center pc.....so yeah this solution seems the best way to handlke it, but I jsut need a suggestion to know how to do it right.

---

Comments from Reviewable

[EmptyChaos]

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 613 at r2 (raw file):

Previously, aldelaro5 (aldelaro5) wrote…

Just so that I don;t go overly complex like last time where I passed an int via the event, do you have a suggestion for a simpler way to do that? I agree that it would be ideal to do since I also noticed that the onPLay refresh is needed, but it was redundant with another center pc.....so yeah this solution seems the best way to handlke it, but I jsut need a suggestion to know how to do it right.

In the class definition:

class CCodeWindow
{
public:
   // ...
   void Repopulate(bool refresh_codeview = true);
   // ...
}

In the cpp:

void CCodeWindow::Repopulate(bool refresh_codeview)
{
  if (refresh_codeview)
    codeview->Refresh();
  UpdateCallstack();
  UpdateButtonStates();
}

Then in CCodeWindow::OnHostMessage

case IDM_UPDATE_DISASM_DIALOG:
  codeview->Center(PC);
  Repopulate(false);
  // ...

This prevents the double refresh while still refreshing as needed in every other case.

---

Comments from Reviewable

[aldelaro5]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 613 at r2 (raw file):

Previously, EmptyChaos wrote…

In the class definition:

class CCodeWindow
{
public:
   // ...
   void Repopulate(bool refresh_codeview = true);
   // ...
}

In the cpp:

void CCodeWindow::Repopulate(bool refresh_codeview)
{
  if (refresh_codeview)
    codeview->Refresh();
  UpdateCallstack();
  UpdateButtonStates();
}

Then in CCodeWindow::OnHostMessage

case IDM_UPDATE_DISASM_DIALOG:
  codeview->Center(PC);
  Repopulate(false);
  // ...

This prevents the double refresh while still refreshing as needed in every other case.

Done. Didn;t see it that way :p

---

Comments from Reviewable

[EmptyChaos]

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/DolphinWX/FrameTools.cpp, line 530 at r3 (raw file):

      g_pCodeWindow->Repopulate();
      // Update toolbar with Play/Pause status
      UpdateGUI();

Actually, looking at this again, the UpdateGUI and g_pCodeWindow->Repopulate need to be inside an if block.

bool was_stopped = CPU::IsStepping();
CPU::EnableStepping(!was_stopped);
// When the CPU stops it generates a IDM_UPDATE_DISASM_DIALOG which automatically refreshes the UI,
// the UI only needs to be refreshed manually when unpausing.
if (was_stopped)
{
  g_pCodeWindow->Repopulate();
  UpdateGUI();
}

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 210 at r4 (raw file):

    PC += 4;
    codeview->Center(PC);
    Repopulate();

Repopulate(false);

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 216 at r4 (raw file):

    PC = codeview->GetSelection();
    codeview->Center(PC);
    Repopulate();

Repopulate(false);

---

Comments from Reviewable

[aldelaro5]

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/DolphinWX/FrameTools.cpp, line 530 at r3 (raw file):

Previously, EmptyChaos wrote…

Actually, looking at this again, the UpdateGUI and g_pCodeWindow->Repopulate need to be inside an if block.

bool was_stopped = CPU::IsStepping();
CPU::EnableStepping(!was_stopped);
// When the CPU stops it generates a IDM_UPDATE_DISASM_DIALOG which automatically refreshes the UI,
// the UI only needs to be refreshed manually when unpausing.
if (was_stopped)
{
  g_pCodeWindow->Repopulate();
  UpdateGUI();
}

Done.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 210 at r4 (raw file):

Previously, EmptyChaos wrote…

Repopulate(false);

Done.

---

Source/Core/DolphinWX/Debugger/CodeWindow.cpp, line 216 at r4 (raw file):

Previously, EmptyChaos wrote…

Repopulate(false);

Done.

---

Comments from Reviewable

[EmptyChaos]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Source/Core/DolphinWX/FrameTools.cpp, line 528 at r5 (raw file):

      CPU::EnableStepping(!CPU::IsStepping());

      g_pCodeWindow->JumpToAddress(PC);

The lines above need to be removed. They're redundant with the rest of the code block, you're pausing then unpausing immediately because of the dual EnableStepping calls.

---

Comments from Reviewable

[aldelaro5]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Source/Core/DolphinWX/FrameTools.cpp, line 528 at r5 (raw file):

Previously, EmptyChaos wrote…

The lines above need to be removed. They're redundant with the rest of the code block, you're pausing then unpausing immediately because of the dual EnableStepping calls.

Done. Oops, shoudl really pay attnetion when pasting....

---

Comments from Reviewable

[EmptyChaos]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Source/Core/DolphinWX/FrameTools.cpp, line 528 at r5 (raw file):

Previously, aldelaro5 (aldelaro5) wrote…

Done. Oops, shoudl really pay attnetion when pasting....

The `JumpToAddress` should not be needed either, `CCodeWindow` will do that itself in `OnHostMessage`.

---

Comments from Reviewable

[aldelaro5]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Source/Core/DolphinWX/FrameTools.cpp, line 528 at r5 (raw file):

Previously, EmptyChaos wrote…

The JumpToAddress should not be needed either, CCodeWindow will do that itself in OnHostMessage.

Done. oh, I was mixing this line with another thing I got, but yeah it;s nto needed now.

---

Comments from Reviewable

[EmptyChaos]

---

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable