Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor data validation handling to use indexes #1207

Open
wants to merge 10 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@Pankraty
Copy link
Member

commented May 12, 2019

Continuation of #1205
Fixes #1188
Fixes #1071

I refactored data validation handling. Adding data validation rules now works much faster:

Before

100 - Action done in 00:00:00.7601712
200 - Action done in 00:00:00.1502512
400 - Action done in 00:00:00.7007287
800 - Action done in 00:00:02.5187801
1600 - Action done in 00:00:09.3206459
3200 - Action done in 00:00:32.6455856
6400 - Action done in 00:02:06.8365056
12800 - Action done in 00:08:23.8926057

After

100 - Action done in 00:00:00.4760072
200 - Action done in 00:00:00.0275315
400 - Action done in 00:00:00.0685165
800 - Action done in 00:00:00.1309623
1600 - Action done in 00:00:00.2648254
3200 - Action done in 00:00:00.5943090
6400 - Action done in 00:00:01.3928023
12800 - Action done in 00:00:03.0208176
25600 - Action done in 00:00:04.3245792
51200 - Action done in 00:00:08.7342740

Copying a worksheet from #1188 now finishes in 13.5 sec (quite good progress from 23 minutes, I think :))

This PR includes breaking changes.

  • IXLDataValidation.Ranges made readonly
  • To add or remove ranges from the data validation rule the user must use dedicated methods (AddRange, AddRanges, ClearRanges, RemoveRange)
  • A single data validation rule cannot apply to ranges from different worksheets (it was a bug that this was possible)

Besides, there were some changes in public API that do not break the existing code but break the binary compatibility.

  • IXLRanges.Remove now returns bool (before it was void)
  • IXLRangeBase.RangeAddress extracted to a base interface IXLAddressable

@Pankraty Pankraty force-pushed the Pankraty:1188_DVRefactoring branch 2 times, most recently from 2901382 to 1803f51 May 16, 2019

@Pankraty Pankraty referenced this pull request May 16, 2019

Open

Poor performance with data validation #1071

1 of 2 tasks complete
@igitur

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Please rebase.

@igitur igitur self-requested a review May 29, 2019

@igitur igitur added this to the v0.95 milestone May 29, 2019

@Pankraty Pankraty force-pushed the Pankraty:1188_DVRefactoring branch from 1803f51 to 9699c04 May 29, 2019

@Pankraty

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

79764d0 speeds up the loading of the file from #1221 from 2:50 to ~13 sec

@Pankraty Pankraty force-pushed the Pankraty:1188_DVRefactoring branch from 79764d0 to 74b1d89 Jun 5, 2019

@Pankraty

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Rebased

if (!_dataValidations.Remove(dataValidation))
return;
var xlDataValidation = dataValidation as XLDataValidation;
xlDataValidation.RangeAdded -= OnRangeAdded;

This comment has been minimized.

Copy link
@igitur

igitur Jun 11, 2019

Member

Should this class not implement IDisposable and events be unhooked upon disposal? Remind me what's best practices for event handlers, please.

This comment has been minimized.

Copy link
@Pankraty

Pankraty Jun 11, 2019

Author Member

Agree. Events ought to be unsubscribed. It may make no difference in 99% of cases but the rest 1% may be a real pain in the neck, so it is safer to always unsubscribe explicitly.
In this particular case, if I am not mistaking, preserving a single reference to the data validation rule may prevent the whole collection from being garbage collected.
WIll fix.

@igitur

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

You rebased this on develop after #1205 was merged, right?

@Pankraty

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

You rebased this on develop after #1205 was merged, right?

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.