Bug 60605 - Convert Workbook.SHEET_STATE_HIDDEN, VISIBLE, and VERY_HIDDEN to enum
Summary: Convert Workbook.SHEET_STATE_HIDDEN, VISIBLE, and VERY_HIDDEN to enum
Status: NEEDINFO
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: unspecified
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks: 59836
  Show dependency tree
 
Reported: 2017-01-19 06:43 UTC by Javen O'Neal
Modified: 2017-01-20 06:07 UTC (History)
0 users



Attachments
Code changes for Option 3 and Option 4 (4.55 KB, text/x-java)
2017-01-20 04:52 UTC, Javen O'Neal
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Javen O'Neal 2017-01-19 06:43:59 UTC
Could be converted to

enum SheetState {
  VISIBLE(0),
  HIDDEN(1),
  VERY_HIDDEN(2);
}
Comment 1 Greg Woolsey 2017-01-19 07:03:03 UTC
We don't need the instance constructor argument, just use enumInstance.ordinal() 

I do like replacing all integer constants like this with enums.
Comment 2 Javen O'Neal 2017-01-20 04:23:58 UTC
Implemented in r1779558.
Comment 3 Javen O'Neal 2017-01-20 04:36:14 UTC
While working on this, I read the javadoc comment for setSheetHidden(int sheetIndex, int state) that the active sheet cannot be hidden, but we do not enforce this.

Presumably, saving a workbook with a hidden active sheet would result in Excel complaining about a corrupted workbook when opened (untested).

There are a few ways we can handle this:
1) Make the recommendation in the javadoc but do not enforce this rule
2) Option 1 plus logging a warning that an active sheet was hidden
3) throw an IllegalStateException when trying to hide an active sheet
4) Activate the next visible sheet if hiding the active sheet. Log a warning that setSheetVisibility had this un-asked for side-effect. If there are no other visible sheets, throw an IllegalStateException (this is closest to what Excel does, but may make for some rare bugs).
5) Same as number 4, but defer checking until saving the workbook (throwing an IllegalStateException at this point would be fatal and could cause problems if the workbook is partially serialized).

Which behavior is least surprising?
Comment 4 Javen O'Neal 2017-01-20 04:52:07 UTC
Created attachment 34653 [details]
Code changes for Option 3 and Option 4
Comment 5 Javen O'Neal 2017-01-20 06:07:09 UTC
@Test
public void testActiveSheetIsHidden() throws IOException {
    Workbook wb = _testDataProvider.createWorkbook();
    wb.createSheet("First Sheet");
    wb.setSheetVisibility(0, SheetVisibility.HIDDEN);
    java.io.FileOutputStream fos = new java.io.FileOutputStream("/tmp/hidden"
            + "." + _testDataProvider.getStandardFileNameExtension());
    wb.write(fos);
    wb.close();
    fos.close();
    
    wb = _testDataProvider.createWorkbook();
    wb.createSheet("First Sheet");
    wb.setSheetVisibility(0, SheetVisibility.VERY_HIDDEN);
    fos = new java.io.FileOutputStream("/tmp/veryhidden"
            + "." + _testDataProvider.getStandardFileNameExtension());
    wb.write(fos);
    wb.close();
    fos.close();
}

Excel 2013 behavior:
hidden.xls and veryhidden.xls open without a warning with no sheets visible. After adding a sheet to hidden.xls, the menu to unhide the first sheet is available.
hidden.xlsx and veryhidden.xlsx open with a
> "We found a problem with some content in 'veryhidden.xlsx'. Do you want us to try to recover as much as we can?"
corrupted workbook dialog.

LibreOffice 4.0.4.2 behavior:
all 4 files open with the first sheet visible.

If we implement this to match Excel's behavior, we would have an inconsistency between HSSF and XSSF.