Skip to content

improving format of exported excel #1456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

improving format of exported excel #1456

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4513ab6
improving format of exported excel
schch Aug 31, 2017
4045db5
implemented requested changes and improved logging on format errors
schch May 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ganttproject-tester/test/biz/ganttproject/impex/csv/CsvImportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ private List<Pair<SpreadsheetFormat, Supplier<InputStream>>> createPairs(String.

private byte[] createXls(String... rows) throws Exception {
ByteArrayOutputStream stream = new ByteArrayOutputStream();
try (SpreadsheetWriter writer = new XlsWriterImpl(stream)) {
try (SpreadsheetWriter writer = new XlsWriterImpl(stream, "test")) {
for (String row : rows) {
for (String cell : row.split(",", -1)) {
writer.print(cell.trim());
Expand Down
2 changes: 1 addition & 1 deletion ganttproject-tester/test/biz/ganttproject/impex/csv/GPCsvImportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ private Map<String, Task> doTestImportAssignments(Supplier<InputStream> supplier

private byte[] createXls(String... rows) throws Exception {
ByteArrayOutputStream stream = new ByteArrayOutputStream();
try (SpreadsheetWriter writer = new XlsWriterImpl(stream)) {
try (SpreadsheetWriter writer = new XlsWriterImpl(stream, "test")) {
for (String row : rows) {
for (String line : row.split("\n", -1)) {
for (String cell : line.split(",", -1)) {
Expand Down
2 changes: 1 addition & 1 deletion ganttproject/src/biz/ganttproject/impex/csv/CsvRecordImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class CsvRecordImpl implements SpreadsheetRecord {

@Override
public String get(String name) {
return myRecord.get(name);
return (isSet(name)) ? myRecord.get(name) : new String();
Copy link
Contributor

@dbarashev dbarashev Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return "" instead of new String()

}

@Override
Expand Down
33 changes: 33 additions & 0 deletions ganttproject/src/biz/ganttproject/impex/csv/CsvWriterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import java.io.IOException;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.math.BigDecimal;
import java.util.Calendar;

/**
* @author akurutin on 04.04.2017.
Expand Down Expand Up @@ -53,4 +55,35 @@ public void close() throws IOException {
myCsvPrinter.flush();
myCsvPrinter.close();
}

@Override
public void newSheet() throws IOException {
println();
println();
}

@Override
public void newSheet(String name) throws IOException {
newSheet();
}

@Override
public void print(Double value) throws IOException {
myCsvPrinter.print(String.valueOf(value));
}

@Override
public void print(Integer value) throws IOException {
myCsvPrinter.print(String.valueOf(value));
}

@Override
public void print(BigDecimal value) throws IOException {
myCsvPrinter.print(value.toPlainString());
}

@Override
public void print(Calendar value) throws IOException {
myCsvPrinter.print(value.toString());
Copy link
Contributor

@dbarashev dbarashev Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this can also be replaced with String.valueOf like for numeric types, just for consistency.

}
}
23 changes: 11 additions & 12 deletions ganttproject/src/biz/ganttproject/impex/csv/GanttCSVExport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,14 @@ private SpreadsheetWriter getCsvWriter(OutputStream stream) throws IOException {
}

private SpreadsheetWriter getXlsWriter(OutputStream stream) {
return new XlsWriterImpl(stream);
return new XlsWriterImpl(stream, i18n("tasksList"));
}

public void save(SpreadsheetWriter writer) throws IOException {
writeTasks(writer);

if (myHumanResourceManager.getResources().size() > 0) {
writer.println();
writer.println();
writer.newSheet(i18n("resourcesList"));
writeResources(writer);
}
}
Expand Down Expand Up @@ -171,22 +170,22 @@ private void writeTasks(SpreadsheetWriter writer) throws IOException {
} else {
switch (defaultColumn) {
case ID:
writer.print(String.valueOf(task.getTaskID()));
writer.print(task.getTaskID());
break;
case NAME:
writer.print(getName(task));
break;
case BEGIN_DATE:
writer.print(task.getStart().toString());
writer.print(task.getStart());
break;
case END_DATE:
writer.print(task.getDisplayEnd().toString());
writer.print(task.getDisplayEnd());
break;
case DURATION:
writer.print(String.valueOf(task.getDuration().getLength()));
writer.print(task.getDuration().getLength());
break;
case COMPLETION:
writer.print(String.valueOf(task.getCompletionPercentage()));
writer.print(task.getCompletionPercentage());
break;
case OUTLINE_NUMBER:
List<Integer> outlinePath = task.getManager().getTaskHierarchy().getOutlinePath(task);
Expand All @@ -203,7 +202,7 @@ private void writeTasks(SpreadsheetWriter writer) throws IOException {
writer.print(getAssignments(task));
break;
case COST:
writer.print(task.getCost().getValue().toPlainString());
writer.print(task.getCost().getValue());
break;
case INFO:
case PRIORITY:
Expand Down Expand Up @@ -282,13 +281,13 @@ private void writeResources(SpreadsheetWriter writer) throws IOException {
writer.print("");
break;
case STANDARD_RATE:
writer.print(p.getStandardPayRate().toPlainString());
writer.print(p.getStandardPayRate());
break;
case TOTAL_COST:
writer.print(p.getTotalCost().toPlainString());
writer.print(p.getTotalCost());
break;
case TOTAL_LOAD:
writer.print(String.valueOf(p.getTotalLoad()));
writer.print(p.getTotalLoad());
break;
}
}
Expand Down
13 changes: 13 additions & 0 deletions ganttproject/src/biz/ganttproject/impex/csv/SpreadsheetWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,26 @@
package biz.ganttproject.impex.csv;

import java.io.IOException;
import java.math.BigDecimal;

/**
* @author akurutin on 04.04.2017.
*/
public interface SpreadsheetWriter extends AutoCloseable {
void print(String value) throws IOException;

void print(Double value) throws IOException;

void print(Integer value) throws IOException;

void print(BigDecimal value) throws IOException;

void print(java.util.Calendar value) throws IOException;

void println() throws IOException;

void newSheet() throws IOException;

void newSheet(String name) throws IOException;

}
52 changes: 49 additions & 3 deletions ganttproject/src/biz/ganttproject/impex/csv/XlsReaderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,13 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Iterators;
import com.google.common.collect.Lists;

import net.sourceforge.ganttproject.language.GanttLanguage;

import org.apache.poi.hssf.usermodel.HSSFWorkbook;
import org.apache.poi.ss.usermodel.Cell;
import org.apache.poi.hssf.usermodel.HSSFDateUtil;
import org.apache.poi.ss.usermodel.Row;
import org.apache.poi.ss.usermodel.Sheet;
import org.apache.poi.ss.usermodel.Workbook;

import java.io.IOException;
Expand All @@ -32,6 +36,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.text.SimpleDateFormat;

/**
* @author torkhov
Expand All @@ -40,10 +45,12 @@ class XlsReaderImpl implements SpreadsheetReader {

private final Workbook myBook;
private final Map<String, Integer> myHeaders;
private final SimpleDateFormat myDateFormat;

XlsReaderImpl(InputStream is, List<String> columnHeaders) throws IOException {
myBook = new HSSFWorkbook(is);
myHeaders = initializeHeader(columnHeaders);
myDateFormat = GanttLanguage.getInstance().getShortDateFormat();
}

@Override
Expand All @@ -53,11 +60,50 @@ public void close() throws IOException {

@Override
public Iterator<SpreadsheetRecord> iterator() {
return Iterators.transform(myBook.getSheetAt(0).iterator(), (input) -> new XlsRecordImpl(getCellValues(input), myHeaders));
Iterator<SpreadsheetRecord> iterator = null;

if(myBook.getNumberOfSheets() > 1) {
Iterator<SpreadsheetRecord> it = null;
for (int i = 0; i < myBook.getNumberOfSheets(); ++i) {
Sheet sheet = myBook.getSheetAt(i);
if (i < myBook.getNumberOfSheets() - 1) {
int lastRow = sheet.getPhysicalNumberOfRows();
sheet.createRow(lastRow + 1);
Copy link
Contributor

@dbarashev dbarashev Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little bit weird: why do we create rows when reading? Even if it makes no changes on the disk (I believe it doesn't) it worth adding a comment about the purpose of these two rows.

Copy link
Contributor Author

@schch schch May 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is because of the general CSV-Reader which expects to have two empty lines between tables e.g. tasks and ressources.

sheet.createRow(lastRow + 2);
}
it = Iterators.transform(sheet.iterator(), (input) -> new XlsRecordImpl(getCellValues(input), myHeaders));
iterator = (iterator == null) ? it : Iterators.concat(iterator, it);
}
}
else {
Copy link
Contributor

@dbarashev dbarashev Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you can safely remove else block and if condition and just do what is written in the true-case block no matter if you have just 1 sheet or more than 1.

iterator = Iterators.transform(myBook.getSheetAt(0).iterator(), (input) -> new XlsRecordImpl(getCellValues(input), myHeaders));
}

return iterator;
}

private List<String> getCellValues(Row row) {
return Lists.newArrayList(Iterables.transform(row, Cell::getStringCellValue));
return Lists.newArrayList(Iterables.transform(row,
(input) -> {
Copy link
Contributor

@dbarashev dbarashev Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like too much indent here. We use 4 spaces indent for continuations, but here the following formatting might be even better:

  return Lists.newArrayList(Iterables.transform(row, (input) -> {
    switch
  }));

switch (input.getCellTypeEnum()) {
case NUMERIC: {
if (HSSFDateUtil.isCellDateFormatted(input)) {
return myDateFormat.format(input.getDateCellValue());
}
else {
if (input.getCellStyle().getDataFormat() == (short)1) {
Copy link
Contributor

@dbarashev dbarashev Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks as if it produces the same result as subsequent else, no? Can we just call String.valueOf(input.getNumericCellValue()) no matter what data format is? Or even just call getStringCellValue in all cases when isCellDateFormatted returns false?

Copy link
Contributor Author

@schch schch May 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first line is for DATE formats the second line is for INTEGER values. The parser does not like to get a String with a . when it expects an integer.
If the cell is marked as NUMERIC using getStringCellValue failes, hence the switch statements.

return String.valueOf((int)input.getNumericCellValue());
}
else {
return String.valueOf(input.getNumericCellValue());
}
}
}
default:
return input.getStringCellValue();
}
}
));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ganttproject/src/biz/ganttproject/impex/csv/XlsRecordImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public String get(String name) {
if (index == null) {
throw new IllegalArgumentException(String.format("Mapping for %s not found, expected one of %s", name, myMapping.keySet()));
}
return myValues.get(index);
return (myValues.size() <= index) ? new String() : myValues.get(index);
Copy link
Contributor

@dbarashev dbarashev Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/new String()/"" ?

}

@Override
Expand Down
Loading