<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"> </head> <body bgcolor="#ffffff" text="#000000"> 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 : <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">Hey Bob (& all) </pre> <blockquote id="StationeryCiteGenerated_1" type="cite"> <pre wrap="">... </pre> </blockquote> <pre wrap=""> 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. </pre> </blockquote> I've not invent anything, it's just the result of code analyse and reorganize it. (many "cut & paste"). <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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 </pre> </blockquote> 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 ? <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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. </pre> </blockquote> 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 ;-) <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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 </pre> </blockquote> 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. <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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. </pre> </blockquote> Ok, I can improve this. <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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) </pre> </blockquote> Thank, I've stupidly not tested with other style names than "Default" and "undefined#" <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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. </pre> </blockquote> Whaou ! super :-) I will send a new patch tomorrow <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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 </pre> </blockquote> Yes, I try to improve the code and send a next patch. <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap="">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 </pre> </blockquote> Ok, thank you. In first times, I will send questions on this dev list because I'm not regular IRC user. Best regard Bob <blockquote id="mid_201104061756_44862_kmachalkova_suse_cz" cite="mid:201104061756.44862.kmachalkova@suse.cz" type="cite"> <pre wrap=""> B. </pre> <pre wrap=""> <fieldset class="mimeAttachmentHeader"></fieldset> _______________________________________________ LibreOffice mailing list <a class="moz-txt-link-abbreviated" href="mailto:LibreOffice@lists.freedesktop.org">LibreOffice@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="http://lists.freedesktop.org/mailman/listinfo/libreoffice">http://lists.freedesktop.org/mailman/listinfo/libreoffice</a> </pre> </blockquote> </body> </html>