[Libreoffice] [PATCH] conditional formatting with an unlimited number of rules

[Katarina Machalkova]

Hey Bob (& all)

> I finally finished my work on the conditional formatting with an
> unlimited number of rules.
> 
> Thank you to those who have helped me to create patch.
> I hope I have done things correctly. You will find the patch in 2
> attachment.

Wow, I'm impressed by how good job you did, esp. considering the fact that 
this is your first submission :) I've applied the patch and tested it and 
(save some small issues), it works reasonably well. Thanks so much for that.

Since this is quite a big feature patch, you're gonna receive rather long 
feedback from me, please don't be put off by that:

1) svx part of the patch is mostly OK ... except for the long list of new 
#includes. As the (French) comment says, not all of them are needed

Now for sc/calc part:

2) I find "Insert" button (newly added to the dialog) a bit redundant. All it 
does is that it appends an empty condition rule at the end of the list 
(position N+1) -- the same thing can be achieved by enabling/selecting 
checkbox by Nth rule in the list -- the scrollbar then moves, so the users see 
they can add more rules.

Additionally, its function is a bit buggy -- it tends to check some boxes that 
weren't previously checked and populates the affected entries with some 
default values (zeroes etc.) Therefore I'd suggest to remove it (+ related 
code as well) ... but we can discuss this further, it's a matter of taste.

3) In ScConditionalFormatDlg::InsertEntry and ScConditionalFormatDlg::AddEntry 
-- instead of handling/copying array of pointers to conditional format entries 
in this way, it'd be cleaner to work with some C++ container holding the 
pointers to entries -- std::vector or boost::ptr_vector

4) In ::Refresh and ::UpdateValueList (as well as ::CondNChecked) methods 
really, really lot of code is duplicated/copy&pasted around. That's on one 
hand consistent with the rest of the code in this class (yeah, unfortunately), 
but on the other hand not very nice.

5) Found a small buglet while testing (easy to fix) -- for newly appended 
condition entries, pre-selected style should be "Default". With your patch, it 
is the one that comes first in alphabetically sorted list (e.g. if I create a 
custom style named "Blue", it'll come before "Default" and all the newly 
created rules will have "Blue" style as default)

Sooo, what I'm gonna do now is that after fixing some very obvious quirks 
(reduce the list of includes in svx, fix some typos -- CondNChecked instead of 
CondNCheked, and three-liner fix for issue 5) I'll push the patch as it is, so 
that LibO users can use the feature already and give it a good deal of 
testing.

As for the code cleanup -- this is mostly about issues 3 & 4 -- we can do it 
together later.  Or, if you feel like it, you can try to improve the code 
yourself and send a patch to LibO list for review

Again, thanks a lot for the patch and keep up good work. Should you have any 
questions, please don't hesitate to poke any of Calc hackers (me, kohei, 
muthusuba etc.) on IRC

B.
-- 
  \\\\\              Katarina Machalkova    
  \\\\\\\__o          LibO developer
__\\\\\\\'/_          & hedgehog painter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110406/5dcebbd2/attachment.pgp>