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
Attachment:
signature.asc
Description: This is a digitally signed message part.