joyfullservice / msaccess-vcs-addin

Synchronize your Access Forms, Macros, Modules, Queries, Reports, and more with a version control system.
Other
203 stars 40 forks source link

Error Breaking #311

Closed joshrosario310 closed 2 years ago

joshrosario310 commented 2 years ago

Just want to start by saying how much I love this tool and how I just know it's going to change my life for the better, thank you very much!

I just fought with the add-in for several hours which was breaking for an error, I believe it was some form that didn't have any VBA. I noticed there were some On Error Resume Next lines nearby and finally realized I had inadvertently set my global VBA Editor to Break on All Errors. As soon as I set that back to Unhandled Errors, the add-in went back to exporting properly.

I don't know if it's possible to detect the User's Error Trapping setting before running the Export script but maybe it might save someone else some time down the road? I for sure will be looking there before any export operation going forward.

Unfortunately, it wasn't saving the log file after breaking but I think it was GetCodeModuleHash() in modHash that was throwing errors on exporting several specific forms.

hecon5 commented 2 years ago

Check out Options: General Tab; thanks for the tip; been meaning to put together some more details on how the error thing works in the Wiki, but @joyfullservice has been forging ahead on the new merge tooling, so I've been waiting until that's settled down to do so.

joyfullservice commented 2 years ago

@joshrosario310 - Thanks for pointing this out! I did a little research this evening and found that the setting for this is stored in the registry in the following location:

image

Those two values correspond to the setting saved by the current user for Error Trapping.

image

I would propose that we add a check during the installation that verifies that the current user has VBA configured to Break in Class Modules. I can't really see a reason why any developer using this add-in would want it any other way. I know we have seen this before in other issues, so perhaps adding the check would help others avoid the hours of troubleshooting that Josh spent on it.

Although it would be trivial to change the registry value and transparently solve the problem for the user without them even knowing about it, I don't feel that this would be proper for the add-in to do this. Instead, I am proposing a message that warns them that they are not using the optimal setting for error handling in VBA, and that the add-in my not function as expected. This could then direct them to the wiki page as @hecon5 referenced, where they could find a more complete explanation of the setting. The user could then make an informed decision and change the setting themself in the VBA options dialog.

By way of explanation, this add-in uses error trapping directives in a number of places in the source code. There are certain things like determining whether a table has a table-level-macro that I have not found a way to identify without trapping the anticipate error. Other things like adding references to a database can generate errors, and it is better if we can trap those errors and provide meaningful information to the user rather than just breaking in the VBA source code and leaving users like Josh scratching their head trying to figure out what is wrong.

I will see if I can possibly put something together in the next few days to address this...

joyfullservice commented 2 years ago

For reference, I am linking to several other issues that related to this same Error Trapping setting in the VBA Options.

Ik-12 commented 2 years ago

"Error Trapping" settings can be get/set in semi-documented way using Application.GetOption/SetOption functions, see e.g. https://www.fmsinc.com/free/NewTips/VBA/BasicErrorHandling.asp

My suggestion would be saving current "Error Trapping" state before export (or import), change it to appropriate value and then restore the original when export has finished. I use this method in my unit testing framework and it works flawlessly.

hecon5 commented 2 years ago

@Ik-12, I like your idea to change it for export/build and then change it back after completion, with a single caveat: we should have a setting to allow that behavior. This would allow users to use other break methods where needed (or desired, even if they are the wrong setting...) for their dev work, and would allow the Addin to do its thing as needed, too.

@joyfullservice, I do think we should check the setting on install and prompt the user to change it, and tell them how to change it (Wiki!), but I don't think we should do it for them (for regular IDE work). Reasoning: we want the user to know WHERE the setting is, and to be able to change it. Teach a person to fish and all. If we simply change the setting, they might not know how to set it back for other areas where they need to change it for various scenarios.

Our admins have been putting the screws to us lately and (especially with Access Macros), it makes them upset when we fiddle with the registry (even with mundane things like this). We've had to recently jump through a lot of hoops to get this tool white listed, and audited. If we (my team) can continue to say we're not fiddling with things without consent it'll be an easier sell. If the Addin just starts to change settings without telling users, they'll ban it immediately and we'll have to start over.

Case in point: I was just sent an audit log and we had to 'splain to them each of the things it did (like writing files, and the mundane "launch after install save setting" #120) , and the like, so I know they're actually monitoring setting changes and it's not going to some humongous log in the sky and ignored.

joyfullservice commented 2 years ago

"Error Trapping" settings can be get/set in semi-documented way using Application.GetOption/SetOption functions, see e.g. https://www.fmsinc.com/free/NewTips/VBA/BasicErrorHandling.asp

My suggestion would be saving current "Error Trapping" state before export (or import), change it to appropriate value and then restore the original when export has finished. I use this method in my unit testing framework and it works flawlessly.

@Ik-12 - That is a fantastic idea! Thanks for pointing out the usage of GetOption/SetOption as well. I was not aware of this functionality and I like it much better than interacting directly with the registry values.

hecon5 commented 2 years ago

ooh, that's really clean, I like it!

joyfullservice commented 2 years ago

I have implemented this concept in the dev branch to roll out in the version 4 release. 👍

This concept of saving and restoring the session state also touched on the idea of cleaning up object references after the add-in completes the current operation. This is becoming increasingly important with the implementation of the ribbon interface, so this also lays some groundwork for this refactoring as well.

Whether an operation is launched from the add-in menu, the ribbon, or a call to the exposed COM API, we need to ensure that the environment is properly prepared for the add-in to run, then cleaned up afterwards so we don't have any dangling object references or temporarily adjusted user settings that could cause issues for our users.

We also need to account for the fact that in some scenarios the code to restore the user's settings may not run if the user ends the VBA code during a debug operation, or if the application crashes. For cases like this, the user's last saved settings should be restored the next time the add-in is run. (Commit coming soon for this.)

hecon5 commented 2 years ago

@joyfullservice Since you've patched this, should this close, or do we need to handle more details on this?

joyfullservice commented 2 years ago

@joyfullservice Since you've patched this, should this close, or do we need to handle more details on this?

Yes, I think we can close it now. I haven't done extensive testing with it, but it does seem to work for me. If we run into any further issues we can always reopen this issue, or create a new one. 👍