Re: riched20: Make ITextDocument stub reusable for ITextServices (try 4)

[Alexandre Julliard]

Caibin Chen  writes:
> diff --git a/dlls/riched20/tomimpl.h b/dlls/riched20/tomimpl.h
> new file mode 100644
> index 0000000..886c3a8
> --- /dev/null
> +++ b/dlls/riched20/tomimpl.h
> @@ -0,0 +1,59 @@
> +/*
> -
> - RichEdit - TOM interfaces implementations
> -
> -
> -
> - Copyright 2013 by Caibin Chen
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
> -
>
> +#ifndef __RICHOLE_H
> +#define __RICHOLE_H
> -
>
> +#include <tom.h>
> -
>
> +#include "editstr.h"
> -
>
> +typedef struct ITextDocumentImpl ITextDocumentImpl;
> +typedef struct ITextSelectionImpl ITextSelectionImpl;
> -
>
> +/* ITextDocument */
> -
>
> +struct ITextDocumentImpl {
> - ITextDocument ITextDocument_iface;
> - IUnknown *outer_unk;
> - ME_TextEditor *editor;
> - ITextSelectionImpl *txtSel;
>
> +};

You shouldn't need a new header either. Particularly since
implementation details shouldn't be global.
> +/**
> -
> - Create an ITextDocumentImpl object, which acts as an inner object of COM aggression.

I'm sure writing COM code can make you feel aggressive, but that's not
the right term ;-)
> -
> -
> -
> - The ITextDocumentImpl object will delegate all of its IUnknown calls to the specified
> -
> - {@code outer_unk}.
> -
> -
> -
> - @param outer_unk The outer objects that creates and delegates ITextDocument
> -
> -
>    method calls to the created ITextDocumentImpl.
> -
> - @param editor The editor implementation.
> -
> -
> -
> - @return An ITextDocumentImpl object that should be destroyed with
> -
> - {@code ITextDocumentImpl_destroy}
> - */

Please don't use that sort of documentation markup. Besides, things like
parameters and return should be obvious from the code, you shouldn't
have to add documentation for this, especially not for internal
functions.

riched20: Make ITextDocument stub reusable for ITextServices (try 4)

[Caibin Chen]

2013/8/2 Alexandre Julliard :
> Caibin Chen  writes:
> > diff --git a/dlls/riched20/tomimpl.h b/dlls/riched20/tomimpl.h
> > new file mode 100644
> > index 0000000..886c3a8
> > --- /dev/null
> > +++ b/dlls/riched20/tomimpl.h
> > @@ -0,0 +1,59 @@
> > +/*
> > -
> > - RichEdit - TOM interfaces implementations
> > -
> > -
> > -
> > - Copyright 2013 by Caibin Chen
> > -
> > -
> > -
> > - This library is free software; you can redistribute it and/or
> > -
> > - modify it under the terms of the GNU Lesser General Public
> > -
> > - License as published by the Free Software Foundation; either
> > -
> > - version 2.1 of the License, or (at your option) any later version.
> > -
> > -
> > -
> > - This library is distributed in the hope that it will be useful,
> > -
> > - but WITHOUT ANY WARRANTY; without even the implied warranty of
> > -
> > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > -
> > - Lesser General Public License for more details.
> > -
> > -
> > -
> > - You should have received a copy of the GNU Lesser General Public
> > -
> > - License along with this library; if not, write to the Free Software
> > -
> > - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> > - */
> > -
> >
> > +#ifndef __RICHOLE_H
> > +#define __RICHOLE_H
> > -
> >
> > +#include <tom.h>
> > -
> >
> > +#include "editstr.h"
> > -
> >
> > +typedef struct ITextDocumentImpl ITextDocumentImpl;
> > +typedef struct ITextSelectionImpl ITextSelectionImpl;
> > -
> >
> > +/* ITextDocument */
> > -
> >
> > +struct ITextDocumentImpl {
> > - ITextDocument ITextDocument_iface;
> > - IUnknown *outer_unk;
> > - ME_TextEditor *editor;
> > - ITextSelectionImpl *txtSel;
> >
> > +};
>
> You shouldn't need a new header either. Particularly since
> implementation details shouldn't be global.

I expose these interfaces and the struct because they are used in both
txtdoc.c and richole.c

I understand that I can reach this goal by forward declare the struct
and interfaces in txtdoc.c. However I don't think it a good practice.

Besides, Jacek suggested in his review of try2 [1] that I should
expose the struct so that I don't need one more function to get the
COM interface from the structure. In this case the interface is the
first member and can be get by type casting, but I think this approach
will result in bad readability of the code.

Regards,

[1] http://www.winehq.org/pipermail/wine-devel/2013-July/100626.html