Remember debugger size and position

hernanmd commented 2 months ago

Add lastKnownExtent and lastKnownPosition to preserve the last extent and position for the next debug request window. Add a setting to enable this behavior (enabled by default after this PR).

StevenCostiou commented 2 months ago

Thank you Hernan.

However I would not like to do it like that, we spent a lot of time removing similar code from the debugger. It solves the problem (I think) but it only add unnecessary complexity to the debugger, using globals etc.

I would like to find time to discuss the solution, for instance to move all settings of the debugger into a single "debugger setting" object instead of class variables.

Please also come to the debugging team to discuss before implementing without us knowing, because we can share a bit of vision and technical constraints and objectives.

StevenCostiou commented 2 months ago

@adri09070 can you have a look?

hernanmd commented 2 months ago

Thank you Hernan.

However I would not like to do it like that, we spent a lot of time removing similar code from the debugger. It solves the problem (I think) but it only add unnecessary complexity to the debugger, using globals etc.

I would like to find time to discuss the solution, for instance to move all settings of the debugger into a single "debugger setting" object instead of class variables.

Please also come to the debugging team to discuss before implementing without us knowing, because we can share a bit of vision and technical constraints and objectives.

Hi Steven,

I've updated the PR with a cleaner StDebugger "remember position" implementation by moving the settings methods to a new class, StDebuggerSettings. Please let me know of any appropriate enhancements.

hernanmd commented 2 months ago

I've fixed some additional issues, so the CI should pass and can be reviewed "safely"

hernanmd commented 2 months ago

In addition to my comments, we need tests ensuring the new introduced behavior.

Done in the last PR commit.

hernanmd commented 2 months ago

Guys, yesterday this PR was green and today I've seen some conflicts so it would be nice to have it integrated on time before other PR's make it obsolete, otherwise, I have to work on this forever.

Now I'm checking this again because StDebugger initializeWindow: was changed somewhere.

hernanmd commented 2 months ago

The failing test is unrelated.

hernanmd commented 2 months ago

Guys, yesterday this PR was green and today I've seen some conflicts so it would be nice to have it integrated on time before other PR's make it obsolete, otherwise, I have to work on this forever.

Now I'm checking this again because StDebugger initializeWindow: was changed somewhere.

Done. Could you have a look @StevenCostiou ?

StevenCostiou commented 2 months ago

Perhaps tomorrow, I am not available today. But worry not, the debugger code does not change that often, that is just bad luck.

StevenCostiou commented 2 months ago

For me now there are two problems left:

what about those nil checks? can we do something?
tests are all red (I didn't check what's happening) If those are fixed, can we agree to merge?

adri09070 commented 2 months ago

Concerning the failing tests, they are not related

hernanmd commented 2 months ago

For me now there are two problems left:

* what about those nil checks? can we do something?

* tests are all red (I didn't check what's happening)
  If those are fixed, can we agree to merge?

I removed the ifNil: checks since I need to work on other issues.

pharo-spec / NewTools

Remember debugger size and position #795