Hi Kohei,
Thanks for your comments.
On Fri, Jul 8, 2011 at 05:02, Kohei Yoshida <kyosh...@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
> <kyosh...@novell.com>
>
>
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
