Hi Kohei, Thanks for your review. Hereby an additional patch with changes on 4 comment-items. 2 items are not done. MPL 1.1 / GPLv3+ / LGPLv3+ > 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. Done. > * 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. Done. > * 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). Done. > * 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). An empty imput box is logical to me, because the Rename dialog shows also an empty in input box. But if you want, I can try to impement it. It is not in this patch, because it more difficult than it sounds. The default name depends on the document where the copy is going to, if it is the same document or another document. > 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. Skipped. If you do undo (Ctrl-Z) twice, then the renaming will also be undone. > 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. Done. Joost <div class="gmail_quote">2010/12/11 Kohei Yoshida <<a href="mailto:kyoshida@novell.com" target="_blank">kyoshida@novell.com</a>> <blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div>On Fri, 2010-12-10 at 20:31 -0500, Kohei Yoshida wrote: > 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. :-) </div>And these are the screenshots of the new dialog. Rename unchecked <a href="http://people.freedesktop.org/%7Ekohei/sheet-rename-unchecked.png" target="_blank">http://people.freedesktop.org/~kohei/sheet-rename-unchecked.png</a> Rename checked <a href="http://people.freedesktop.org/%7Ekohei/sheet-rename-checked.png" target="_blank">http://people.freedesktop.org/~kohei/sheet-rename-checked.png</a> <div><div></div><div> -- Kohei Yoshida, LibreOffice hacker, Calc <<a href="mailto:kyoshida@novell.com" target="_blank">kyoshida@novell.com</a>> </div></div></blockquote></div>