Bug 49054 - EDITING: Table Design: changing column type does not reset length to default value
Summary: EDITING: Table Design: changing column type does not reset length to default ...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
3.4.4 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: BSA
Keywords:
: 53591 (view as bug list)
Depends on:
Blocks: Database-Tables
  Show dependency treegraph
 
Reported: 2012-04-22 08:42 UTC by Sandra Farnedi
Modified: 2019-12-03 14:02 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
bts (13.11 KB, text/plain)
2014-07-29 19:12 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sandra Farnedi 2012-04-22 08:42:15 UTC
When I define a table field in LbreOfficeBase I notice that the following field types NUMERIC, DECIMAL, FLOAT, VARCHAR, VARCHAR_IGNORECASE, inherit the latest set length, that is: when I add a field it is set to VARCHAR (100), if I change it to FLOAT its length is automatically set to 17, if I come back to VARCHAR, the field length is not set back to 100, but still remains 17.
Comment 1 Ferran Vidal 2012-04-22 11:05:42 UTC
[Reproducible] with "LibreOffice 3.5.2.2 - GNU/Linux Ubuntu 11.10,English
UI"
Comment 2 Alex Thurgood 2012-08-27 19:06:43 UTC
*** Bug 53591 has been marked as a duplicate of this bug. ***
Comment 3 Alex Thurgood 2012-08-27 19:09:30 UTC
Changing version to 3.4.4 as this is the earliest release in which I can reproduce the behaviour.


Alex
Comment 4 Alex Thurgood 2012-08-27 19:12:36 UTC
Changed title to better reflect situation
Comment 5 Julien Nabet 2014-07-24 07:31:27 UTC
On pc Debian x86-64 with master sources updated today, I could reproduce this.

After a gdb session, it seems the problem is here:
    224         if ( _pType->aCreateParams.isEmpty() )
    225         {
    226             SetPrecision(_pType->nPrecision);
    227             SetScale(_pType->nMinimumScale);
    228         }

http://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx#224

Indeed, the types which have the problem have _pType->aCreateParams not empty and so don't enter the if block.

NUMBER => "PRECISION,SCALE"
VARCHAR => "LENGTH"
etc.

Lionel: What's the purpose of this if block? Could it be removed?
Comment 6 Julien Nabet 2014-07-24 07:34:41 UTC
Forgot to say I was looking "precision" because of this line of the same file:
   1422     if( pLength )
   1423         pLength->SetText( OUString::number(pFieldDescr->GetPrecision()) );
See http://opengrok.libreoffice.org/xref/core/dbaccess/source/ui/control/FieldDescControl.cxx#1422
Comment 7 Julien Nabet 2014-07-24 08:12:51 UTC
If we suppose_pType->aCreateParams.isEmpty() is useful, here's a naive patch:
diff --git a/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx b/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx
index 65fb772..d3495a8 100644
--- a/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx
+++ b/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx
@@ -221,7 +221,7 @@ void OFieldDescription::FillFromTypeInfo(const TOTypeInfoSP& _pType,bool _bForce
                         SetScale(::std::min<sal_Int32>(GetScale() ? GetScale() : DEFAULT_NUMERIC_SCALE,_pType->nMaximumScale));
                 }
         }
