[Bug 58787] New: [patch] Border Drawing utility that does not create unnecessary styles

classic Classic list List threaded Threaded
59 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[Bug 58787] New: [patch] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

            Bug ID: 58787
           Summary: [patch] Border Drawing utility that does not create
                    unnecessary styles
           Product: POI
           Version: 3.14-dev
          Hardware: PC
            Status: NEW
          Severity: normal
          Priority: P2
         Component: SS Common
          Assignee: [hidden email]
          Reporter: [hidden email]

Created attachment 33391
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=33391&action=edit
Patch file generated via Ant script

This new utility can be used to create "CellBorders" that can then be applied
to one or more sheets of the workbook without creating unnecessary intermediate
styles. In addition, this patch includes some helper methods to allow a
workbook to report it's own SpreadsheetVersion.

The patch has an example file that includes color borders, and removing
borders, and tests for changes to CellUtil.setCellProperty.

This patch depends on 58633.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [patch] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Mark Murphy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 OS|                            |All
         Depends on|                            |58633

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [patch] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #1 from Mark Murphy <[hidden email]> ---
A remaining issue, though not sure how important it is. This process creates
unnecessary <border> elements in the XLSX file. Largely because
CellUtil.setFormatProperties adds each piece of the border individually instead
of adding them in a single shot. This is likely to be the case no matter how
borders are drawn. A fix to this would be to hold all border properties in
their own HashMap and process that in one piece in
CellUtil.setFormatProperties. I have not done this. The same issue occurs with
fills, but not in the border drawing process.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [PATCH] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Dominik Stadler <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[patch] Border Drawing      |[PATCH] Border Drawing
                   |utility that does not       |utility that does not
                   |create unnecessary styles   |create unnecessary styles
           Severity|normal                      |enhancement
           Keywords|                            |PatchAvailable

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [PATCH] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Bug 58787 depends on bug 58633, which changed state.

Bug 58633 Summary: [patch] Need to set multiple CellStyle properties at once
https://bz.apache.org/bugzilla/show_bug.cgi?id=58633

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [PATCH] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |58879

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [PATCH] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787
Bug 58787 depends on bug 58879, which changed state.

Bug 58879 Summary: Return SpreadsheetVersion from Workbook
https://bz.apache.org/bugzilla/show_bug.cgi?id=58879

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [PATCH] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |NEEDINFO

--- Comment #2 from Javen O'Neal <[hidden email]> ---
It looks like all the changes in patch.tar.gz/patch.txt were committed in bug
58633 and bug 58879.

I think the two tests from TestCellUtil.java were committed as part of bug
58633. If not, could you rebase your changes, and rewrite to use junit4 instead
of junit3?

For CellBorder.java, you might want to use an enum here so that CellBorders can
be used in switch statements in Java 6, and also improves code quality due to
type checking. See SpreadsheetVersion's implementation if you need an example.
For another example, see o.a.p.ss.usermodel.HorizontalAlignment. Functions in
CellBorder.java should have unit tests before this is committed.

Per comment 1, if creating unnecessary intermediate <border> elements is still
an issue after bug 58633, please write a unit test for that.

Thanks for the DrawingBorders example!

==== Side-note about JUnit testing in POI ====

You'll find two different flavors of unit tests in Apache POI

There's junit3:
import junit.framework.TestCase;
public class TestSomeClass extends TestCase {
    public void testSomeMethod() { }
}

and junit4:
import org.unit.Test;
public class TestSomeClass {
    @Test
    public void testSomeMethod() { }
}

We're slowly trying to convert our junit3 tests over to junit4. There are a few
reasons for this:
* junit4 has new features that are more flexible
* inheritance: we don't need to inherit from TestCase, making it easier to
inherit from a base class that makes the tests more concise (see
BaseTestWorkbook, TestXSSFWorkbook)
* Don't need to build and maintain test suite classes--that is, classes that
just list the TestCase classes to be run.

New tests should be written in junit4.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Javen O'Neal <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[PATCH] Border Drawing      |[In progress] Border
                   |utility that does not       |Drawing utility that does
                   |create unnecessary styles   |not create unnecessary
                   |                            |styles

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Mark Murphy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33391|0                           |1
        is obsolete|                            |

--- Comment #3 from Mark Murphy <[hidden email]> ---
Created attachment 33455
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=33455&action=edit
Updated patch file containing quick guide modifications

I added a Quick guide paragraph for this enhancement and regenerated the patch
file. I also removed a redundant reference to the workbook object.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #4 from Mark Murphy <[hidden email]> ---
If you use the drawing borders example I added to the Quick Guide for
setCellProperties, it will not create extra styles. This enhancement really
adds some additional helpers around border drawing that will make it even
easier.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Mark Murphy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33455|0                           |1
        is obsolete|                            |

