Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update insert blank disk handling and improve invalid disk handling for saving #251

Closed
wants to merge 7 commits into from

Conversation

cormacj
Copy link
Contributor

@cormacj cormacj commented Feb 10, 2025

This change adds a Format button to the insert blank disk dialog in options.

image

Once the format button is clicked, it calls dsk_format for the appropriate disk with the select disk format.

This initialises the disk structure properly ensuring that later save disk works as expected.

A second part of this file is to improve error handling for trying to save when no disk was inserted.

If Save is called and the disk tracks are zero, no file will be written and an error message modal is displayed.

image

This should resolve #250

@ColinPitrat
Copy link
Owner

Indeed, it looks like a functionality that was never fully implemented :-) drvA_format and drvB_format are not used anywhere at the moment. The way I personally handle this is that I have an empty.dsk file that I load when I need it. But I agree the feature would be nice to have.

I have some concerns with this PR though. I'll need to think a bit more about it, but here are my current thoughts:

Concern 1

It's a bit weird to have the "Format" button in the options. The options menu is meant to configure Caprice, not to do actions. On the other hand, I don't have a much better place to put it in the menu.

It would also be weird in Load/Save as this is not really about loading/saving a file but about creating an emtpy disk that doesn't exist yet as a file. However, I think I'd still prefer it at this place.

Having a new dedicated menu seems overkill. We could also have tabs in Load/Save (and potentially rename it) to make it less confusing.

Another alternative would be to call dsk_format at startup, but it would mean it's not possible to have a CPC without a disk inserted. Not a big deal for most users, but still annoying for anybody who would want to test the case where no disk is present for some reason (I know I had to do that in the past).

Finally, we could introduce it as keyboard shortcuts only but it's not ideal either.

Concern 2

The name of the button, "Format", is a bit weird when just above, we have a text saying "Insert blank disks as". We may want to reword one of them, either make the button "Insert" or make the text "Format disks as".

I personally prefer the former (renaming the button to "Insert") because that's what we really want this button to do: to replace the disk in the drive. This naturally leads to the next concern:

Concern 3

When clicking "Insert", we should get an error message if there's already a disk in the drive. We may want to add a button "Eject" to allow removing the disk. But then we don't want to eject a disk if it's not been saved without warning the user.

But then the user has to exit the options, go to Load/Save, save and then come back to the options. That's very weird (although probably not something that will happen very frequently) and suggest this whole Disk tab may be better placed in Load/Save. Or alternatively the whole Load/Save menu could be moved to this Disk tab!

But the more I think about it, the less it makes sense to me to have the Disk tab in Options. None of this is saved in cap32.cfg. Currently, these formats are not used at all but if we were to use them, it makes sense to decide on it when doing the action, not when configuring Caprice.

@ColinPitrat
Copy link
Owner

Note: if you can split this PR in 2 (extracting everything except CapriceOptions.h and CapriceOptions.cpp in a different PR) I can merge the rest as we think more about the correct change for the insertion of an empty disk.

@cormacj
Copy link
Contributor Author

cormacj commented Feb 11, 2025

I agree with Concern 1. Perhaps change the Load/Save button to being renamed as Disks. It would then make more sense to change the Load/Save drop down and make the options "Insert Blank", "Load" and "Save"

An "Eject" option could also be added into this dropdown as I don't really see any way to eject a disk in the user interface.

ColinPitrat added a commit that referenced this pull request Feb 17, 2025
@ColinPitrat
Copy link
Owner

I managed to mess your PR while trying to separate the error messages from the UI changes, sorry about that.

In the end, I copied your change for the error messages in 595eb83

For the UI changes, I'd like to go with moving the "new disk" to the Load/Save menu, which may need some renaming, I'll try a few things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not create and load blank disk with Caprice32 4.6.0
2 participants