Re: [Patch] [Feature] Gradient Fill for Cells in Calc

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