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
_______________________________________________
Libreoffice-ux-advise mailing list
Libreoffice-ux-advise@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice-ux-advise
