Re: [Libreoffice] [PATCH] Making default tab prefix name configurable in Calc

On Sat, 2011-07-02 at 12:09 +0200, Albert Thuswaldner wrote:
> Hi
> Submitting a patch for review. This one enables the user to set the
> default tab prefix name in calc.
> The new option is located in Menu -> Tools -> Options -> LibreOffice
> Calc -> Defaults
>
> - I think this Is a useful feature for some at least,  a few LO/OOo
> users i've talked to seem to agree.
> - it is unobtrusive to the user, it does not change the default behavior.
> - also code-wise it does not come with any radical changes.
>
> As always please let me know if you want me to improve it.

Hi Albert,

First of all, thanks for your patches.  It's good to see you continue to
work on hacking Calc.  As you requested, here are my comments on your
changes followed by the relevant (partial) hunk.

1)

+    SC_DLLPUBLIC sal_Bool       GetFallBackPrefix(String& aStrPrefix)
const;

Let's use the real bool here instead of sal_Bool in the new method
signature.  Also, let's use rtl::OUString instead of String here as
well.  We are trying very hard to remove sal_Bool and String types, so
it's best not to use them in new code.

2)

+    // Test if prefix is valid
+    sal_Bool bPrefix = ValidTabName( aStrPrefix );
+    if ( !bPrefix )
+        {
+            bPrefix = ValidTabName( aStrPrefix );
+            aStrPrefix = aStrPrefix2;
+        }

Our preferred bracket style would be

+    if ( !bPrefix )
+    {
+        bPrefix = ValidTabName( aStrPrefix );
+        aStrPrefix = aStrPrefix2;
+    }

3)

+            case SCDEFAULTSOPT_TAB_PREFIX:
+                OUString aPrefix;
+                if (pValues[nProp] >>= aPrefix)
+                    SetInitTabPrefix(aPrefix);
+                break;
+

Here, you've declared a local variable inside a case scope.  You need to
surround the whole case block with { } like so:

case SCDEFAULTSOPT_TAB_PREFIX:
{
    ...
}

4)

@@ -84,4 +92,6 @@ int ScTpDefaultsOptions::DeactivatePage(SfxItemSet* /*pSet*/)
     return KEEP_PAGE;
 }
 
+//ScGlobal::GetRscString(STR_TABLE_DEF); //"Table"
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */

This hunk is unnecessary.

5) Regarding the options paths, with your change we now have

Calc/Defaults/Other/TabCount
Calc/Defaults/Other/TabPrefix

option paths.  I would prefer changing them to

Calc/Defaults/Sheet/SheetCount
Calc/Defaults/Sheet/SheetPrefix

because 1) I don't like to use Other or Misc *unless* there are no other
options, and 2) these options apply globally to sheet's internal default
behavior, not just the sheet tab, so I would prefer that being reflected
in the option names as well.  Here, whether to use Sheet or Table is a
matter of preference, but I tend to use Sheet in user visible parts.

These are minor issues, and I can make these changes if you want me to.

Now, a slightly bigger issue.  The sheet prefix option input box doesn't
check for valid sheet name.  It even allows an empty one, which is not
very good.  So, we should at least apply the same restriction as in
ScDocument::ValidTabName() method (located in document.cxx#250).  You
can simply call that method from the option page to validate the name
since that method is static.

The preferred way to set the restriction is to inspect the new name each
time the text value in the input box changes.  You can intercept a input
value change event for the input box to do the inspection and accept or
reject the new text value.  I believe there are other UI parts that do
the same thing so hopefully you can find some example code to follow.
If not, let me know and I'll dig in to find one.

This is all I can think of at the moment.  Let me know if you need more
clarifications on any of these points.

Kohei

-- 
Kohei Yoshida, LibreOffice hacker, Calc
<kyoshida@novell.com>