New Menu Handling

[ChetSimpson]

Over the last few days I've managed to make a fair amount of progress on getting the cartridge interfaces moved along including updating how the cartridge menu is handled. The cartridges no longer need to build the menu when they start up or rebuild the menu any time a property of the cartridge changes. Instead, when the cartridge menu is about to be opened the main application will fetch a list of menu items from the attached cartridge. This changes how the menu id's are handled, before the cartridge had to have knowledge of the id range (5000-5099) now their id's start at 0. This also adds separators in sub menus and by the time the release is complete checkboxes and disabled states.

I wasn't going to replace the menu system (CartridgeMenu is completely gone) but having it done makes separating the UI and device implementations of a cartridge easier. I've also been working on updating the menu to select the cartridge there instead of by file and it the old system kept breaking and getting in my way.

Here's a screenshot that shows the submenu separators along with the list of cartridges to select from.

[ejaquay]

The MPI interface is pretty good the way it is except right now it can't even load a rom cartridge without crashing VCC. My fault for not paying attention. I thought pulling a couple PR's that were just adding doxygen comments would be harmless. I spent a lot of time last summer getting the interface and mpi configuration to work properly and not crash vcc. Now it does not work.

Too many PR's that do not fix listed issues and that are not even being tested properly. There is going to much more scrutiny on PR's and what changes are being made in the future. Sorry for the rant but I am sort of pissed.

The MPI menu shown above might seem easier to use but it is probably not. Visibility and control over what slots cart are in is just as important as what is being loaded. Unless we create a cartridge metadata system (like mame) we will still need to let users select what cart to load by browsing files in a directory and forcing those file to exist in any particular directory is a bit of a non-starter.

[ChetSimpson]

Yes, bugs will happen from time to time, especially when you're making improvements to terrible code. You have code that's so fragile you're afraid to even touch it. You can't make any enhancements of any substance without breaking half a dozen things along the way and having have a dozen other pieces of poor code pop up that you have to fix, enhance, or update in some way just to move forward with your own changes. I know this because every time I've tried to add something even minimally substantial the existing code base is continuously getting in my way - that's the core motivation for my changes. Even if I did manage to get something of substance in the value of those enhancements would be immediately diminished because of the quality of the code surrounding it. So, If I make a few or even a fair number of bugs while improving a garbage code base I'm ok with that because I know that at the end of the day the entire program is going to be better off. Next time instead of getting pissed about it just give people a heads up and ask if they can take a look, it's a lot more productive and the problem usually gets resolved a lot quicker.

Wut? Are you saying it's so much harder to choose a cart DLL from an automatically populated menu instead of navigating away from my ROM directory, selecting the DLL, then when I want to insert a ROM navigating back over to my ROM directory to select it. You have a limited number of DLL carts, each that provides information including the name and catalog id which can be used in the menu - or just add a menu_name property that's not part of the instance. Why should they have to navigate anywhere just to select a cart DLL that VCC already knows about? Why should they even know the detail of the cart's being DLL's unless they're installing one from a third party which I do not see happening anytime soon.

As for this particular enhancement there are other changes that have already been made to support it. The cartridge DLL's are now placed in a cartridges subdirectory in VCC proper and the core ROMs (coco3, orch90, etc) are in a ROM subdirectory. The user should never have to know where they are and if they do it's because they are installing something from an external source and you expect them to have a greater idea of what they're doing such that a simple document is all that's needed.

If having a bug in the MPI code bothers you, you're likely going to have issue with a lot of the other changes that are coming. I'm making substantial changes across all cartridges including stabilization of interfaces, consolidation of common code (i.e. disk images), and separating the UI elements from the device logic. Below is a list of changes that I have either already finished or will be working on soon.

