Hi Joost, On Fri, 2010-12-10 at 20:56 +0100, Joost Eekhoorn wrote:
> Please review if this patch is realy correct and complete. Good work! This is definitely a step in the right direction. There are some comments from my side, which I provide below. About UI * Let's remove the 'New Name' string as I feel this is redundant. And let' place the rename box either to the immediate right of 'Rename' check box, or immediately below it. I prefer it being to the right of the check box since we have a plenty of space there. * Instead of hiding the rename box, it's better to disable it when the Rename check box is not checked. We generally don't show or hide controls but enable or disable it. * Let's disable the Rename check box as well as the rename input box when multiple sheets are selected. We don't know what we should do for multiple sheet copy/move & rename yet, so I would be more comfortable disabling it in such cases (at least for now). * I think it would be more user-friendly if the Rename input box showed the default sheet name. When moving a sheet, this would be the original sheet name, while when copying a sheet it would be the original name followed by '_' + <num> (e.g. Sheet1 -> Sheet1_1). Others * Move & rename sheet and undo afterward doesn't undo the renaming. But this is less critical, and I could look into it if you don't want to. Code: * Regarding the additional parameter in ScViewFunc::MoveTable(), I prefer using a pointer to String with a default value of NULL, since it's an optional parameter conceptually. All in all, I'm happy with what I see there, so, good work! :-) Let me know if you need any help with any of the above items. I'll CC Christoph in case he has some comments on this feature as well as on my comments above. Christoph, please feel free to add your comments as well if you have any. :-) > I did not know how test the automation part in > source/ui/view/viewfun2.cxx It looks okay to me, though if we decide to only support a single sheet rename (as I suggested above) we may need to change the code here a bit. But that's not a big deal. > And check if String() in correct in ExecuteDrop() in > sc/source/ui/view/tabcont.cxx It's correct, though as I mentioned above, if we use a String pointer with NULL as the default value we won't need to modify this part. > What must I did with move-copy-sheet.xml (on 2 places!). Let's leave this one alone. This file is for the new layout engine whose development was suspended at the moment. So, let's not worry about this file. > Must the help be adapted? How/where must that be done? I would say let's finish this feature first, then worry about the help later. Good stuff! Kohei -- Kohei Yoshida, LibreOffice hacker, Calc <kyosh...@novell.com> _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
