[Libreoffice-ux-advise] [Patch] [Feature] Gradient Fill for Cells in Calc
Muthu Subramanian K
sumuthu at suse.com
Tue Sep 11 00:46:32 PDT 2012
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
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!
More information about the Libreoffice-ux-advise