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

Hi Katarina,

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" 
and "undefined#"

> 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.

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.

Best regard

Bob


> B.
>
>
> _______________________________________________
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice