Misc. updates/fixes/enhancements for extensibility and useability

[WoozyG]

Interested to see what you think of these.

Some of these are enabling simpler/cleaner extension and modification
in consuming apps, others are widening of support or cleanup of
behaviors I've been using. Much of it is replacing custom logic that
evaluates things that ultimately map to Excel formulas with POI formula
evaluations. This eliminates duplication, improves support for complex
expressions, and leverages the wide test base for POI formula
evaluation.

NOTES:

DefaultHyperlinkCellClickHandler -
has been revamped to avoid parsing the formula with a RegEx. There are
too many runaway cases and missed edge formula syntax examples. Much
simpler and reliable to just use a formula evaluation trick.

The class is no longer implemented as static functions. Properties have
been added/exposed making it much easier to customize behavior in
applications if desired.

Spreadsheet -

• removed unused field topLeftCellHyperlinksLoaded (it was causing a
  conditional to always be false incorrectly)
• standardized initialization a bit, removing some implicit constructor
  initialization in favor of moving things to the init() method and giving
  users a way to customize implementations.
• cleanup to fix minor bugs I've been handling in subclasses, like more
  places to clear formula result cache to avoid displaying stale data,
  keeping tab selection and active tab in sync, etc.
• cell lookup should check for sheet name in refs, and if present, honor
  it
• connector state could throw ArrayIndexOutOfBoundsException if new rows
  are added/referenced programmatically in some paths
• POI 4 supports using conditional formatting rules to modify cell value
  formatting, so that evaluator needs to be cached, passed, and cleared
  like FormulaEvaluator
• extend support for selecting by ranges to include formula-defined
  ranges

---

This change is 

[tulioag]

In addition to the other comments, there also seems to be a lot of formatting changes. Those should be reverted.

[tulioag]

