Hey Kohei, On Thu, Aug 28, 2025 at 10:26 AM Kohei Yoshida <ko...@libreoffice.org> wrote:
> Hi Markus, > > Good to see you again after a long hiatus. :-) > Great to see you too. > On 8/10/25 16:55, Markus Mohrhard wrote: > > Attached are two screenshots showing a comparison between the MSO Excel > rendering of my test document and the current Calc rendering. > > I think the rendering looks decent, even without font handling. > > > - font formatting is not implemented at all. Font handling is a lot > more complicated than background and border handling > > Indeed. I just looked through the code in question, and the complexity is > quite high there. Lots of special case handling... > > > - unit tests are completely missing > > I don't think that's a big deal especially for rendering features. > > > - link between table style and DB goes through the name instead of a > reference to the actual table definition > > I think that's fine as well. If we decide later that there is a better way > we can switch to that. > > > - The autoformat feature should be removed and folded into the table > style implementation which would also provide a UI for modifying table > styles. > > Auto format is such an outdated concept that I don't think anyone would > miss it. Now, a UI for modifying table styles, that can be done in a > separate step at least initially especially when availability of time (your > time) is limited. > As far as I know there is a GSOC project that will implement a new UI for table styles/autoformat. I would reuse that UI and just replace the existing autoformat feature. > > - merged cells are not handled at all right now > > A corner case that can be handled later, if it's really needed. > > > - some UNO interfaces to interact with this feature > > ... can definitely wait. :-) > > > Additionally, I might fix the following database range related issues > before merging if I continue with the current design: > > No objections from me. > > Now to the part that I'm not happy with and why I think someone else might > want to have a second look at the ideas behind this change: > > > - Storing the table style information purely on the ScDBData makes > some operations quite easy but at the same time makes the code in > ScDocument::FillInfo a lot more complicated. At the same time I have > not come up with a design that would allow us to avoid the position > dependent property lookup during FillInfo. We could store a reference to > the table style in the ScPatternAttr but then we need to keep the ScDBData > range and the ScPatternAttr in sync. In my mind the table style information > is not a cell attribute until the rendering stage. > > Table style information IMO is definitely not a cell attribute as you > say. In terms of how to store the table style information, I don't have a > strong opinion, but maybe you can try storing it outside of ScDBData and > use ScDBData's name as a way to reference it. I would personally avoid > touching ScPatternAttr for this feature, unless you absolutely have to. > > > - Adding font handling is going to be painful as the font lookup is > delayed until the actual drawing > - My current idea would be to store a reference to the table style > and a struct storing the style lookup information, e.g. this cell looks > at > first column stripe, second row stripe and whole table item sets. This > way > the information only needs to be computed once and the lookup for all > the > font properties can be done reasonably quickly. > - Potentially this could also be done by combining the handling of > conditional formatting and table styles. In theory a slightly cleaner > approach would be to have a list of SfxItemSet instances that are > checked > in order until an explicitly set item was found. That way there is no > need > to encode application layer information into the rendering code. > > Sounds sensible to me. Conditional formatting is probably the closest > feature in terms of applying extra font attributes, so it makes sense to > keep the new logic close to it. > > > - The fix for the border drawing issue mentioned above requires the > dynamic generation of new border items that can be passed through > ScDocument::FillInfo to the rendering code or a rework of the border > rendering. > > I think FillInfo itself could use some rework. ;-) It's screaming for > rework every time I look at it... Way too much logic is lumped into one > function body... But honestly I haven't spent that much time struggling > with this part of the Calc code, so take this with a grain of salt. > > I'd appreciate feedback from some other Calc developers about the design > and whether it is worth continuing with this design. > > I think it's well worth pursuing, if you are willing. > Thanks for the kind words. Cheers, Markus My 2 cents. > Kohei >
