ScopeASCOM::SetupDialog should not unconditionally destroy COM scope instance

[bwdev01]

ScopeASCOM::SetupDialog change to address #1106.

[bwdev01]

This is issue 1106, just reported by end-user today. This involves some bare-knuckle COM code so it needs a careful review.

[agalasso]

This is issue 1106, just reported by end-user today. This involves some bare-knuckle COM code so it needs a careful review.

@bwdev01 small request: could fix the pr title so it is not truncated and add a pr desciprion.

BTW, when you refer to a github issue or PR and write it as #1106 then github will automatically create a link so the reader does not have to manually look up the issue

[bwdev01]

I felt like the original fix was rather awkward and a simpler approach could be taken. I just pushed those changes, let me know what you think.

From: Andy Galasso @. Sent: Monday, October 9, 2023 10:20 PM To: OpenPHDGuiding/phd2 @.> Cc: bwdev01 @.>; Mention @.> Subject: Re: [OpenPHDGuiding/phd2] ScopeASCOM::SetupDialog should not unconditionally destroy COM scope instance (PR #1107)

@agalasso commented on this pull request.

the fix looks good, thanks!

I have some readability/maintainability suggestions.

---

In scope_ascom.cpp https://github.com/OpenPHDGuiding/phd2/pull/1107#discussion_r1351482517 :

@@ -136,7 +136,7 @@ wxArrayString ScopeASCOM::EnumAscomScopes() return list; }

-bool ScopeASCOM::Create(DispatchObj& obj) +bool ScopeASCOM::Create(DispatchObj& obj, bool *alreadyRegistered)

I think this will be a little easier to understand if we change the name and invert the sense of this parameter

⬇️ Suggested change

-bool ScopeASCOM::Create(DispatchObj& obj, bool alreadyRegistered) +// registered is set to true if the Dispatch object was registered in the +// Global Interface Table +bool ScopeASCOM::Create(DispatchObj& obj, bool *registered)

so that if registered is returned as true then the caller may need to call Unregister

I would also suggest setting *registrered = false at the top of the function, then set it to true right after the call to m_gitEntry.Register(obj); as that will ensure that it never gets set to tru prematurely regardless of thrown exceptions etc.

---

In scope_ascom.cpp https://github.com/OpenPHDGuiding/phd2/pull/1107#discussion_r1351503162 :

@@ -179,7 +180,8 @@ bool ScopeASCOM::HasSetupDialog() const void ScopeASCOM::SetupDialog() { DispatchObj scope;

• if (Create(scope))
• bool alreadyRegistered;

⬇️ Suggested change

• bool alreadyRegistered;
• bool registeredInGit; // whether object just got registered in the global interface table

  ---

In scope_ascom.cpp https://github.com/OpenPHDGuiding/phd2/pull/1107#discussion_r1351503420 :

@@ -194,7 +196,8 @@ void ScopeASCOM::SetupDialog() // state where the user has killed the ASCOM local server instance and PHD2 is // holding a reference to the defunct driver instance in the global interface // table

• m_gitEntry.Unregister();
• if (!alreadyRegistered)

⬇️ Suggested change

• if (!alreadyRegistered)
• if (registeredInGit)

  ---

In scope_ascom.cpp https://github.com/OpenPHDGuiding/phd2/pull/1107#discussion_r1351504557 :

• bool alreadyRegistered = false;
• if (!Create(pScopeDriver, &alreadyRegistered))

if we make the output parameter optional (default value = nullptr) then we can keep this code simple as it was before

⬇️ Suggested change

• bool alreadyRegistered = false;
• if (!Create(pScopeDriver, &alreadyRegistered))
• if (!Create(pScopeDriver))

  ---

In scope_ascom.h https://github.com/OpenPHDGuiding/phd2/pull/1107#discussion_r1351504981 :

@@ -78,7 +78,7 @@ class ScopeASCOM : public Scope wxString m_choice; // name of chosen scope

 // private functions

• bool Create(DispatchObj& obj);
• bool Create(DispatchObj& obj, bool *alreadyRegistered);

⬇️ Suggested change

• bool Create(DispatchObj& obj, bool *alreadyRegistered);
• bool Create(DispatchObj& obj, bool *alreadyRegistered = nullptr);

— Reply to this email directly, view it on GitHub https://github.com/OpenPHDGuiding/phd2/pull/1107#pullrequestreview-1666198301 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ADDHSV63LG7DFXI2KTFJQZLX6TLJRAVCNFSM6AAAAAA5ZUHUZCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNRWGE4TQMZQGE . You are receiving this because you were mentioned. https://github.com/notifications/beacon/ADDHSV7E6YGTBHMOTZLUAKTX6TLJRA5CNFSM6AAAAAA5ZUHUZCWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTDKAVR2.gif Message ID: @. @.> >