Bug 142960 - (usability) illegal characters in sheet name are checked one by one
Summary: (usability) illegal characters in sheet name are checked one by one
Status: RESOLVED FIXED
Alias: None
Product: calligrasheets
Classification: Applications
Component: general (show other bugs)
Version: 1.6.1
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Calligra Sheets (KSpread) Bugs
URL:
Keywords:
: 143171 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-03-14 14:20 UTC by Mathieu Jobin
Modified: 2007-08-10 15:29 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Jobin 2007-03-14 14:20:43 UTC
Version:           1.6.1 (using KDE 3.5.6, Gentoo)
Compiler:          Target: i686-pc-linux-gnu
OS:                Linux (i686) release 2.6.19-suspend2-r1-justbudget

This is really unclear as if it is a bug or a wish. It's not really a bug because while it ain't natural, in the end, it works. and its not something I really wish for deep in my heart. lets call it a usability bug.

lets rename a sheet for 'part-time vs full-time'

click OK. you can an error message that says the sheet name contains illegal characters. click OK. the first dash (-) has been replaced by an underscore (_) automatically. which is good. but then you click OK again, and get the same error message. click OK again, the second dash got then replaced. couldn't they both be replace at the same time?

was this a planned feature? or a weird usability? what do you guys think about this?

close a WONTFIX? I don't mind, as long as someone think about it. so I'm filling this so you know.

and before you tell me, the mailing list is a better place to discuss such things. I'm not sure how much power my opinion might have. and I can't be on every mailing list. IMHO, bugs/tickets are made for these.

regards,
Comment 1 Stefan Nikolaus 2007-08-01 21:05:03 UTC
*** Bug 143171 has been marked as a duplicate of this bug. ***
Comment 2 Stefan Nikolaus 2007-08-10 15:22:15 UTC
SVN commit 698608 by nikolaus:

Sheet		Sheet Name.
		No need to restrict ourselves too much: sheet names containing any character
		can be used for cell references. D-Bus object paths can contain letters,
		digits and underscores; replacing any other character by an underscore. The
		OpenDocument specification has no restrictions for sheet names.

BUG: 142960


 M  +1 -1      Map.cpp  
 M  +22 -12    Sheet.cpp  
 M  +2 -3      Sheet.h  
 M  +0 -19     Util.cpp  
 M  +0 -2      Util.h  
 M  +0 -18     ui/View.cpp  


--- trunk/koffice/kspread/Map.cpp #698607:698608
@@ -124,7 +124,7 @@
 Sheet* Map::createSheet()
 {
   QString name( i18n("Sheet%1", d->tableId++) );
-  Sheet* sheet = new Sheet( this, name, name.toUtf8() );
+  Sheet* sheet = new Sheet(this, name);
   return sheet;
 }
 
