Hey Albert, > I will check this patch tomorrow. A quick look through the patch did > not show any serious problems only some small nit-picks that I will > change while reviewing. >
Ok. So as promised I reviewed the patch now. Sadly there are some more problems than I thought after my first quick look: - please use Sc instead of SC for type prefix names - Can you recheck ScSortParam constructos that already fill maKeyState? std::vector[] does not increase the vector size so you are only allowed to access existing entries. I think we should use push_back there. - It seems that you have still some whitespace changes in ScTabPageSortFields::Reset - personally I'm not a huge fan of hungarian notation but since we are using it it would be great if you could use aNewSortParam instead of theNewSortParam - personally I prefer static_cast in c++ over the old c-cast but I have no strong opinion there - ScSortDescriptor::FillSortParam looks like there might happen some index out of bounds access Except for these small remarks the patch looks really good. Can you have a look at these small issues? The patch also contains a nice clean-up in the dialog code that will be a good start for the new ui. Thanks, Markus _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
