Hi Albert, On Sat, 2012-02-25 at 01:12 +0100, Albert Thuswaldner wrote:
> 1.) ScDocument::SetDocOptions requires some rethink. Here, now that > the format option stuff is moved to appoptions, only > xPoolHelper->SetFormTableOpt(rOpt); uses data from docoptions, the mix > of options makes this method ugly. Splitting this up might be a > solution, however ScDocument::SetDocOptions is used in quite many > places.... So, your observation is correct. As you say, it's best to take the code that sets the formula options out of SetDocOptions and into its own method. We could call it SetFormulaOptions() or something. The new SetDocOptions() would basically have nothing other than { OSL_ENSURE( pDocOptions, "No DocOptions! :-(" ); *pDocOptions = rOpt; xPoolHelper->SetFormTableOpt(rOpt); } and the rest can go to the new method. Also, while it's true that SetDocOptions() is called in many places, we only need to worry about two places; one is in ScModule::ModifyOptions(), and the other is in ScDocShell::SetConfigOptions(). In SetConfigOptions(), we won't need to worry about the doc options at all and only handle the formula options. I hope this will make it easier for you. > 2.) ScModule::ModifyOptions here improvements can possible be made to > minimize the need for updating/repainting the open document. Yes, although repainting can take place only in the visible region and is done only once after the Options dialog is dismissed. So, even if we repaint the whole thing it's not too bad. > 3.) ScTpFormulaOptions Class might be simplified somehow, for instance > to "group" the options like is done for View options and Print > Optiions(?) Yup, I agree. If we could somehow group the formula options like the view options, that would improve the readability as well as it would make it easier to extend it in future. But you can do this as a separate, follow-up change if you wish. BTW, the other part of your patch looks good. Thanks a lot for your work. Kohei -- Kohei Yoshida, LibreOffice hacker, Calc _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice