Sfdk: Set GUI/LastNormalWindowPosition on setVideoMode #550
Conversation
With failure propagation enabled the checkpoint would not be reached in case of failure and so the functor would not be called. It must have been enabled by mistake.
Failure handling is explicit in the fixed part of execution. The optional part, given the use of a checkpoint, is meant to complete despite of errors. Failure propagation must have been enabled by mistake here.
| { | ||
| BatchComposer composer = BatchComposer::createBatch("setLastNormalWindowPosition"); | ||
| fetchExtraData("GUI/LastNormalWindowPosition", Sdk::instance(), [=, batch = composer.batch()](QString data, bool ok) { | ||
| if (!ok) |
There was a problem hiding this comment.
Let's fix error reporting now. We need
*allOk &= ok;
prior to this line and then { *allOk = false; return; } instead of just return in assertions.
There was a problem hiding this comment.
That was left out on purpose. Even if this part of setVideoMode fails, the others might be enough, depending on VirtualBox version.
There was a problem hiding this comment.
Ah, I guess you assume it would cancel the other parths. That's not the case here. It would just indicate that not all went as planned to the user. So I would prefer to be very specific about the two scenarios we expect here, branch for old and new version of vbox we currently know about and be verbose in other cases.
Note: When I tried with vbox 6.1.36r152435, the GUI/LastNormalWindowPosition property is set for my emulators, so this should not fail normally. Also when I tried with nonexisting property, it still returned zero, just the message is "No value set!". Older emulators will get reinstalled this time, because we are touching the emulator package, so our emulators should have that property set after upgrade.
There was a problem hiding this comment.
Ah, I guess you assume it would cancel the other parths. That's not the case here. It would just indicate that not all went as planned to the user. So I would prefer to be very specific about the two scenarios we expect here, branch for old and new version of vbox we currently know about and be verbose in other cases.
You are again right. I wonder how I could think that, when the code is right in front of me?
Note: When I tried with vbox 6.1.36r152435, the GUI/LastNormalWindowPosition property is set for my emulators, so this should not fail normally. Also when I tried with nonexisting property, it still returned zero, just the message is "No value set!". Older emulators will get reinstalled this time, because we are touching the emulator package, so our emulators should have that property set after upgrade.
Thanks for testing!
vigejolla
force-pushed
the
jb60004
branch
2 times, most recently
from
155ab4e
to
a0a289c
Compare
VirtualBox 7.0 no longer uses GUI/LastGuestSizeHint, so we need to set both in order to make sure window size is correct
VirtualBox 7.0 no longer uses GUI/LastGuestSizeHint, so we need to set both in order to make sure window size is correct