Unifying Color

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