1. [DONE] Changing the build output structure so that all executable files are placed in the same directory or one of its subdirectories. Place the cartridges and system roms in their own directories (cartridges and ROMS respectively).
2. [DONE] Enable multiprocessor compilation.
3. [DONE] Clean up orch90 cartridge and update to use common ROM image components
4. [DONE] Clean up GMC cartridge and move the sn76496 implementation to the emulation library.
5. [DONE] Clean up Becker Port cartridge.
6. [DONE] Clean up RamDisk cartridge.
7. [DONE] Update HardDisk to wrap the implementation in a cartridge.
8. [DONE] Update SuperIDE to wrap the implementation in a cartridge.
9. [DONE] Update SDC cartridge to wrap the implementation in a cartridge.
10. [DONE] Update ACIA cartridge to wrap the implementation in a cartridge.
11. [DONE] Update FD502 cartridge to wrap the implementation in a cartridge. This is a precursor to the full FD502 replacement and needed to remove the C api.
12. [DONE] Update project settings and make cartridges a dependency of the main application.
13. [DONE] Remove procedural interface for cartridges.
14. [DONE] Split cartridge_context to expansion_port_host, expansion_port_ui, and expansion_port_bus.
15. [DONE] Align documentation in libcommon with previous changes
16. [DONE] Move Interrupt and InterruptSource back into vcc proper and replace with bus_interrupt for only irq, cart, and nmi (remove firq). Scrap using InterruptSource.
17. [DONE] Replace menu handling (i.e. CartridgeMenu) for cartridges.
18. Add cartridge_controller for menu and status functionality and update cartridges to return an instance when cartridge is created.
19. Move cartridge_slot to vcc/bus as expansion_slot.
20. Add unit tests for cartridge and related components.
21. Formalize device interface for items under ::vcc::devices
22. Add common component for DMK disk image support.
23. [ON-HOLD] 70% - Refactor FD502 cartridge primarily to replace much of the WD1793 emulation with new device code and remove use of C string based API's currently in libcommon. This will result in common components for disk image access and disk drive emulation. This is on hold while I situate the libraries.
24. Refactor HardDisk cartridge to full implementation of cartridge interface, leverage common disk image components, and remove use of C string based API's currently in libcommon.
25. Refactor SuperIDE cartridge to full implementation of cartridge interface, leverage common ROM and disk image components, and remove use of C string based API's currently in libcommon.
26. Refactor SDC cartridge to implement the new cartridge interface and leverage common wd1793 and ROM and disk image components
27. Refactor ACIA cartridge to full implementation of cartridge interface and leverage common ROM and disk image components.
28. Refactor connectivity in Becker Port device (likely with with boost::asio) and craft a common serial device interface.
29. Remove cartridge.reset being called right after cartridge.start
30. Create common serial interfaces and connectivity (likely with with boost::asio) and refactor both ACIA and Becker cartridges to use it.
31. Removing remaining use of C string based API's currently in libcommon by cartridges.
32. Move C string apis from libcommon back into the main VCC app once the dependency has been broken in all cartridges.
33. Replacing the insert cartridge functionality in the main VCC app and MPI cartridge to show a list of DLL based cartridges (in the cartridges directory) by name.
34. Overhaul cartridge and cartridge related UX to provide overall better configuration experience.
35. Address remaining todo's and fixme's in libcommon.

I also have a supplemental list of smaller items I've noticed along the way and other things that I'm considering.

[ChetSimpson]

The MPI interface is pretty good the way it is except right now it can't even load a rom cartridge without crashing VCC. My fault for not paying attention. I thought pulling a couple PR's that were just adding doxygen comments would be harmless. I spent a lot of time last summer getting the interface and mpi configuration to work properly and not crash vcc. Now it does not work.

There's a fix up for it. Took about 12 seconds to launch VCC in a debugger and find the problem and less than that to actually fix it. The problem had nothing to do with the MPI itself, the commit you referenced did have a couple of code changes which makes the bug super easy to identify even if you just want to eyeball it, and the actual problem is pretty obvious. It took less time for me to wrap it up than it did for you to write the up the issue and then hijack my post with your rant. As far as I can tell there's nothing at all about this problem to get all twisted about and certainly nothing to warrant the negativity so I'm not going to bother with it beyond this.

[ejaquay]

I found the bug by finding the commit that caused it. After that simple inspection shows code changes in a couple of PRs that claimed to be just be adding comments. My bad for not doing that inspection and rejecting at the time. I am not perfect, specially when I am less than a month from a major surgery. The fact is simple unit testing on your part would have found the bug and should have before the PR was even issued. Nor should a PR be doing things not mentioned in the commit comments.

The following sums up a major problem: "If I make a few or even a fair number of bugs while improving a garbage code base I'm ok with that because I know that at the end of the day the entire program is going to be better off."

We simply can not work that way. There are issues besides "the code is garbage" to fix (There is a list that will be growing as we test and from user experiance) and we can't if you are creating new issues while radically changing the code. To fix the current issues the rest of us have to spend time understanding, agreeing to, and working with the changes you have done and are planning to do and that is not going to happen overnight.

A better process would be to enumerate specific things you see wrong with enough detail that we can consider and then collectively work on them. "Garbage" maybe desciptive but is not nearly specific enough for consideration.

Now to go down the list you supplied.

