Skip to content

Conversation

Xcone
Copy link

@Xcone Xcone commented Oct 19, 2017

Hi Jing,

Here's the pull request for #61. Support for text-cells, yay! :-)

AutoFill behavior has slightly changed in this PR. This is intentional, as I believe it's for the better. Of course it's your project, so please review the new behavior and let me know if you agree with it or not. You can quickly view the "AutoFillSerialInputOutputTestDataProvider" method in the unit-test-class. There's a lot of I/O testing described in there.

Big picture of the solution:

  1. There's an AutoFillSequence, which covers the entire selected range.
  2. AutoFillSequence splits itself into AutoFillSections. Each section has its own diff-calculation and therefor it's own repeat pattern. AutoFillSection does no calculations itself. It just stores it.
  3. The section is made up out of AutoFillSectionEntries, each representing a single cell's value. This is the one calculating the diff and extrapolation to other entries. Because of this, it should be relatively easy to add AutoFillSectionEntries for DateTime and other datatypes.

Future benefits:
It should be relatively easy to refactor this a bit more to support custom AutoFillSectionEntries. All that's needed is the ability to customize for the AutoFillSectionEntryFactory from externally. Then the user can add his own data types and extrapolation logic.

Notable Differences:
You may remember this remark in #61
[1],[4],[2],[5],[3],[6]. With an avarage increase of 1 per cell I would've expected: [4],[7],[5],[8],[6],[9]. But instead Reogrid produces: [9,625][12,625][10,625][13,625][11,625][14,625]. The pattern is there, but the calculation of the avarage increase per cell is bust.

You can now see in the unit-test it produces "7, 10, 8, 11, 9, 12, 13, 16, 14".
Over a span of 6 cells, the value increased from 1 to 6, which avarages to 1-per-cell. The pattern then repeats after 6 cells. Meaning it starts with:
<BaseValue (1)> + <AvgPerCell(1)> * <length of the pattern (6) = 7.
After that the new basevalue is 7, en the rest will be added/substracted based on the differences in the pattern.
So the comma-values is fixed. But repeats patterns, it does not continue patterns.

Another difference would be the workings of sections. Take for instance:
1, 2, null, 3, 4, null
You might expect it to continue as
5, 6, null, 7, 8, null
Instead, because the null is in the middle, it counts as 2 separate sections, which calculate it's diff and extrapolate independently. So it'll continue as:
3, 4, null, 5, 6, null, 5, 6, null, 7, 8, null

So this last one I'm not too sure if you like it. It makes sense to me from technical stance. After all, there's an empty cell, so why wouldn't it be separate sequences? But I don't know if you and your userbase appreciate the technical stance on this.
I might be able to fix this for null-cells. But when instead the pattern becomes:
1,2,SomeText,3,4,SomeText
it'll become vague. So I actually rather not work around it, unless you think it's a must.

My expectation on the grand scale is that these tricky patterns are rare anyway. :-)

Love to hear what you think!

@Xcone
Copy link
Author

Xcone commented Oct 19, 2017

Oh, I forgot one more notable behavior difference.
AutoFilling from 1 cell no longer assumes a +1.
So where:
1,2 autofills to 3,4,5
When starting 1 cell:
1 autofills to 1,1,1

This is consistent with Excel and made sense to me. The assumed increase of +1 seems a bit arbitrary to me. But if you rather have the assumed increase of +1 each cell, it shouldn't be hard change that.

@jingwood
Copy link
Member

Excellent! Thanks!

The refactoring made the code more clear and flex. The text-autofill feature also works fine! And I can run your tests from Visual Studio directly.

Because of this, it should be relatively easy to add AutoFillSectionEntries for DateTime and other datatypes.

Nice, I like it!

So this last one I'm not too sure if you like it. It makes sense to me from technical stance. After all, there's an empty cell, so why wouldn't it be separate sequences? But I don't know if you and your userbase appreciate the technical stance on this.

Input Output
ReoGrid 1, 2, null 3, 4, null, 5, 6, null, ...
Excel 1, 2, null 3, 4, null, 5, 6, null, ...
ReoGrid 1, 2, null, 3, 4, null 3, 4, null, 5, 6, null, ...
Excel 1, 2, null, 3, 4, null 4.6, 5.3, 6, 6.7, 7.4, null, ...

OK, let's create our own rule by using your pull-request, and wait for user feedback. 👍
And I think the null-related process is not so important, we can ignore it for the time being.

This is consistent with Excel and made sense to me. The assumed increase of +1 seems a bit arbitrary to me.

No problem, agreed to this change.

I just noticed that there are some special cases that Excel does.

Input Output
ReoGrid a1, aa2, aaa3 a1, aa2, aaa3, ... No autofill
Excel a1, aa2, aaa3 a2, aa3, aaa4, ...
ReoGrid a1, a2, 3c, 4d a1, a2, 3c, 4d, ... No autofill
Excel a1, a2, 3c, 4d a2, b3, 3c, 4d, ...

Seems Excel stops at first cell which does not follow [text-number] pattern. But I think these are rare cases and can be ignored now.

I will merge this change and prepare for a NuGet package release at this weekend. If you are interested in the future development please feel free to create any new pull-request.

@Xcone
Copy link
Author

Xcone commented Oct 20, 2017

Glad to hear you like it!

I don't see myself becoming a regular contributer. But if our users give us more feedback for improvements that would benefit Reogrid as a whole, I'll make sure to be in touch again.
Perhaps I'll jump in if I happen to see an issue being opened about Autofilling.

Thanks for accepting the change, and I'll be looking forward to the NuGet update! 👍

@jingwood jingwood merged commit b27d7be into unvell:master Oct 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants