[Libreoffice] [PATCH] conditional formatting with an unlimited number of rules
kmachalkova at suse.cz
Wed Apr 6 08:56:31 PDT 2011
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
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
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
\\\\\ Katarina Machalkova
\\\\\\\__o LibO developer
__\\\\\\\'/_ & hedgehog painter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 198 bytes
Desc: This is a digitally signed message part.
More information about the LibreOffice