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.

Sign up for GitHub

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

Jump to bottom

Fix removeAll method in sections #2141

Merged
merged 4 commits into from
Feb 18, 2021
Merged

Fix removeAll method in sections #2141

merged 4 commits into from
Feb 18, 2021

Conversation

mats-claassen
Copy link
Member

Override the removeAll(where:) method as it causes Duplicate Tag errors

Fixes #2048

mtnbarreto
mtnbarreto requested changes Feb 16, 2021
View reviewed changes
Source/Core/Section.swift Outdated
}
kvoWrapper.rows.removeObjects(in: removedRows)

for row in removedRows {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 line of code....
x.foreach {
x.map {

Source/Core/Section.swift Outdated
kvoWrapper.rows.removeObjects(in: removedRows)

for row in removedRows {
row.willBeRemovedFromSection()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

willBeRemovedFromSection , but the rows were already removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the same as for removeAll(keepCapacity:

mtnbarreto
mtnbarreto requested changes Feb 17, 2021
View reviewed changes
Source/Core/Form.swift Outdated

public func removeAll(where shouldBeRemoved: (Section) throws -> Bool) rethrows {
let indices = try kvoWrapper._allSections.enumerated().filter { (element) -> Bool in
return try shouldBeRemoved(element.element)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the short syntax .filter { try shouldBeRemoved($0.element )}

Source/Core/Form.swift Outdated
let indices = try kvoWrapper._allSections.enumerated().filter { (element) -> Bool in
return try shouldBeRemoved(element.element)
}.map { $0.offset }
var removedSections: [Section] = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var removedSections = [Section]()

Source/Core/Section.swift Outdated
@@ -324,6 +324,19 @@ extension Section: RangeReplaceableCollection {
}
}

public func removeAll(where shouldBeRemoved: (BaseRow) throws -> Bool) rethrows {
let indices = try kvoWrapper._allRows.enumerated().filter { (element) -> Bool in
return try shouldBeRemoved(element.element)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

→

Source/Core/Section.swift Outdated
let indices = try kvoWrapper._allRows.enumerated().filter { (element) -> Bool in
return try shouldBeRemoved(element.element)
}.map { $0.offset }
var removedRows: [BaseRow] = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

→

@mtnbarreto
Copy link
Member

😎

mtnbarreto
mtnbarreto approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@mtnbarreto mtnbarreto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪💪💪💪 groso Mathi!

@mats-claassen mats-claassen merged commit 15ca571 into master Feb 18, 2021
@mats-claassen mats-claassen deleted the fix/section_removeAll branch February 18, 2021 15:44
@jserrafindster
Copy link

jserrafindster commented Mar 11, 2021 •
edited
Loading

this is causing me some trouble, any idea when we will see a new release?

@jserrafindster
Copy link

Just installed 5.3.3 and im stilll having this issue

#0 0x0000000182c8186c in __exceptionPreprocess ()
#1 0x0000000197c9cc50 in objc_exception_throw ()
#2 0x0000000182b87000 in +[NSException raise:format:arguments:] ()
#3 0x0000000183f1b91c in -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] ()
#4 0x000000018582cb20 in -[UITableView endCellAnimationsWithContext:] ()
#5 0x0000000185846164 in -[UITableView endUpdatesWithContext:] ()
#6 0x0000000101aab760 in FormViewController.rowsHaveBeenRemoved(:at:) at /Users/my-user/my-app/Pods/Eureka/Source/Core/Core.swift:732
#7 0x0000000101aac72c in protocol witness for FormDelegate.rowsHaveBeenRemoved(_:at:) in conformance FormViewController ()
#8 0x0000000101b4fee0 in Section.KVOWrapper.observeValue(forKeyPath:of:change:context:) at /Users/my-user/my-app/Pods/Eureka/Source/Core/Section.swift:94
#9 0x0000000101b516fc in @objc Section.KVOWrapper.observeValue(forKeyPath:of:change:context:) ()
#10 0x0000000183fa98e0 in NSKeyValueNotifyObserver ()
#11 0x0000000183fabe70 in NSKeyValueDidChange ()
#12 0x0000000183fab8dc in -[NSObject(NSKeyValueObservingPrivate) _changeValueForKeys:count:maybeOldValuesDict:maybeNewValuesDict:usingBlock:] ()
#13 0x0000000183ee43a0 in -[NSObject(NSKeyValueObservingPrivate) _changeValueForKey:key:key:usingBlock:] ()
#14 0x0000000183fa597c in _NSSetObjectValueAndNotify ()
#15 0x0000000101b4f150 in Section.KVOWrapper.removeAllRows() at /Users/my-user/my-app/Pods/Eureka/Source/Core/Section.swift:78
#16 0x0000000101b58920 in Section.removeAll(keepingCapacity:) at /Users/my-user/my-app/Pods/Eureka/Source/Core/Section.swift:320
#17 0x0000000100696114 in closure #1 in SettingsVC.reloadDevicesSection() at /Users/my-user/my-app/app/UI/Settings/SettingsVC.swift:369
#18 0x00000001005e07d8 in thunk for @callee_guaranteed () -> () ()
#19 0x00000001005e0830 in thunk for @escaping @callee_guaranteed () -> () ()
#20 0x0000000185b76638 in +[UIView(Animation) performWithoutAnimation:] ()
#21 0x0000000100695e08 in SettingsVC.reloadDevicesSection() at /Users/my-user/my-app/app/UI/Settings/SettingsVC.swift:368

@mats-claassen
Copy link
Member Author

Please open a new issue and add details for the exception you are seeing. What is the error message? It seems you are calling removeAll(keepingCapacity) and not removeAll(where:). So please also add some code context info

@navyseai navyseai mentioned this pull request Apr 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtnbarreto mtnbarreto mtnbarreto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent state/crash when removing rows from sections with removeAll(where:)
3 participants
@mats-claassen @mtnbarreto @jserrafindster