update disasm window when break state changes

[pvmm]

This should fix issue #21. Not with a refresh button, but actually updating the disasm window after each debug step.

[MBilderbeek]

Can you explain why this is the right fix?
The updateData method is only called after making the connection or getting into the breaked state. Why is that the right moment to trigger an update of the disassembly view? I may be missing something, but this doesn't seem to be the moment where 'slot changes or code is rewritten'.

[pvmm]

I just added an update to the disasm widget on the same place where all the other widgets get updated (slot, registers and main memory). The only difference is that DisasmViewer lacks an explicit refresh() function. But the description is about the desired outcome.

[pvmm]

I may be missing something, but this doesn't seem to be the moment where 'slot changes or code is rewritten'.

That is because the debugger lacks the functionality to detect such changes.

[MBilderbeek]

That is because the debugger lacks the functionality to detect such changes.

Yes, so, why would it help to trigger this on the 'breaked' state? It means you get a refresh if you toggle this state right? Is that good enough?

[pvmm]

We are already counting on the emulator to do most of the grunt work anyway. I can create a signal/slot structure to detect slot changes, but tell me how you would expect the debugger to detect self modifying code?

[m9710797]

@MBilderbeek: I don't claim to understand how the debugger update stuff works. But I propose to be pragmatic about this:

• If it works (it fixes the update problem)
• and if it's similar to how the other debugger code is written (pvmm claims it is, i haven't checked)

Then I propose we already merge the PR. And if needed we can continue to discuss about a better way to fix it.

[MBilderbeek]

To be completely clear, the title of this PR is "update disasm window when slot changes or code is rewritten". But I think what is implemented is: "update disasm window when break state changes". Am I correct, or did I misunderstand somewhere?
And, as after each debug step, the break state changes, after each step the disasm view is updated. Correct?

[MBilderbeek]

If I'm correct, I agree with this change, but I'd make it a bit different:

• in DisasmViewer rename the symbolsChanged method into 'refresh' and connect the symbolsChanged signal to this refresh method.
• In DebugForm, instead of emitting symbolsChanged, just call the refresh method in DisasmViewer in updateView.

[pvmm]

While this is a more consistent approach regarding the rest of the widgets code, I think we should loosen the coupling by using more signals/slots rather than less. So maybe the other widgets should follow suit and adopt slots too? What do you think?

[MBilderbeek]

While this is a more consistent approach regarding the rest of the widgets code, I think we should loosen the coupling by using more signals/slots rather than less. So maybe the other widgets should follow suit and adopt slots too? What do you think?

In principle, yes. But I just did an attempt (e.g. emit BreakStateChanged and let the stuff in updateData be handled by processing this signals) but I run into some inconsistencies... e.g. there is an updateData after finalizing the connection... why? Can I leave out the stuff in updateData for that case, which updates the slot, disasm and mainMemoryView?
What is that 'emulationChanged" signal for exactly? It is only emitted exactly in updateData! Can we replace it with the new breakStateEntered signal? (Which also means it doesn't get emitted when finalizing the connection.)

The attached patch tries to clean this up first a bit, assuming things are good enough like this.... please review and comment.

After this patch is applied, adding that update of disasm when break state changes is a matter of adding one connect line.
more_signals_slot.patch.txt

[pvmm]

I think the function names are misleading. finalizeConnection is not called when the user disconnects, but when the debugger finishes establishing connection. So that the debugger widgets are automatically updated upon connection which makes sense. Also, there is a mess in this piece of code:

    if (halted) {
        setBreakMode();
        breakOccured();
    } else {
        setRunMode();
        updateData();
    }

if emulator is halted, setBreakMode() will update the debugger interface, but then, breakOccured() will update it again, and it also calls updateData(), so calling breakOccured here is redundant. This piece of code should be:

    if (halted) {
        setBreakMode();
    } else {
        setRunMode();
    }
    updateData();

[MBilderbeek]

Why always call updateData? breakOccured already calls updateData() as well.
What do you think of the other changes I made?

Here's a an updated patch with your first remark processed.

Can you also help testing with this patch?
more_signals_slot.patch.txt

[pvmm]

OK, I made your patch work by repurposing my previous fix within your patch (and I squashed my previous commit, because it was no longer relevant). It's not extensively tested, but I think it has improved the situation of the bitmap VRAM dialog too from what I've checked.

I also looked at DebuggerForm::handleUpdate() and reused the same idea in DebuggerForm::finalizeConnection(), so my previous suggestion was modified, but it is basically the same concept.

[pvmm]

My last commit allows BitmappedView and TileView to reload VRAM data even if connection was established after their dialogs were created.

[MBilderbeek]

OK, I made your patch work by repurposing my previous fix within your patch (and I squashed my previous commit, because it

What exactly was not working in my patch?
Note that I explicitly did not include yet the main change: to refresh the disasm view. That was for a next commit.

EDIT: ah, I did make a mistake: I removed the wrong line in the finalizeConnection method. I should have removed setBreakMode instead of breakOccured... sorry for that.

was no longer relevant). It's not extensively tested, but I think it has improved the situation of the bitmap VRAM dialog too from what I've checked.

I would like to change one thing at a time please.... I would not like to discuss other changes separately, otherwise I get completely confused mixing up all these changes.

I also looked at DebuggerForm::handleUpdate() and reused the same idea in DebuggerForm::finalizeConnection(), so my previous suggestion was modified, but it is basically the same concept.

I wonder if it is correct to NOT call updateData() when after connection halted is false.

Please, let's do one thing at a time. Let me know if my last patch (as attached) doesn't break anything. If it's OK, then I'll add the new connect to update the disasm view. And after that, let's see about changes of the VRAM viewers. OK?

[pvmm]

What exactly was not working in my patch?

The removed line:

 	mergeBreakpoints = true;
 	if (halted) {
 		setBreakMode();
-		breakOccured();
 	} else {
 		setRunMode();
 		updateData();

I changed it to:

 	mergeBreakpoints = true;
 	if (halted) {
		breakOccured();
 	} else {
 		setRunMode();
 		updateData();

Since we want the update to occur when connection is established to a halted emulator. The rest of my commit is basically the same.

EDIT: ah, I did make a mistake: I removed the wrong line in the finalizeConnection method. I should have removed setBreakMode instead of breakOccured... sorry for that.

Yes, that was the problem.

[MBilderbeek]

OK, I cleaned up my commit and pushed it. It also needed to do an initial refresh of 2 parts (slot viewer and main memory viewer).

Now we have a base to continue. Please review my patch though.

[pvmm]

I wonder if this is necessary though:

 void SlotViewer::setMemoryLayout(MemoryLayout* ml)
 {
 	memLayout = ml;
+	update();
 }

SlotViewer should be updated by refresh, like the other widgets.

EDIT: I saw that you did not commit this line.

[MBilderbeek]

I removed that in the final patch. It was a mistake indeed. Op di 22 feb. 2022 00:10 schreef Pedro de Medeiros ***@***.***

…

: I wonder if this is necessary: void SlotViewer::setMemoryLayout(MemoryLayout* ml) { memLayout = ml; + update(); } SlotViewer should be updated by refresh, like the other widgets. — Reply to this email directly, view it on GitHub <#78 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADAWXCS6VWE4NLCQ4274BKLU4LA6TANCNFSM5OWIIL6Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>