Unifying Color

classic Classic list List threaded Threaded
3 messages Options
Chris Sherlock Chris Sherlock
Reply | Threaded
Open this post in threaded view
|

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:


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:


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. ;)

Any comments from others would be nice. 

Thanks,
Chris


_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Chris Sherlock Chris Sherlock
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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:


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:


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. ;)

Any comments from others would be nice. 

Thanks,
Chris



_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Tomaž Vajngerl Tomaž Vajngerl
Reply | Threaded
Open this post in threaded view
|

Re: Unifying Color

Hi,

On Sun, Feb 11, 2018 at 10:30 PM, Chris Sherlock
<[hidden email]> wrote:
> I see the error I’ve made now - it’s not possible to move Color to vcl as
> svl uses it.
>
Well basegfx is allowed for svl.. Anyway, to which module it belongs
is the least of the problems now. Tools is fine for the time being
(IMHO).

As for combining BitmapColor and others, look also into my nativealpha
branch, specifically the change to BitmapColor there  [1] where I got
rid of the mbIndex field and re-purposed it to be the alpha channel.
The trick here is that we know from the Bitmap if the colors are
indexed or not so there is no need for mbIndex flag at all. With only
BGRA channels this makes it practically the same as ColorData and
SalColor (supports only RGB, but for nativealpha it was extended to
RGBA). The thing you need to be very careful is the order of fields if
we store the BitmapColor structure into a file or the Metafile.

[1] https://cgit.freedesktop.org/libreoffice/core/commit/?h=feature/nativealpha&id=1aa579feaa841efcaee7389447c15c43a3d15b6d

Regards, Tomaž
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice