[Bug 61474] New: [Patch] Adjusting of formulas in context of column shifting

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

[Bug 61474] New: [Patch] Adjusting of formulas in context of column shifting

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

            Bug ID: 61474
           Summary: [Patch] Adjusting of formulas in context of column
                    shifting
           Product: POI
           Version: unspecified
          Hardware: PC
            Status: NEW
          Severity: normal
          Priority: P2
         Component: POI Overall
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

In POI there is class FormulaShifter which deals with problem of adjusting cell
references inside formulas in case of shifting or copying of rows. This works
fine, but only for rows.
There is completely same need for reference-adjusting in the case of shifting
or copying of columns, and it is not implemented.
Algorithm for formula adjusting is pretty complex, with a lot of use cases, so
it would be good to use existing algorithm for columns, and not create an other
one.

Solution :
I've created class CompleteFormulaShifter, as an improved version of
FormulaShifter, with following enhancements :
- it takes formula string as input, not a list of Ptgs;
- it can work in both column and row modes.
Column mode is implemented in a simple way :
1. transpose cell references (switch column and row coordinates);
2. adjust references using existing algorithm for shifting of rows;
3. transpose new cell references back.

--
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 61474] [Patch] Adjusting of formulas in context of column shifting

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

Dragan Jovanović <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |PatchAvailable
                 OS|                            |All

--- Comment #1 from Dragan Jovanović <[hidden email]> ---
In order to make this work, I have made two minor changes in some old classes.
In FormulaShifter,  private static enum ShiftMode became public.
In HSSFCell,  protected CellValueRecordInterface getCellValueRecord() became
public.

--
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 61474] [Patch] Adjusting of formulas in context of column shifting

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=61474

--- Comment #2 from PJ Fanning <[hidden email]> ---
Dragan - there appears to be no patch attached. If you are still working on
this, could you add your patch as a Pull Request in github. It makes it easier
to review the patch. https://github.com/apache/poi

--
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 61474] [Patch] Adjusting of formulas in context of column shifting

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=61474

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

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

--
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 61474] Adjusting of formulas in context of column shifting

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=61474

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[Patch] Adjusting of        |Adjusting of formulas in
                   |formulas in context of      |context of column shifting
                   |column shifting             |
           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 61474] Adjusting of formulas in context of column shifting

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=61474

Dragan Jovanović <[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 61474] Adjusting of formulas in context of column shifting

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=61474

Dragan Jovanović <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #3 from Dragan Jovanović <[hidden email]> ---
Created attachment 35287
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=35287&action=edit
new class CompleteFormulaShifter, and two minor changes, in patch format

--
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 61474] Adjusting of formulas in context of column shifting

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=61474

--- Comment #4 from Dragan Jovanović <[hidden email]> ---
I could not create pull request (I tried to push changes, and got refused), so
I am sending a patch with changes.

--
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 61474] Adjusting of formulas in context of column shifting

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=61474

--- Comment #5 from PJ Fanning <[hidden email]> ---
Dragan - could you add unit test coverage?

--
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 61474] Adjusting of formulas in context of column shifting

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=61474

--- Comment #6 from Dragan Jovanović <[hidden email]> ---
Created attachment 35294
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=35294&action=edit
partial test case for CompleteFormulaShifter

Added junit test for CompleteFormulaShifter.
It's work in progress and it's not very detailed at the moment, but it should
be good enough to help you conclude is CompleteFormulaShifter conceptually
good.

--
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 61474] Adjusting of formulas in context of column shifting

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=61474

--- Comment #7 from Javen O'Neal <[hidden email]> ---
(In reply to Dragan Jovanović from comment #3)
> Created attachment 35287 [details]
> new class CompleteFormulaShifter, and two minor changes, in patch format

CompleteFormulaShifter appears to duplicate what some of the existing code
already does. I'd like to see this contribution have a neutral or a net
negative SLoC on the codebase.

I agree that I'd like to see column shifting (moving, copying, inserting,
deleting) implemented eventually. There are still quite a few open issues with
row shifting. Besides the open bugs, the API is weak and should be improved
(rewritten). I think it'd be wise to resolve the duplicated row-shifting code
in HSSF and XSSF and get a solid row-shifting API implemented before moving on
to column shifting, unless we can do so without increasing the amount of buggy
code with a poor API. Column operations are an order of magnitude more
difficult in POI than row operations due to the row-major storage.

Open bugs with "shift" in title:
bug 46742: Tracker: Remaining functionality for Sheet.shiftRows()
bug 56454: XSSFSheet.shiftRows(...) and HSSFSheet.shiftRows(...) incorrectly
handle merged regions that do not contain column 0.
bug 59733: shiftRows() causes
org.apache.xmlbeans.impl.values.XmlValueDisconnectedException
bug 57423: shiftRows() produces a corrupted xlsx file
bug 60072: Sheet shiftRows doesn't trigger charts position updating
bug 59731: Consolidate duplicated code for row shifting
bug 53320: FormulaShifter doesn't take care to absolute references
bug 58221: Shifting rows while trying to sort causes
XmlValueDisconnectedException
bug 59306: Shifting rows does not shift chart data series references
bug 56123: ShiftRows, Bug in POI 3.10 Beta 2: "Could not find 'internal
references' EXTERNALBOOK"
bug 54509: Cells in shifting rows are not used in column-referencing formulas.
bug 54533: Shifting rows does not shift the Zero Height flag

--
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 61474] Adjusting of formulas in context of column shifting

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=61474

--- Comment #8 from Dragan Jovanović <[hidden email]> ---
Thanks for comment. You're right that there is duplication in the code. I would
be willing to work on this to eliminate the duplication but that would probably
not result in net neutral SLoC because there is additional functionality.
Column shifting certainly is useful functionality which  provides value and
which we and others need. We will definitely be using it for our application
context.

I have taken a look at the issues that you have listed to see which issues are
really relevant. 46742 is certainly relevant as it sketches additional
functionality that would be useful. But this is really independent from column
shifting. Column shifting does not make these items much easier or harder to
fix. Many of the other bugs do not seem to really be very relevant (i.e. 53320,
54509). Unfortunately my employer is not very likely to support work on these
issues.

So overall I see this as your call. I can do work on eliminating the
duplications.  

Maybe I would be willing to fix some of the open issues on my own time. But if
you want to consider column shifting only after all the earlier bugs have been
resolved, that would make little sense to me.

--
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 61474] Adjusting of formulas in context of column shifting

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=61474

--- Comment #9 from Javen O'Neal <[hidden email]> ---
Let's rewind to what signature shifting columns should have.

Sheet.shiftRows(int startRow, int endRow, int n[, boolean copyRowHeight])

I wrote copyRows with a CellCopyPolicy object to avoid having a long signature.
Do we want something similar for a new shiftRows and shiftColumns method?

Usually when I use shiftRows, I have to shift from some insertion point to the
last row in the workbook so I don't shift rows onto existing rows. Can
shiftRows/shiftColumns be used with n>0 and endRow!=last row number?

Can we make shiftRows/shiftColumns behave as close to Excel as possible to make
it as intuitive to use as possible? I favor calling it insert rows instead of
shift rows since it's clearer that it isn't copying or cutting (though we
should provide that ability).

If all you have time for now is a clone of shiftRows behavior, we'll commit
that and incrementally improve it later if there's interest. It just means
breaking the API at a later date.

--
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]