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
_______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