1. Changing build output locations. I want them the way they are except for a couple of debug builds going to the wrong place and if I decide to fix them I will let everybody know in advance. You are free to write your own private scripts to copy outputs from where they are to where you want.
2. Multiprocessor compiles OKAY.
3. Clean up orch90. Why? "Shitty" code is not a valid reason. Need specifics.
4. Clean up GMC. Why? Is it broken?
5. Clean up Becker. Same
6. Clean up RamDisk. Same
7. Update HardDisk. Works now.
8. Update SuperIDE. Works now.
9. Update SDC. Fixes only. I am aworking on some.
10. Update ACIA. Maybe use libcommon for network
11. Update FD502. Works now.
12. Make cartridges a dependancy of VCC. Need specifics.
13. Remove procedural interface for carts. Why? Rom carts are dumb. Interface is minimal.
14. Fix documentation. OKAY.
15. Move interrupts back into vcc. I would trust Craig to do that after he fixes robocop.
16. Replace menu handling. The long term goal of menu handling changes should be towards cross platform. Changes must be pre-approved with mockups.
17. Move cartridge slot to vcc/bus Why? Again "Shitty" code is not a good reason.
18. Unit tests. OKAY as long as they require no code changes.
19. Formalize device interface. Document first.
20. Common component for DMK disk image. NO. Not common. FD502 only.
21. Refactor FD502 to not use capi. Not now.
22. Refactor HardDisk, SuperIDE, or SDC. Not now.
23. Refactor Becker Port with boost. NO Vcc builds and works without including 3rd party libraries. (legacy exception for opengl but legacy will go away in time)
24. Craft common serial interface. OKAY.
25. Cartridge reset timing. Okay if that addresses a specific issue.
26. Move C string based API's out of libcommon. OKAY if done carefully.
27. Changes to MPI config menu. Subject to preapproval.
28. Fixing to TODOs and FIXME. Okay if they are causing specific issues.

[ejaquay]

Are you saying it's so much harder to choose a cart DLL from an automatically populated menu instead of navigating away from my ROM directory, selecting the DLL, then when I want to insert a ROM navigating back over to my ROM directory to select it.

I have been thinking about adding that problem as an issue. It would be nice if the ROM directory stored in the config was different from the DLL directory. That will require changing the MPI config to allow user to choose which is being loaded while presenting the directory or list. For testing purposes there is still a need to have a directory other than vcc's .exe directory for the DLL's

[ChetSimpson]

I found the bug by finding the commit that caused it. After that simple inspection shows code changes in a couple of PRs that claimed to be just be adding comments. My bad for not doing that inspection and rejecting at the time. I am not perfect, specially when I am less than a month from a major surgery. The fact is simple unit testing on your part would have found the bug and should have before the PR was even issued. Nor should a PR be doing things not mentioned in the commit comments.

Ok, so you want to make a big deal out of something that's not and then continue to nitpick about it. K.

The following sums up a major problem: "If I make a few or even a fair number of bugs while improving a garbage code base I'm ok with that because I know that at the end of the day the entire program is going to be better off."

We simply can not work that way. There are issues besides "the code is garbage" to fix (There is a list that will be growing as we test and from user experiance) and we can't if you are creating new issues while radically changing the code. To fix the current issues the rest of us have to spend time understanding, agreeing to, and working with the changes you have done and are planning to do and that is not going to happen overnight.

I think there's some confusion here. The PR's I've submitted were done as a courtesy to aid in improving your overall codebase. Since you were initially receptive I continued to submit them until you unceremoniously "locked" everything down and only accepted bug fixes. While I have no problems fixing any issues you might find, my focus, priorities, and schedule haven't changed.

A better process would be to enumerate specific things you see wrong with enough detail that we can consider and then collectively work on them. "Garbage" maybe desciptive but is not nearly specific enough for consideration.

The FD502 has several parts to it. The UI, the FD502 logic, the WD1793 logic, the disk drive logic (for index hole timing), and the virtual disk image. The problem is that it's all mixed together - all parts are tightly coupled with at least one other part, there's no cohesion, there are numerous code complexity issues, it leaks memory, has potential buffer overruns, the code is far from testable, and fair amount of just general bad coding practices including these absolutely useless micro-optimizations - if ( (Side==1) & (CurrentDisk==NONE) ). Then there's the code in the FD502 that's duplicated in other cartridges and the details that are leaked everywhere, and JFC the menuing which is ridiculously dumb. Now extrapolate that or whatever to the other cartridges except maybe acia which is ok and you're covered. These problems have nothing to do with the language it was written in, the paradigm that was followed, or when it was written - it's just garbage. I haven't gone over VCC proper in detail yet but I don't expect my opinion to change much if at all when I do. If you don't happen to see those things maybe have a few conversations with Copilot while it reviews the code.

Now to go down the list you supplied.

Again, I think there's some confusion here. I wasn't soliciting your approval for any of changes that I've made or will be making. Just a courtesy heads up so you know what work is going on over on my side and can decide whether you want to tag along. The items that are marked done are all merged into my develop branch. Several small pieces of functionality are disabled or non-working since they're part of code that's about to get nuked so be aware of that if you ever pull anything.