Milen A. Radev wrote:
> Please review the attached patch.
>
> (Thanks to Petyo Milotinov for his invalueable help)
Hi Milen,
This is an eyeball only review so far, but here's a few thoughts...
- pgDatabaseObject::CanDisconnect() doesn't seem to be used.
- frmMain::ReconnectDatabase() is obviously copied from
frmMain::ReconnectServer()... but is this the right code to use? If the
database state is just set back to disconnected, surely the existing
code can be used as if the user were connecting to the database for the
first time in the session?
- I'm not convinced that disconnectDatabaseFactory should be in
pgObject.cpp. Per the precedence set by disconnectServerFactory, it
should be in dlgDatabase.cpp (however, I'm not sure even that is right -
pgServer.cpp/pgDatabase.cpp would seem more sensible places for both).
- Commented out code such as:
+ //form->execSelChange(obj->GetId(), true);
should either have a comment explaining why it's commented out, or be
omitted from the patch altogether - otherwise, when we look at the code
in years to come we end up wondering why it's commented out!!
- What happens if the user disconnects from the maintenance database on
the server? Perhaps you intended pgDatabaseObject::CanDisconnect() to
return false in that case?
BTW, don't be put off by all those comments. You've jumped into one of
the more complex parts of the browser code :-)
Regards, Dave.
