[MonoDevelop] Some more code for version control support
Tue, 07 Dec 2004 17:46:52 +1030
First, thanks a lot for your comments. Just a few things:
> * VcService is using a copy of the GetXmlAttributes and
> SetXmlAttributes from AbstractProject. We need to factorize this
> code into a class so it can be easily reused (although that code
> is really awful).
I absolutely agree, the reason I copied the code is I wasn't quite
sure where to put this new class, or what would be its main purpose.
> * Project specific information should always be stored in the
> project's directory, never in a global shared file.
The only project information stored in the VC file concerns the
repository, which can be shared among various projects - this
is inspired by the way things are handled in Eclipse. Another
reason to keep this stuff out of the project file was to
keep passwords/usernames in a separate place.
> * Looks like the class VcResource is not used at all.
You are right :)
We will use that once the connection between the backends and the vc
core is working. I think the actual interface that will use it
is in the wiki (bitflux has all that done :)
> * Why the IVcRepository interface is defining ToString()? This
> method is already defined in Object. If you want to give a
> special meaning to that method (such as, for example, getting
> the uri of the repository) better use another name.
That was not a good idea on my part - I will change that.
> * IVcLocalProject.PrjxFile: better use "ProjectFile", since the
> prjx extension may change in the future.
> * Since CvsService is actually a version control backend, I think
> that CvsBackendService would be more clarifying name.
> * In IVcBackend, it is difficult to understand what's the
> difference between CreateRepository and RawCreateRepository. I
> rather would use CreateRepository to create a IVcRepository
> instance, and a ShowEditRepositoryDialog method that displays
> the dialog that can be used to enter/modify repository
Good suggestion, will do that.
> * The CvsRepository.GetOptions method (which implements
> IVcRepository.GetOptions) returns some HTML that shows
> repository parameters. You are mixing version control logic code
> with GUI code here, and that's not good design pattern. A better
> solution would be to return, for example, a NameValueCollection,
> and leave to the GUI the choice of the best way of showing this
Using the HTML widget was not a good idea in the first place. Because
different backends will need to show different configuration stuff,
there will need to be a method returning a widget, or a method
that receives the optiondialog so that the backend can put its
configuration parameters there. Does that sound ok?
> * CvsService.CreateConnection is also mixing logic and GUI. You
> are assuming that a dialog will always be displayed when
> connecting a project to a repository, and this may not be true.
> You should have a method for creating a connection object and
> another for displaying the connection dialog.
I can certainly split the two, but I am not sure that this will
be a practical improvement - since the VC code does not know how
to manipulate the IVcRepository object at all - and it would probably
be useless to have a method to simply create a repository without
useful configuration parameters.
> * After going through the code, I think that a IVcRepository is
> not a version control repository, but a set of repository
> connection parameters. IVcLocalProject looks like it contains
> version control parameters for a project. Maybe you should use
> names that better describe what the class represents.
Ok - I was also following the wiki examples, so I accept new
name suggestions :)
> * The cvsOptionsDialog and cvsRepositoryWidget dialogs look very
> similar, they could probably be the same.
I will look into this.