Date: prev next · Thread: first prev next last
2011 Archives by date, by thread · List index


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>



Context


Privacy Policy | Impressum (Legal Info) | Copyright information: Unless otherwise specified, all text and images on this website are licensed under the Creative Commons Attribution-Share Alike 3.0 License. This does not include the source code of LibreOffice, which is licensed under the Mozilla Public License (MPLv2). "LibreOffice" and "The Document Foundation" are registered trademarks of their corresponding registered owners or are in actual use as trademarks in one or more countries. Their respective logos and icons are also subject to international copyright laws. Use thereof is explained in our trademark policy.