Unifying Color

Sun Feb 11 13:30:59 UTC 2018

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 at 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 at Color@@QBEKXZ) referenced in function "public: virtual long __cdecl SvNumberFormatterServiceObj::queryColorForNumber(long,double,long)" (?queryColorForNumber at SvNumberFormatterServiceObj@@UAAJJNJ at 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 at SvNumberformat@@QBEXAA_N0AAG1 at Z)
>> zforscan.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall Color::Color(unsigned long)" (__imp_??0Color@@QAE at K@Z) referenced in function "public: __thiscall ImpSvNumberformatScan::ImpSvNumberformatScan(class SvNumberFormatter *)" (??0ImpSvNumberformatScan@@QAE at PAVSvNumberFormatter@@@Z)
>> zforscan.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: virtual __thiscall Color::~Color(void)" (__imp_??1Color@@UAE at XZ) referenced in function "public: __thiscall ImpSvNumberformatScan::ImpSvNumberformatScan(class SvNumberFormatter *)" (??0ImpSvNumberformatScan@@QAE at PAVSvNumberFormatter@@@Z)
>> zforscan.o : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall Color::Color(class Color const &)" (__imp_??0Color@@QAE at ABV0@@Z) referenced in function "public: void __thiscall std::allocator<class Color>::construct<class Color,class Color>(class Color *,class Color &&)" (??$construct at VColor@@V1@@?$allocator at VColor@@@std@@QAEXPAVColor@@$$QAV2@@Z)
>> zforscan.o : error LNK2001: unresolved external symbol "public: virtual unsigned char __thiscall Color::GetBlue(void)const " (?GetBlue at Color@@UBEEXZ)
>> zforscan.o : error LNK2001: unresolved external symbol "public: virtual unsigned char __thiscall Color::GetGreen(void)const " (?GetGreen at Color@@UBEEXZ)
>> zforscan.o : error LNK2001: unresolved external symbol "public: virtual unsigned char __thiscall Color::GetRed(void)const " (?GetRed at Color@@UBEEXZ)
>> zforscan.o : error LNK2001: unresolved external symbol "public: virtual void __thiscall Color::SetBlue(unsigned char)" (?SetBlue at Color@@UAEXE at Z)
>> zforscan.o : error LNK2001: unresolved external symbol "public: virtual void __thiscall Color::SetGreen(unsigned char)" (?SetGreen at Color@@UAEXE at Z)
>> zforscan.o : error LNK2001: unresolved external symbol "public: virtual void __thiscall Color::SetRed(unsigned char)" (?SetRed at Color@@UAEXE at 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
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/libreoffice/attachments/20180212/8801e4c0/attachment.html>