Re: Unifying Color

I see the error I’ve made now - it’s not possible to move Color to vcl as svl uses it. 


> On 11 Feb 2018, at 6:10 pm, Chris Sherlock <chris.sherlock79@gmail.com> wrote:
>
> Hi all, 
>
> I am trying to unify our color code. We currently have the following that handles color:
>
> Color - in tools
> BitmapColor - completely seperate and in vcl
> SalColor - a typedef to a sal_uInt32 with a MAKE_SALCOLOR constexpr
> BColor - part of basegfx, takes three doubles and inherits from B3DTuple
>
> As a start, I have removed the very unhelpful operator Color() from BitmapColor. It’s not clear 
> that the conversion from BitmapColor to Color is being handled by the operator function, so I 
> changed this to GetColor() to make it very clear that the two are not the same. 
>
> I firmly believe that the Color class should not be in tools, however. The only modules that use 
> this need vcl, and vcl seems to be the perfect module to keep this. Also, BitmapColor, IMO, 
> should really be derived from Color (thus doing away with BitmapColor::GetColor()).
>
> As a start I’ve submitted a change to gerrit to move Color from tools to vcl:
>
> https://gerrit.libreoffice.org/#/c/49169/ <https://gerrit.libreoffice.org/#/c/49169/>
>
> This compiles cleanly on my Linux and OS X system, but gives the following error on Windows in 
> gerrit (I don’t have a Windows system to compile on, unfortunately) - I genuinely don’t know why 
> this is occurring, can anyone help?
>
> > Creating library 
> > C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32/workdir/LinkTarget/Library/isvl.lib
> >  and object 
> > C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32/workdir/LinkTarget/Library/isvl.exp
> > numfmuno.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: unsigned 
> > long __thiscall Color::GetColor(void)const " (__imp_?GetColor@Color@@QBEKXZ) referenced in 
> > function "public: virtual long __cdecl 
> > SvNumberFormatterServiceObj::queryColorForNumber(long,double,long)" 
> > (?queryColorForNumber@SvNumberFormatterServiceObj@@UAAJJNJ@Z)
> > zformat.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: bool 
> > __thiscall Color::operator==(class Color const &)const " (__imp_??8Color@@QBE_NABV0@@Z) 
> > referenced in function "public: void __thiscall SvNumberformat::GetFormatSpecialInfo(bool &,bool 
> > &,unsigned short &,unsigned short &)const " 
> > (?GetFormatSpecialInfo@SvNumberformat@@QBEXAA_N0AAG1@Z)
> > zforscan.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall 
> > Color::Color(unsigned long)" (__imp_??0Color@@QAE@K@Z) referenced in function "public: 
> > __thiscall ImpSvNumberformatScan::ImpSvNumberformatScan(class SvNumberFormatter *)" 
> > (??0ImpSvNumberformatScan@@QAE@PAVSvNumberFormatter@@@Z)
> > zforscan.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual 
> > __thiscall Color::~Color(void)" (__imp_??1Color@@UAE@XZ) referenced in function "public: 
> > __thiscall ImpSvNumberformatScan::ImpSvNumberformatScan(class SvNumberFormatter *)" 
> > (??0ImpSvNumberformatScan@@QAE@PAVSvNumberFormatter@@@Z)
> > zforscan.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall 
> > Color::Color(class Color const &)" (__imp_??0Color@@QAE@ABV0@@Z) referenced in function "public: 
> > void __thiscall std::allocator<class Color>::construct<class Color,class Color>(class Color 
> > *,class Color &&)" (??$construct@VColor@@V1@@?$allocator@VColor@@@std@@QAEXPAVColor@@$$QAV2@@Z)
> > zforscan.o : error LNK2001: unresolved external symbol "public: virtual unsigned char __thiscall 
> > Color::GetBlue(void)const " (?GetBlue@Color@@UBEEXZ)
> > zforscan.o : error LNK2001: unresolved external symbol "public: virtual unsigned char __thiscall 
> > Color::GetGreen(void)const " (?GetGreen@Color@@UBEEXZ)
> > zforscan.o : error LNK2001: unresolved external symbol "public: virtual unsigned char __thiscall 
> > Color::GetRed(void)const " (?GetRed@Color@@UBEEXZ)
> > zforscan.o : error LNK2001: unresolved external symbol "public: virtual void __thiscall 
> > Color::SetBlue(unsigned char)" (?SetBlue@Color@@UAEXE@Z)
> > zforscan.o : error LNK2001: unresolved external symbol "public: virtual void __thiscall 
> > Color::SetGreen(unsigned char)" (?SetGreen@Color@@UAEXE@Z)
> > zforscan.o : error LNK2001: unresolved external symbol "public: virtual void __thiscall 
> > Color::SetRed(unsigned char)" (?SetRed@Color@@UAEXE@Z)
> > C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32/instdir/program/svllo.dll
> >  : fatal error LNK1120: 11 unresolved externals
> > [build CXX] framework/source/helper/statusindicator.cxx
> > [build CXX] framework/source/helper/statusindicatorfactory.cxx
> > [build CXX] framework/source/helper/tagwindowasmodified.cxx
> > [build CXX] framework/source/helper/titlebarupdate.cxx
> > [build CXX] framework/source/helper/uiconfigelementwrapperbase.cxx
> >
> > mt.exe : general error c101008d: Failed to write the updated manifest to the resource of file 
> > "C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32/instdir/program/svllo.dll".
> >  The system cannot find the file specified.
> >
> > make[2]: *** 
> > [C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32/svl/Library_svl.mk:20:
> >  
> > C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32/instdir/program/svllo.dll]
> >  Error 96
> > make[2]: *** Waiting for unfinished jobs....
> > make[2]: Leaving directory 
> > 'C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32'
> > make[1]: *** [Makefile:280: build] Error 2
> > make[1]: Leaving directory 
> > 'C:/cygwin/home/tdf/lode/jenkins/workspace/lo_gerrit/Config/windows_msc_dbgutil_32'
> > make: *** [C:/cygwin//home/tdf/lode/bin/cygwrapper.Makefile:6: all] Error 2
> > Build step 'Execute shell' marked build as failure
> > Finished: FAILURE
>
>
> ------
>
> Secondly, I have a gerrit patch that makes BitmapColor inherit from Color:
>
> https://gerrit.libreoffice.org/#/c/49203/ <https://gerrit.libreoffice.org/#/c/49203/>
>
> Moving forward, I’d like to get rid of BitmapColor entirely. The following comment was Tomaz, 
> haven’t had a chance to hop onto IRC so I thought I’d send an email to the mailing list. But his 
> comment was:
>
> > Hi Chris! 
> >
> > Good work with this but we need to think about what we want to do with Color first before we 
> > make any move like this. Generally, I feel the ultimate place where Color should actually be is 
> > in basegfx, but not in its current state. There is also BitmapColor in vcl, which does very 
> > similar things than Color and SalColor typedef, then there is also BColor in basegfx, which is a 
> > different beast (double colors channels are an overkill for most practical purposes) and let's 
> > ignore that one, for now at least.
> >
> > Generally I would like to first combine Color, BitmapColor and SalColor in a way first. Maybe 
> > get rid of BitmapColor altogether as it has very little purpose. And just have a simplistic 
> > ColorData/SalColor like struct or just a typedef, and then a Color wrapper around it, which does 
> > what currently Color, BitmapColor do. 
> >
> > There is also that that I'm refactoring those things to add alpha support (see 
> > feature/nativealpha branch), however I don't have much free time to work on this and there is 
> > also that I'm not too confident that this won't cause regressions. Especially regarding the SVM 
> > - StarView Metafile which is just writing of the Metafile to the disk. This is used in a lot of 
> > places and just changing one structure could possibly lead to breaking of that.
> >
> > This is why my new effort here is to separate and make it completely independent, how we store a 
> > Metafile to the disk as SVM. So it would become similar to any other vector format. But again, 
> > to do this there need to test for the Metafile. This is where I started to build svm tests [1], 
> > but they aren't complete. Once they are complete I can separate Metafile file persisting, which 
> > liberates Metafile in a way that it can be changed independently to the SVM file itself, which 
> > then means I can freely also change structures in VCL and can refactor things more confidently 
> > when adding native alpha support. What a rabbit hole…
> >
> > Anyway, if you want to join the effort, we could schedule a meeting (on IRC or whatever is good 
> > for you) and I'll explain the ideas I have and how/what to change if you wish. I'm currently in 
> > New Zealand so the timezone shouldn't be a problem. ;)
> >
> > [1] https://cgit.freedesktop.org/libreoffice/core/tree/vcl/qa/cppunit/svm/svmtest.cxx 
> > <https://cgit.freedesktop.org/libreoffice/core/tree/vcl/qa/cppunit/svm/svmtest.cxx>
> Any comments from others would be nice. 
>
> Thanks,
> Chris