Bug 92893 - replace UNO in table import
Summary: replace UNO in table import
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyMedium, easyHack, skillCpp
Depends on:
Blocks: XLSX
  Show dependency treegraph
 
Reported: 2015-07-23 16:18 UTC by Markus Mohrhard
Modified: 2021-08-09 05:25 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Mohrhard 2015-07-23 16:18:38 UTC
Currently we use the UNO API for table import from XLSX. We should use direct calls and use ScDBData directly.

The import can be found in sc/source/filter/oox/tablebuffer.cxx
Comment 1 Robinson Tryon (qubit) 2015-12-13 10:58:03 UTC Comment hidden (obsolete)
Comment 2 Robinson Tryon (qubit) 2016-02-18 14:52:18 UTC Comment hidden (obsolete)
Comment 3 Chen Huang 2016-03-05 03:57:18 UTC
I would like to give this task a try.

Just curious, is this just a cleanup of legacy API or is there any other issue with UNO API (performance, buggy,...)?
Comment 4 Markus Mohrhard 2016-03-05 19:41:45 UTC
(In reply to Chen Huang from comment #3)
> I would like to give this task a try.
> 
> Just curious, is this just a cleanup of legacy API or is there any other
> issue with UNO API (performance, buggy,...)?

UNO is not a legacy API. It is our public external API. However as an external API it is quite slow as it has to do many checks (invalid user input, ...) and needs to take care of features not used during import like undo/redo, ...
In contrast to that using the direct calc internal API (normally around ScDocument) is as fast as possible and makes both debugging as well as profiling a lot easier.
Comment 5 Chen Huang 2016-03-08 01:21:58 UTC
Markus, thanks for the clear explanation.

I am assigning this to myself and start to work on it.
Comment 6 Chen Huang 2016-03-17 04:24:37 UTC
I have a few questions after looking at the code for a bit.

From tablebuffer.cxx:
void Table::finalizeImport() {
  ...
  PropertySet aPropSet( ... );
  if (no header) {
    aPropSet.setProperty( PROP_ContainsHeader, false);
  }
  ...
}
Why do we ever set PROP_ContainsHeader in this method given that aPropSet is a local variable and not passed back to caller?


I saw we have a ScDocument class and a ScDBData class, what's the difference between them?

If I understand correctly we don't really need to populate all the members of above classes and only need to properly fill maDestRange and mnTokenIndex in Table class. Is it correct?
Comment 7 Markus Mohrhard 2016-03-19 14:59:43 UTC
(In reply to Chen Huang from comment #6)
> I have a few questions after looking at the code for a bit.
> 
> From tablebuffer.cxx:
> void Table::finalizeImport() {
>   ...
>   PropertySet aPropSet( ... );
>   if (no header) {
>     aPropSet.setProperty( PROP_ContainsHeader, false);
>   }
>   ...
> }
> Why do we ever set PROP_ContainsHeader in this method given that aPropSet is
> a local variable and not passed back to caller?

aPropSet is a wrapper around xDatabaseRange with a nicer interface. You can look into the PropertySet class to see how it stores xDatabaseRange in a member variable and in the call to setProperty forwards the call to this member variable.

> 
> 
> I saw we have a ScDocument class and a ScDBData class, what's the difference
> between them?

ScDocument represents a calc document. ScDBData a database range.

> 
> If I understand correctly we don't really need to populate all the members
> of above classes and only need to properly fill maDestRange and mnTokenIndex
> in Table class. Is it correct?

No. We need to set all properties that are handled at the moment through UNO directly to ScDBData.
Comment 8 jani 2016-04-19 06:26:26 UTC
A polite ping, still working on this issue ?
Comment 9 Chen Huang 2016-04-19 20:58:50 UTC
Sorry for no updates...

I am still working on this and should have followup updates soon.
Comment 10 Chen Huang 2016-04-26 08:38:19 UTC
My thinking is to add a helper in WorkbookGlobals like this:
ScDBData* createDatabaseRange(OUString& orName, range and other args) {
  NamedDBs& dbs = getScDocument().GetDBCollection()->getNamedDBs();
  if (dbs.findByUpperName(orName) != nullptr) { error }
  else {
    ScDBData* range = create ScDBData with orName, range and other args;
    dbs.insert(range);
    update maDBRangeName;
  }
}
Is this a reasonable way to achieve what we want?


And I have a few more questions:
- ScDBData starts a listener upon inserted into NamedDBs. But at this point we have not validated the range of the ScDBData yet so we are possibly listening on a wrong area, is it ok?

- Does mnTokenIndex in Table map to nIndex in ScDBData? If so how come they have different types? Although I think in reality it might not matter since int16 would be large enough to hold the number of sheets...

- I am having some trouble to see how the current Reference<XDatabaseRange> based implementation eventually transforms the range into a ScDBData, can you point me to where this occurs?
Comment 11 Chen Huang 2016-05-11 04:16:15 UTC
This seems to touch a lot of stuffs here and there and I am having trouble to fully understand the impact of my change given that this is my first task and I am still trying to get myself more familiar with the code base.

To not block this from being solved by others, I am making this back to "OPEN" and will look for some other tasks that have more limited scope.
Comment 12 jani 2016-05-11 05:51:53 UTC
resetting assignee.