Date: prev next · Thread: first prev next last
2012 Archives by date, by thread · List index


On 09/10/2012 03:56 AM, Jan Holesovsky wrote:
Hi Muthu,

Muthu Subramanian K píše v Po 03. 09. 2012 v 13:04 +0530:

I have attached a first cut patch for the gradient fill for cells.
I initially tried using only the 'Gradient' tab from the area fill
dialog - but it was too tightly tied to it.
On the other hand probably we might want to include the other fill
styles (and other tabs in the area dialog) for cells in calc too (?).

Wow - I love the screenshot, thanks so much! :-)  It was your HackWeek
project, wasn't it? - nice work.

Pending issues (afaik):
1. Import/Export it to the ods format
2. Import/Export to xlsx format
3. Remove color tab from the format cell dialog
4. Implement 'no fill' option in area fill dialog
5. Testing: There could very well be bugs with this patch - I haven't
     done an extensive testing.
6. Minor (for documentation sake): Background color is set without
     testing if the user selected it or not. if(set) is required there.

I appreciate your listing of remaining issues. My question is that, this is apparently a work-in-progress code. Are you going to be working on the pending issues, and if yes, do you think you can make it in time for 3.7 or do you need more time? Or do you need someone else to take over? What's the overall game plan?

I would be really nice if somebody (both from calc and ux team) could
review this patch, please?

I don't feel like I should be approving the code, letting it to the Calc
guys...  Eike, Moggi, Kohei? :-)

I can't really "approve or disapprove" here, since the change is mostly in areas I'm not that familiar with. I trust Muthu did his due diligence to make sure the feature works, minus the pending issues. From my POV, I would just need some prospect on how the remaining issues are to be addressed.

Having said that, there are two minor comments I'd like to make. First, I'm a bit uncomfortable with the usages of pOldSet and pNewSet in ScTabViewShell::ExecuteCellAreaDlg(). It looks to me like pNewSet will leak when the dialog returns with Cancel. Also, I'm not sure if duplicating the old value with pOldSet there is necessary. You can perhaps use boost::scoped_ptr here to prevent accidental leakage of heap-allocated objects (if you really need to allocate them on the heap).

Second point is that the definition of operator== for XGradient has moved from the source to the header. I personally dislike putting method definitions in the header file, so I may be biased here. But I don't see the benefit of this relocation. I would just leave it at its original location.

Other than that, nothing else jumps out to me. The feature itself will be an welcome addition. I would like to see it included in the 3.7 release.

Thanks,

Kohei

--
Kohei Yoshida, LibreOffice hacker, Calc

Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.