Re: [Libreoffice] [PATCH] EasyHacks 3.27 Change Sheet copy process

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
<kyoshida@novell.com>