Differential D24761

Asign correct column width when importing XLS file
ClosedPublic
Actions

Authored by davidllewellynjones on Oct 18 2019, 3:53 PM.

Details

Reviewers
pvuorela
Group Reviewers
Calligra: 3.0
Commits
R8:e80856e8eb6a: Asign correct column width when importing XLS file
Summary

When importing an Excel 97-2003 .xls file (using the excel importer) the column widths don't match those from the file exported from Excel (or LibreOffice). Moreover, the widths of the columns depend on the dpi of the device in use, even though this doesn't affect the contents of the cells.

For any screen that is higher than 96 dpi the columns will be too narrow and cell contents are likely to be truncated.

This patch fixes the issue so that the correct column widths are assigned. As far as I can tell, this is the only situation where QWidget.physicalDpiX() is being explicitly used in the code.

http://www.flypig.co.uk/dnload/dnload/other/calligra-column-widths.zip

Test Plan
  1. Download the following archive and extract the test-widths.xls and test-widths.xlsx files.

http://www.flypig.co.uk/dnload/dnload/other/calligra-column-widths.zip

  1. Open the files. The column widths should be the same.

The archive also contains screenshots of the patched and unpatched versions of Calligra using two different dpi screens.

Diff Detail

Repository
R8 Calligra
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald TranscriptOct 18 2019, 3:53 PM
davidllewellynjones requested review of this revision.Oct 18 2019, 3:53 PM
davidllewellynjones edited the test plan for this revision. (Show Details)Oct 18 2019, 4:10 PM
pvuorela added a comment.Oct 25 2019, 11:27 AM

Seems it was indeed the only physicalDpi, though not too many logical either. Not trivial to follow, but seems sane and the screenshots are thorough. I'll approve if no-one objects shortly.

pvuorela accepted this revision.Oct 28 2019, 8:56 AM
This revision is now accepted and ready to land.Oct 28 2019, 8:56 AM
dcaliste added a subscriber: dcaliste.Oct 28 2019, 1:36 PM

Well, after reading the difference between the logical and physical DPI values: https://stackoverflow.com/questions/16561879/what-is-the-difference-between-logicaldpix-and-physicaldpix-in-qt (see the first answer), using the logical value is consistent with the hard coded "arial 10" font metric some line above.

Maybe adding a comment near the modified line explaining that logicalDPI should be used because of the "Arial 10" font metric being nice also on high DPI screen would be nice. If not too much burden…

Good catch @davidllewellynjones !

davidllewellynjones updated this revision to Diff 68959.Oct 29 2019, 10:08 AM

Thanks here also @dcaliste. This is all a bit confusing I agree and deserves some explanation. I've added in something along the lines you suggested, which I think helps (your insight that the scaling has to match the font DPI really makes it clear).

Closed by commit R8:e80856e8eb6a: Asign correct column width when importing XLS file (authored by davidllewellynjones, committed by pvuorela). · Explain WhyOct 30 2019, 7:41 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 69027

View Options

filters/sheets/excel/sidewinder/sheet.cpp

Loading...