Reviewed 2 of 10 files at r1.
Reviewable status: 1 of 12 files reviewed, 4 unresolved discussions (waiting on @tulioag and @WoozyG)

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/DefaultHyperlinkCellClickHandler.java, line 118 at r4 (raw file):

            UI ui = UI.getCurrent();
            final Navigator navigator = ui == null ? null : ui.getNavigator();
            if (address.startsWith("#!") && navigator != null) {

If the navigator is null, should we enter the next clause and treat the address as an inter-sheet address? If not, we should put the line final Navigator navigator = ui == null ? null : ui.getNavigator(); and null check inside the if

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/DefaultHyperlinkCellClickHandler.java, line 140 at r4 (raw file):

     * @param address
     */
    protected void navigateTo(Cell cell, String address) {

Can we keep this method private? If it is supposed to be extended or used by subclasses, we should improve the documentation.

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/SpreadsheetUtil.java, line 550 at r4 (raw file):

                getAllReferencedCells(eval, spreadsheet, cells);
        } else if (rawEval instanceof ExternalNameEval) {
            // TODO: now what? Ignore this?

Do you know under what conditions this would happen? What would be the consequences of just ignoring this?

[WoozyG]

Reviewable status: 1 of 12 files reviewed, 4 unresolved discussions (waiting on @tulioag)

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/DefaultHyperlinkCellClickHandler.java, line 118 at r4 (raw file):

Previously, tulioag (Tulio Garcia) wrote…

If the navigator is null, should we enter the next clause and treat the address as an inter-sheet address? If not, we should put the line final Navigator navigator = ui == null ? null : ui.getNavigator(); and null check inside the if

Good thoughts. I've updated the logic accordingly.

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/DefaultHyperlinkCellClickHandler.java, line 140 at r4 (raw file):

Previously, tulioag (Tulio Garcia) wrote…

Can we keep this method private? If it is supposed to be extended or used by subclasses, we should improve the documentation.

Yes, it should be private. I've updated the class.

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/SpreadsheetUtil.java, line 550 at r4 (raw file):

Previously, tulioag (Tulio Garcia) wrote…

Do you know under what conditions this would happen? What would be the consequences of just ignoring this?

This would typically be a name in a different workbook. If the instance is of this type, as far as I can tell that means POI didn't have that workbook registered in the current evaluation scope, and could proceed no further with expression evaluation. Throwing an exception here alerts the user to the unresolved external reference, so they can fix it. For my purposes at least, this is helpful for people designing our reports and other spreadsheets, telling them the copy they are working with hasn't been fully cleaned up wrt local references.

If it is just ignored, things would proceed, but some values may be blank when they would be expected to have values, with no indication of why. Formula syntax being so complex sometimes, it is helpful to have this error, I think. Would not affect most users, only very complex workbooks that may have originated with links to other workbooks.

POI can still handle that, but needs special coding to register all known external workbooks in the evaluation context. It's hard to get right, I've played with the unit tests some.

[tulioag]

Reviewable status: 1 of 12 files reviewed, 2 unresolved discussions (waiting on @tulioag and @WoozyG)

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/DefaultHyperlinkCellClickHandler.java, line 118 at r4 (raw file):

Previously, WoozyG (Greg Woolsey) wrote…

Good thoughts. I've updated the logic accordingly.

 UI ui = UI.getCurrent();

That could also be inside the if block

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/SpreadsheetUtil.java, line 550 at r4 (raw file):

Previously, WoozyG (Greg Woolsey) wrote…

This would typically be a name in a different workbook. If the instance is of this type, as far as I can tell that means POI didn't have that workbook registered in the current evaluation scope, and could proceed no further with expression evaluation. Throwing an exception here alerts the user to the unresolved external reference, so they can fix it. For my purposes at least, this is helpful for people designing our reports and other spreadsheets, telling them the copy they are working with hasn't been fully cleaned up wrt local references.

If it is just ignored, things would proceed, but some values may be blank when they would be expected to have values, with no indication of why. Formula syntax being so complex sometimes, it is helpful to have this error, I think. Would not affect most users, only very complex workbooks that may have originated with links to other workbooks.

POI can still handle that, but needs special coding to register all known external workbooks in the evaluation context. It's hard to get right, I've played with the unit tests some.

Ok, sounds good to throw an exception. I Just think we could create a more meaningful exception to be throw in that case.

[tulioag]

Reviewable status: 1 of 12 files reviewed, 3 unresolved discussions (waiting on @tulioag and @WoozyG)

[WoozyG]

Reviewable status: 1 of 12 files reviewed, 3 unresolved discussions (waiting on @tulioag)

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/DefaultHyperlinkCellClickHandler.java, line 118 at r4 (raw file):

Previously, tulioag (Tulio Garcia) wrote…

 UI ui = UI.getCurrent();

That could also be inside the if block

Done.

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/SpreadsheetUtil.java, line 550 at r4 (raw file):

Previously, tulioag (Tulio Garcia) wrote…

Ok, sounds good to throw an exception. I Just think we could create a more meaningful exception to be throw in that case.

Actually, digging deeper into POI, this will never be the final result of a formula evaluation - either it will be unwrapped to actual values, or POI will throw a similar RuntimeException if it can't find the external workbook. So I can just remove this clause entirely, as it is effectively unreachable.

[tulioag]

Reviewed 3 of 10 files at r1, 2 of 5 files at r2, 1 of 3 files at r3, 2 of 4 files at r4, 3 of 3 files at r8.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @tulioag and @WoozyG)

---

vaadin-spreadsheet/src/main/java/com/vaadin/addon/spreadsheet/Spreadsheet.java, line 1040 at r8 (raw file):

Previously, WoozyG (Greg Woolsey) wrote…

Here, the conditional was wrong before, and now is correct.

Ok. Nice catch.

[tulioag]

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @WoozyG)

[tulioag]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @WoozyG)

[tulioag]

Dismissed @WoozyG from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved