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

Hi Kohei,
Thanks for your comments.

On Fri, Jul 8, 2011 at 05:02, Kohei Yoshida <kyoshida@novell.com> wrote:
> 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.

Ok I'll fix that. I actually though that sal_Bool was the preferred
type, now I know better.

> 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;
> +    }

typo, fixing it.

> 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:
> {
>    ...
> }

Ok, do that.

> 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.

Yea, left over should have been deleted.

> 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.

Ok, makes perfectly sense now that there are two options in this
category. I'll make the change.

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

Thanks but I'll do that myself, otherwise I will never learn. ;)

> 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.

It makes sense to check it directly at input. I wasn't sure how to
implement that, especially without cluttering up the code.
Thanks for the pointers. i will have a go at it.
/Albert

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