Bug 11028

Created attachment 2432 [details]
Changes to HSSFColor and other POI classes so that the color class contains instances rather than seperate classes

here is my problem with this.  You now have x number of classes that may *never*
be used.  Having one instance of BLACK and RED, I can stamp on to several
classes is less memory consumptive.  Can you think of a way to do this without
all those static instances created as required overhead?

-Andy

But isnt there only one instance of each color object being created? they were 
defined as public static final right? I think that you mean that the color 
BLACK (for instance) is always created but it may never be used, whereas in the 
existing approach the BLACK instance is only created at the time that it is 
required.

Whilst I see your point, I dont think that the overhead of a few default 
pallette colours is that bad compared with the memory used when reading in the 
XLS files.

Maybe the overhead (both memory and speed) of the class loader being invoked 
many times in loading the color classes will negate any perceived benefits, but 
I really dont know. I know that you should try to reduce the number of objects 
created, however I dont know that if the approach that exists is any better in 
the long run (but id like to be corrected ;-) )

Like many things there is a trade off between code readibility and maintenance, 
speed and memory usage.

Obviously since I made the patch I believe that this approach, whilst it may 
sacrifice some memory, is better in the long run.

What do you think? Is the patch worth the perceived increase in memory?

Jason

Personally I don't think so.  But I think you can accomplish what you're trying
to accomplish and what I'm trying to accomplish another way.

Secondly, I'm resistant to anything that increases memory for this form of
convienience.  Memory consumption is the biggest thing I want to fix after 3.0.

Third, any modificaiton to this class needs to keep the issue of "custom
palletes" in mind.  Meaning I don't want to drastically change this twice.  (Not
to mention it will be a big pain in the serializer)

Fourth, is that patch worth the memory -- AND breaking (though we don't
gurarantee interface backward compatibility in the development tree, we should
"be nice") every piece of code ever written to POI?

Here is what *would* be worth it in my view.

1. Look at custom pallettes 
2. Make this compatibile with a later approach involving custom palettes
3. probably make the constants static final ints for the indicies into the
default pallette

Here is what you'll come close to (I think, just a guess):

HSSFPallete  ----------------->  HSSFDefaultPallete     
    p HSSFColor getColorMatchingDefault(int colorindex)
    p HSSFColor getColorMatchingDefault(String triplet)
    p HSSFColor createColor(int index, String triplet, double
whateveryouneedforcustompallettes)
    p HSSFColor createColor(int index, String triplet, boolean matchesDefault,
int defaultindex, double whateveryouneedforcustompallettes)
    p HSSFColor getColor(int index)
    p HSSFColor getColor(String triplet)
    

HSSFDefaultPallete would have, in addition to the usual methods, constants
matching the *default* pallette 
 pfs int BLACK

HSSFWorkbook
  getDefaultPallette
  createCustomPallete
     (returns a pallete populated with the defaults)

While you don't have to implement the custom pallette functionality (I've
resigned myself to having to do this eventually), anything done to HSSFColor
that inconviences all existing users in hopes of convienienceing future users
needs to go in this direction IMHO, otherwise we'll just do it again later, and
thats just mean.

So I'll give you a vote on this.  If the majority of you, Glen, Avik, and Shawn
all agree on applying your patch I'll remove my objection.  (active HSSF developers)

What do you think?

BTW do you have the Excel 97 Developers kit?  

-Andy

I think that with the current aproach that there is a point when the number of 
colors that have been created will exceed the memory usage of this approach. 
Look at it this way, say there are 20 default pallete colors, with the current 
approach there are 20 classes that represent those colors. If i only want to 
use 10 colors then the class loader needs to load those 10 classes and then 
instanciate those classes. If I want to use all 20 colors then there are 20 
classes plus 20 objects to create. Which uses more memory here the class 
loading or the instanciation of the classes? Is it better to load one class and 
instanciate 20 objects. To be honest I dont know.

At the end of the day I dont mind if the old scheme is kept. I see the reason 
behind it, Andy wanted to keep the memory usage down , which IMHO is *very* 
important.

Yes I agree the solution should support the custom palletes. As such I am happy 
to leave this area as is for a while until the custom palletes code is worked 
on.

No I do not own the excel developers kit which is probably why i havent got my 
feet wet in the lower level structures.

Having said that I did try and find how to add a custom cell background color 
to cell in excel just to see the number of records involved. But i couldnt even 
find how to allocate a custom color in excel 97 !!! Its probably right in front 
of me but how do you do it?

Jason

I don't have Excel in front of me at the moment but....IIRC, If you look under
tools? -> options on "colors" there is a modify button.  

The Excel Developer's kit is about $15(US) on Amazon  (its out of print).  Your
fellow Aussie Glen picked one up.

As far as the memory issue, your point is well taken.  Its probably neither here
nor there.  I think my main issue is I'd like to do this just once.

If you're about to get the custom pallete stuff working that would be awesome. 
Otherwise I'll get around to it before too long I hope.

Yup cool its there all right.

Im going on holidays for the next two weeks. Maybe ill get bored and give it a 
go !

Jason

To help out I submitted the starts of the palette record class.  Open a new bug
or reopen this one when you have something.