--- trunk/koffice/kspread/Sheet.cpp #698607:698608
@@ -95,6 +95,20 @@
 
 namespace KSpread {
 
+static QString createObjectName(const QString& sheetName)
+{
+    QString objectName;
+    for (int i = 0; i < sheetName.count(); ++i)
+    {
+        if (sheetName[i].isLetterOrNumber() || sheetName[i] == '_')
+            objectName.append(sheetName[i]);
+        else
+            objectName.append('_');
+    }
+    return objectName;
+}
+
+
 class Sheet::Private
 {
 public:
@@ -155,18 +169,10 @@
   return (*s_mapSheets)[ _id ];
 }
 
-Sheet::Sheet( Map* map, const QString &sheetName, const char *objectName )
+Sheet::Sheet(Map* map, const QString& sheetName)
     : QObject( map )
     , d( new Private )
 {
-  Q_ASSERT(objectName);
-  // Get a unique name so that we can offer scripting
-  if ( !objectName )
-  {
-    objectName = "Sheet" + QByteArray::number(s_id);
-  }
-  setObjectName( objectName );
-
   if ( s_mapSheets == 0 )
     s_mapSheets = new QHash<int,Sheet*>;
 
@@ -179,8 +185,12 @@
 
   d->name = sheetName;
 
+  // Set a valid object name, so that we can offer scripting.
+  setObjectName(createObjectName(d->name));
   new SheetAdaptor(this);
-  QDBusConnection::sessionBus().registerObject( '/'+map->doc()->objectName() + '/' + map->objectName()+ '/' + objectName, this);
+  QDBusConnection::sessionBus().registerObject('/' + d->workbook->doc()->objectName() +
+                                               '/' + d->workbook->objectName() +
+                                               '/' + this->objectName(), this);
 
   d->cellStorage = new CellStorage( this );
   d->rows.setAutoDelete( true );
@@ -225,8 +235,8 @@
         d->name = other.d->name + QString("_%1").arg(i++);
     while (d->workbook->findSheet(d->name));
 
-    QString objectName = d->name;
-    setObjectName(objectName.replace(' ', '_'));
+    // Set a valid object name, so that we can offer scripting.
+    setObjectName(createObjectName(d->name));
     new SheetAdaptor(this);
     QDBusConnection::sessionBus().registerObject('/' + d->workbook->doc()->objectName() +
                                                  '/' + d->workbook->objectName() +
--- trunk/koffice/kspread/Sheet.h #698607:698608
@@ -102,10 +102,9 @@
     enum TestType        { Text, Validity, Comment, ConditionalCellAttribute };
 
     /**
-     * Creates a sheet in \p map with the name \p sheetName . The internal
-     * name, for scripting, is set by \p name .
+     * Creates a sheet in \p map with the name \p sheetName.
      */
-    Sheet ( Map* map, const QString &sheetName, const char* name = 0 );
+    Sheet(Map* map, const QString& sheetName);
 
     /**
      * Copy constructor.
--- trunk/koffice/kspread/Util.cpp #698607:698608
@@ -169,26 +169,7 @@
         return false;
 }
 
-//used in View::slotRename
-bool KSpread::Util::validateSheetName(const QString &name)
-{
-  if (name[0] == ' ')
-  {
-    return false;
-  }
-  for (int i = 0; i < name.length(); i++)
-  {
-    if ( !(name[i].isLetterOrNumber() ||
-           name[i] == ' ' || name[i] == '.' ||
-           name[i] == '_'))
-    {
-      return false;
-    }
-  }
-  return true;
-}
 
-
 //not used anywhere
 int KSpread::Util::penCompare( QPen const & pen1, QPen const & pen2 )
 {
--- trunk/koffice/kspread/Util.h #698607:698608
@@ -62,8 +62,6 @@
      */
     KSPREAD_EXPORT QString encodeColumnLabelText( int column );
 
-    bool validateSheetName(const QString &name);
-
     //Return true when it's a reference to cell from sheet.
     KSPREAD_EXPORT bool localReferenceAnchor( const QString &_ref );
 
--- trunk/koffice/kspread/ui/View.cpp #698607:698608
@@ -6592,24 +6592,6 @@
 
   if( !ok ) return;
 
-  while (!Util::validateSheetName(newName))
-  {
-    KMessageBox::information( this, i18n("Sheet name contains illegal characters. Only numbers and letters are allowed."),
-      i18n("Change Sheet Name") );
-
-    newName = newName.simplified();
-    int n = newName.indexOf('-');
-    if ( n > -1 ) newName[n] = '_';
-    n = newName.indexOf('!');
-    if ( n > -1 ) newName[n] = '_';
-    n = newName.indexOf('$');
-    if ( n > -1 ) newName[n] = '_';
-
-    newName = KInputDialog::getText( i18n("Rename Sheet"),i18n("Enter name:"), newName, &ok, this );
-
-    if ( !ok ) return;
-  }
-
   if ( (newName.trimmed()).isEmpty() ) // Sheet name is empty.
   {
     KMessageBox::information( this, i18n("Sheet name cannot be empty."), i18n("Change Sheet Name") );
Comment 3 Stefan Nikolaus 2007-08-10 15:29:58 UTC
SVN commit 698613 by nikolaus:

Sheet		Sheet Name.
		Backward compatibility for KSpread's native format.

CCBUG: 142960


 M  +11 -1     Sheet.cpp  


--- trunk/koffice/kspread/Sheet.cpp #698612:698613
@@ -2138,8 +2138,18 @@
 QDomElement Sheet::saveXML( QDomDocument& dd )
 {
     QDomElement sheet = dd.createElement( "table" );
-    sheet.setAttribute( "name", sheetName() );
 
+    // backward compatibility
+    QString sheetName;
+    for (int i = 0; i < d->name.count(); ++i)
+    {
+        if (d->name[i].isLetterOrNumber() || d->name[i] == ' ' || d->name[i] == '.')
+            sheetName.append(d->name[i]);
+        else
+            sheetName.append('_');
+    }
+    sheet.setAttribute("name", sheetName);
+
     //Laurent: for oasis format I think that we must use style:direction...
     sheet.setAttribute( "layoutDirection", (layoutDirection() == Qt::RightToLeft) ? "rtl" : "ltr" );
     sheet.setAttribute( "columnnumber", (int)getShowColumnNumber());