-        if ( _pType->aCreateParams.isEmpty() )
+        if ( _pType->aCreateParams.isEmpty() || _bForce )
         {
             SetPrecision(_pType->nPrecision);
             SetScale(_pType->nMinimumScale);

since _bReset seems to deal with SetFormatKey and SetControlDefault, see lines 176..180 of this same file.
Comment 8 Lionel Elie Mamane 2014-07-24 08:36:19 UTC
(In reply to comment #7)
> If we suppose_pType->aCreateParams.isEmpty() is useful, here's a naive patch:
> diff --git a/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx
> b/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx
> index 65fb772..d3495a8 100644
> --- a/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx
> +++ b/dbaccess/source/ui/tabledesign/FieldDescriptions.cxx
> @@ -221,7 +221,7 @@ void OFieldDescription::FillFromTypeInfo(const
> TOTypeInfoSP& _pType,bool _bForce
>                          SetScale(::std::min<sal_Int32>(GetScale() ?
> GetScale() : DEFAULT_NUMERIC_SCALE,_pType->nMaximumScale));
>                  }
>          }
> -        if ( _pType->aCreateParams.isEmpty() )
> +        if ( _pType->aCreateParams.isEmpty() || _bForce )
>          {
>              SetPrecision(_pType->nPrecision);
>              SetScale(_pType->nMinimumScale);

To me, it looks like when changing type, the "switch" statement just before in the code is in charge (when the type changed, thus bForce is true, even if _bForce is not) of setting the default values. I'd recommend looking through it, what happens when the bug is hit?

But actually, it is not immediately obvious to me what the best behaviour should be here, actually. The bug reporter seems to suggest "always reset to the type default, for *any* type change". But keeping the same length (precision?) kinda makes sense, too. E.g. when going from FLOAT(17) to VARCHAR(n), well, a FLOAT(17) "fits" in a VARCHAR(17) (or is it VARCHAR(18) to account for the decimal separator? Need to look at definitions...), so one could argue that n=17 (or 18 / ...) is a rather good heuristic. A very quick glance at the code suggests that maybe that's what is being done, very much on purpose.

Similarly, when changing from NUMERIC to DECIMAL or vice-versa, keeping the same precision and scale again makes sense to me... Similarly, "in a perfect world" switching from INTEGER(10) to NUMERIC, I'd say that NUMERIC(10, 0) is as good a guess than any.

I'd also keep the previous length when switching between CHAR, VARCHAR, VARCHAR_IGNORECASE, etc, etc.
Comment 9 Julien Nabet 2014-07-29 19:12:53 UTC
Created attachment 103650 [details]
bts

After having put a break on SetPrecision, I retrieved differents bts showing 2 tests:
1) Varchar => Numeric (where precision isn't changed) which call 2 times SetPrecision()
2) Numeric => Real (where precision changed) which call 3 times SetPrecision() (because _pType->aCreateParams.isEmpty() - see my previous comment - is true)

If it can help...
Comment 10 Alex Thurgood 2015-01-03 17:39:49 UTC Comment hidden (no-value)
Comment 11 QA Administrators 2016-09-20 10:18:59 UTC Comment hidden (obsolete)
Comment 12 QA Administrators 2019-12-03 14:02:25 UTC
Dear Sandra Farnedi,

To make sure we're focusing on the bugs that affect our users today, LibreOffice QA is asking bug reporters and confirmers to retest open, confirmed bugs which have not been touched for over a year.

There have been thousands of bug fixes and commits since anyone checked on this bug report. During that time, it's possible that the bug has been fixed, or the details of the problem have changed. We'd really appreciate your help in getting confirmation that the bug is still present.

If you have time, please do the following:

Test to see if the bug is still present with the latest version of LibreOffice from https://www.libreoffice.org/download/

If the bug is present, please leave a comment that includes the information from Help - About LibreOffice.
 
If the bug is NOT present, please set the bug's Status field to RESOLVED-WORKSFORME and leave a comment that includes the information from Help - About LibreOffice.

Please DO NOT

Update the version field
Reply via email (please reply directly on the bug tracker)
Set the bug's Status field to RESOLVED - FIXED (this status has a particular meaning that is not 
appropriate in this case)


If you want to do more to help you can test to see if your issue is a REGRESSION. To do so:
1. Download and install oldest version of LibreOffice (usually 3.3 unless your bug pertains to a feature added after 3.3) from http://downloadarchive.documentfoundation.org/libreoffice/old/

2. Test your bug
3. Leave a comment with your results.
4a. If the bug was present with 3.3 - set version to 'inherited from OOo';
4b. If the bug was not present in 3.3 - add 'regression' to keyword


Feel free to come ask questions or to say hello in our QA chat: https://kiwiirc.com/nextclient/irc.freenode.net/#libreoffice-qa

Thank you for helping us make LibreOffice even better for everyone!

Warm Regards,
QA Team

MassPing-UntouchedBug