Re: [Libreoffice] [PATCH] Removed dependencies on tools/solar.h in drawpage.hxx/.cxx

Hi and thank you for your comments

2011/1/5 Kohei Yoshida <kyoshida@novell.com>:
> Hi Sören,
>
> Thanks for the patch.  Is there a reason why this one is not an
> attachment? :-)
It is not an attachment as I used "git send-email" to send it to the
mailing list, but the result seems a bit wierd when reading the list,
and it is not possible to give extra explanations in the mail when
sending the patch, so I think I will go back to attachments for my
next commits.

> Anyway, I have some comments.
>
> On Tue, 2011-01-04 at 20:20 +0100, Sören Möller wrote:
> > Replaced datatypes from tools/solar.h with corresponding types from sal/types.h in 
> > drawpage.hxx/.cxx and added missing include of sal/types.h in docparam.hxx
> > ---
> >  sc/inc/docparam.hxx              |    1 +
> >  sc/inc/drawpage.hxx              |    4 ++--
> >  sc/source/core/data/drawpage.cxx |    2 +-
> >  3 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/sc/inc/docparam.hxx b/sc/inc/docparam.hxx
> > index 2511f26..01170a5 100644
> > --- a/sc/inc/docparam.hxx
> > +++ b/sc/inc/docparam.hxx
> > @@ -32,6 +32,7 @@
> >  #ifndef SC_DOCPARAM_HXX
> >  #define SC_DOCPARAM_HXX
> >
> > +#include <sal/types.h>
> >  #include "address.hxx"
>
> This should not be necessary (though it won't hurt) as the address.hxx
> already includes sal types.  Does it not build without this change for
> you?
It does build without the change for me, I did the change to ensure
that we don't break the file accidentially by changing includes of
address.hxx, although I now can see that this maybe isn't necessary
for such basic includes (types are quite standard and almost everyone
includes address.hxx). I think I felt it more important yesterday
after reading a lot of files using things from tools/ but only
including them through other includes, which would break the code, if
I would remove the tools/ parts from the other includes first.

> >  // Let's put here misc structures that get passed to ScDocument's methods.
> > diff --git a/sc/inc/drawpage.hxx b/sc/inc/drawpage.hxx
> > index faa43fa..5e207b7 100644
> > --- a/sc/inc/drawpage.hxx
> > +++ b/sc/inc/drawpage.hxx
> > @@ -30,7 +30,7 @@
> >  #define SC_DRAWPAGE_HXX
> >
> >  #include <svx/fmpage.hxx>
> > -
> > +#include <sal/types.h>
> >
> >  class ScDrawLayer;
> >
> > @@ -39,7 +39,7 @@ class ScDrawLayer;
> >  class ScDrawPage: public FmFormPage
> >  {
> >  public:
> > -    ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, BOOL bMasterPage=FALSE);
> > +    ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, sal_Bool bMasterPage=sal_False);
> >      ~ScDrawPage();
> >
> >      virtual ::com::sun::star::uno::Reference< ::com::sun::star::uno::XInterface > 
> > createUnoPage();
> > diff --git a/sc/source/core/data/drawpage.cxx b/sc/source/core/data/drawpage.cxx
> > index d489d5d..5a91540 100644
> > --- a/sc/source/core/data/drawpage.cxx
> > +++ b/sc/source/core/data/drawpage.cxx
> > @@ -44,7 +44,7 @@
> >
> >  // -----------------------------------------------------------------------
> >
> > -ScDrawPage::ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, BOOL bMasterPage) :
> > +ScDrawPage::ScDrawPage(ScDrawLayer& rNewModel, StarBASIC* pBasic, sal_Bool bMasterPage) :
> >      FmFormPage(rNewModel, pBasic, bMasterPage)
> >  {
> >      SetSize( Size( LONG_MAX, LONG_MAX ) );
>
> For these two, let's use the standard bool type.  In fact, its parent
> class FmFormPage does expect a parameter of the bool type, not sal_Bool
> nor BOOL (though the default value is for some reason set to sal_False
> instead of false, which should really be fixed...), which provides
> another reason to use bool, not sal_Bool.
I think I'm still too afraid of breaking UNO-things, if there just is
something UNO-related close to the code, when using native types, I
will try to be more confident in using bool, and believing in it, if
my build works out fine.

Regards
Sören


> Thanks!
>
> Kohei
>
> --
> Kohei Yoshida, LibreOffice hacker, Calc
> <kyoshida@novell.com>