--- Comment #5 from Mark Murphy <[hidden email]> ---
Created attachment 33456
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=33456&action=edit
Rebased and updated again

This really only contains a new class CellBorders and a Quick Guide and
Examples. Not real sure how to write unit tests for this since I need to look
at the generated spreadsheet to determine if it worked or not. I could use some
coaching on that.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

Mark Murphy <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEEDINFO                    |NEW

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #6 from Mark Murphy <[hidden email]> ---
Created attachment 33457
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=33457&action=edit
This is a SVN diff of the quick-guide.xml

I used SVN to create a diff of the quick-guide.xml with the description of how
to use CellBorder.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #7 from Mark Murphy <[hidden email]> ---
I created a new object for this named CellBorder because I am working on cell
borders. However, I can see this being useful for defining and applying a bunch
of fills to a region as well. And, for that matter, there are other cell style
properties that could be applied in this manner. So I am conflicted on whether
this should be in a new class called CellBorder, or in the RegionUtil class. I
would need a slightly different approach to put it in the RegionUtil class.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #8 from Javen O'Neal <[hidden email]> ---
In general, I prefer creating inheritable classes rather than util functions
because
* behavior can be overridden in an object-oriented way
* features are more discoverable through Javadocs. People have to know CellUtil
exists and contains some goodies. Util classes get disorganized, turning into a
junk drawer where it's hard to search through the class to find what you want.
Splitting up a util class solves the junk drawer problem but  invites the
Wall-e spork clarification problem: if a function could belong in either of two
util classes, which one does it go in, and if people expect it in the opposite
one, possibly creating a new function that duplicates functionality.

Util functions make sense in some situations: return the maximum row number for
sheets in a workbook, or return diff two workbooks, or interleave the rows of
two spreadsheets (where there isn't an implied directionality, so that object
and subject in `object.action(subject)` are swappable.

In general, having higher level data structures/class allows higher level
client code, and that is a good thing.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #9 from Mark Murphy <[hidden email]> ---
Perhaps it should be called CellStyleTemplate instead of CellBorder. Then we
could have methods that add fills and other CellStyle properties, and then
apply them all at once.

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #10 from Javen O'Neal <[hidden email]> ---
Mark,

Did you take a look at RegionUtil to see if meets your needs? See BugĀ 54593.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #11 from Mark Murphy <[hidden email]> ---
(In reply to Javen O'Neal from comment #10)
> Mark,
>
> Did you take a look at RegionUtil to see if meets your needs? See BugĀ 54593.

Yes, in fact RegionUtil is the source of my previous queries. But RegionUtil is
slow since it takes a one at a time approach at setting cell properties. Thus
creating a lot of unused intermediate CellStyles. I didn't see it until I
created a moderately sized sheet. That was when I set about creating
setCellProperties(). But RegionUtil can't really utilize that method the way it
is written. CellBorder takes the approach of putting together the borders in a
separate object, then applying them to the sheet all at once. This should
perform significantly better. I am not sure if we should upgrade RegionUtil or
remove it. RegionUtil is a better name because it can include fills as well,
but it needs a better implementation. CellBorder does everything RegionUtil
does, but performs better, and may use less memory because all those unused
intermediate styles aren't created.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 58787] [In progress] Border Drawing utility that does not create unnecessary styles

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=58787

--- Comment #12 from Javen O'Neal <[hidden email]> ---
(In reply to Mark Murphy from comment #5)
> Not real sure how to write unit tests for this since I need to
> look at the generated spreadsheet to determine if it worked or not. I could
> use some coaching on that.

Sometimes writing unit tests is tough. At the bare minimum, a unit test can be
used to show someone how the code was intended to be used (this works great as
example code if you exclude the assertions). Unit tests should say what your
code should and should not do. For example, createBorder(region, ALL) should
create a left border on all cells in the first column of region, and should not
create a left border on any column to the right of the first column in the
region.

You can and should review your results by opening the file in Excel, but that's
probably the last time your feature will be tested using Excel unless there's a
bug.

Your unit test will have to assume some functionality is implemented
correctly--which is fair to say about anything that the test wasn't explicitly
written to test. This might mean writing:

cell = row.createCell(0);
//blank/empty cells don't have any style to start with.
assertEquals(0, Util.getNumOfBorders(cell));

Util.setBorder(cell, LEFT);
assertTrue(Util.isBorderSet(cell, LEFT));
// the right border should not be set
assertFalse(Util.isBorderSet(cell, RIGHT));

--
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

123