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

Hi Muthu,

On Tue, Sep 11, 2012 at 3:46 AM, Muthu Subramanian K <sumuthu@suse.com> wrote:

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

Not for an experimental feature. But I'd still like to have *some*
plan for getting the file import/export supported.

> 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 :(

No one does.  Someone will have to dig deeper and find out.  If you
are willing, great. if not, we'll need to find someone else willing to
dig in. Unfortunately I'm already loaded with other things, so I won't
be available for this.. :-(

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

Then perhaps the same issue exists in the code you modeled yours from?
 Fixing that would be a bonus point. ;-)

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

I would still leave it as-is before your change (as I said in my previous post).

Thanks,

Kohei