Implement Form handler to ensure form states and actions are not hijacked

[hecon5]

          I'd like to request this is re-opened. I found a few cases where the form does automatically close. 

Reproduction steps:

1. Open AddIn.
2. (From ribbon) export source files. You can also run single object export, or whatever from the ribbon, too. It leads to same result.
3. Export runs.
4. Addin window closes.

It turns out that the way the form is referenced in the addin is the culprit; each module seems to load it separately. This led me to a red-herring approach when I tried this the first time and clsVersionControl.OpenVCSMainForm.

I think the "right" approach here is to have a form handler routine that manages the open methods and references. This allows us to ensure the "correct" flags are set regardless of the entry point; this also forces a single reference and if we need separate instances of the form open, we can handle that, too.

Regardless, the form still seems to close when you have it open and then run an action from the ribbon.

Originally posted by @hecon5 in https://github.com/joyfullservice/msaccess-vcs-addin/issues/503#issuecomment-2025252635

[hecon5]

503 related, but not the same issue

[hecon5]

See also this comment

[josef-poetzl]

What if you allowed multiple instances?

https://github.com/josef-poetzl/msaccess-vcs-addin/commit/7bace71de429607fd98bc39adf455993a72abe0b

[hecon5]

I think ultimately, that'll be the solution. It's what I did in a few of my projects to similar effect. There are a few caveats, though, which make me leery of doing such trickery in this case and lean towards a form manager/operation manager controller.

The issue becomes that without a form management class you can sometimes lose the form you needed, and any unhandled exceptions / errors (and sometimes handled ones) will cause the whole window to just poof out of existence and the whole process gets killed. This is obviously undesirable as if that happens during import/export you can end up with a corrupted code base or database, neither of which are desired.

[joyfullservice]

Multi-instance forms are very handy, and I use them in some applications like an Order Entry interface that allows multiple orders to be opened as different tabs. This isn't about anything wrong with the concept; I am just kind of scratching my head thinking, do we really need that overhead and complexity for this particular project?

Let me give a couple examples... The logging class has object references to the main form for outputting status messages. In a multi-instance environment, you will need to register the log instance to a specific instance of the form, and properly release those references. Yes, this can be done, but you have to be really careful not to get into an issue with circular or orphaned references that can in some cases unexpectedly crash Access. That's just not nice for the end users, especially if they have unsaved work and the crash corrupts their database. If you go into a debug situation, those class instances can get pretty dicey, in my experience. (Exactly like what @hecon5 describes above.)

With this add-in, I would like to keep things simple where we don't really need complexity. The cleaner and more intuitive the codebase, the more it will invite other developers to engage and contribute helpful features and bug fixes. For this particular issue, I am thinking that a simple approach would be to simply limit users from initiating other actions while a form is open. That will encourage linear operations, which is really how this tool is designed to be used. I might go ahead and put something together for that...

[bclothier]

Like Adam, I'm not sure multiple instancing is the answer here.

If the problem is with persisting the state, then I'd say that the logical thing to do is to take the state out of the form. It should be "dumb", simply displaying the output from some other place (e.g. a module for example). In this way, we don't have to care whether the form is open, is closed, or is hidden or whatever because the state is persisted independently of the form. That also makes it easier to implement checks to block actions that are inappropriate (e.g. running a export while the form is open) by invalidating the ribbon and disabling the controls.

Just an idea.

[joyfullservice]

Good thoughts, @bclothier. The way I think about it, the ribbon is what initiates actions. Some of these are simple autonomous actions like opening the source folder or visiting a web page. We don't care about state for those. But the more complicated operations (import/export/merge) are complex linear operations that may (or may not) use form elements, log classes, etc... We have some state management happening in the log class. This could be improved, or perhaps broken out into a better logical class (regardless of whether or not a log was involved) but that might be a rainy day project. 😉

I took a stab at a simplistic approach like what I was describing above. This seems to handle the immediate issue of unexpected behavior when another operation takes over the main form that is still open after the previous operation. (Technically this isn't a bit issue, in that the previous operation is already finished, but it could be a bit confusing for the user.)

[hecon5]

I like the approach, it's similar to what I'd originally proposed in PR #506.

I'd suggest two changes though:

1. Name it something else. HasFormOpen doesn't mean anything at all to me. What form? What has it open? Perhaps OperationInProcess would make sense given the response.
2. Instead of blocking the operation, I think the function should instead have a start/stop boolean static value, something like MajorOperationActive that is set / cleared by calling the function.

You could set it by calling, and return true if you have the "goahead" to do so, and return false if something else is already in operation. This ensures a clean entry/exit, and if a user just has the form open (but nothing happening yet), they don't get annoyed when they discover the form has to be closed only to reopen again.

I've got an idea how to mock this up, I'll post a PR shortly on it.