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

Hi Kohei,

On 09/10/2012 11:03 PM, Kohei Yoshida wrote:
> 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?
The minor issues can be handled before 3.7. I guess the only feature is
import/export to and from ods and xlsx. How important are they for an 
experimental feature? Is it necessary before the code is merged to 
master (or 3.7), please?

If so, I would surely need help there - specifically the formats for 
ods. Do we have something there which we could (re)use (e.g. reuse area 
fill elements?). I don't know much there, unfortunately :(

> > > 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).
oh..I will look at this - I just used the way ExecuteCellFormatDlg() 
works - I could have very well missed important things there :( Thanks!

> 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.
If you look at that class - all of its members are inline (and defined 
in the header). Unfortunately, it seems like the library is not 
available to do the == test in frmitems.cxx. Since all the other members 
are inline and this check too was a simple return statement I chose to 
move it to the header. Would you prefer some other way, please?

> 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.
I would like it too in 3.7 at least as an experimental feature for users 
to try it out - even if its with limitations.

Thank you so much!
Muthu Subramanian

> Thanks,
>
> Kohei