[Libreoffice] [PATCH] conditional formatting with an unlimited number of rules
libo at bobiciel.com
Wed Apr 6 16:47:04 PDT 2011
Thank for your evaluation work and your detailed feedback. I insert my
answers in your text.
Le 06/04/11 17:56, Katarina Machalkova a écrit :
> Hey Bob (& all)
> 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.
I've not invent anything, it's just the result of code analyse and
reorganize it. (many "cut & paste").
> 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
Oups ! I create a patch with translations of French comments ;-)
More seriously : I have let things like that because I project to add
other "preview fonctionalities" (rotate, borders, ...) and I don't know
exactly which header-files are necessary.
I will work to clean it.
Is there a tool to make warning for useless hxx ?
> 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.
The "insert fonctionality" make _realy an insert_ ! But the visual
result is not really evident for user ...
A new entry is inserted on the first position in the dialog box (It's
not correctly visible because I've directly selected aCbxCond1).
I think then insert an unchecked entry, will better see new entry. What
do you think about that ?
(I hope my googled-english is readable ;-)
> 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
Ok, I'm going to buy a new C++ book, because I don't find "std::vector"
references in my old one ! (Borland C++ dating the past siecle, with the
price in francs ;-)
I think I can work on this in a next time.
> 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.
Ok, I can improve this.
> 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)
Thank, I've stupidly not tested with other style names than "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
Whaou ! super :-) I will send a new patch tomorrow
> 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
Yes, I try to improve the code and send a next patch.
> 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
Ok, thank you. In first times, I will send questions on this dev list
because I'm not regular IRC user.
> LibreOffice mailing list
> LibreOffice at lists.freedesktop.org
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the LibreOffice